On 11/07/18 09:28pm, Santiago Torres-Arias wrote:
Hello Brett.I took some time to randomly sample your PKGBUILDs and give some feedback: - ags: - it appears that you use sed to change CFLAGS in the makefile definition, although it appears that the Makefile itself lets you overwrite them. I'd advice trying to use native tooling as possible, and to try to get familiar with the toolchain of each package as much as possible.
I will admit that I didn't think to go through those lines when I inherited it. You're totally right, there's no reason to do it that way.
- The optdepends description on wine is a bit confusing in my
opinion.
I removed it. There's little reason to have it there anyway. The optdepends isn't a good place to inform people about that anyway.
- I marked the package as out-of-date, as there appears to be a new
version (3.1.4.15) as of almost two months ago.
Long story short, that was pretty much exactly during the time when I accidentally clobbered my urlwatch file. Thanks for bringing that up to me.
- I noticed that you didn't add a LICENSE file for this package.
Artistic2.0 is a uncommonly used common license! (/usr/share/licenses/common/Artistic2.0/license.txt)
- hib-dlagent:
- I see that you backported a patch on this and ags. I was rather
surprised to see that neither patches were added to new
tags/releases. You could, however, cherry pick the commits rather
than depending on the github api (which can change) to compute the
diff for you. For this, you could use the git transport on
makepkg.
That would bring another dependency on git, though. I can surely do if if it's more 'correct' but I wouldn't imagine that Github would change that API anytime soon.
Or would it be better to just carry the patch locally in the repo?
- I noticed that you didn't add a LICENSE file for this package.
Yikes, the project doesn't even have a license! I should have checked that when I inherited it (the packager just slapped a GPL2 on it). Really, I had just uploaded it so it wouldn't have been lost after the AUR 4 migration.
I'll bug upstream about it.
- gam-git:
- I'm not sure if this would work when built in a chroot due to
those touch calls.
- After reviewing the package I doubt this doesn't need a build()
step. Otherwise I'd label this package a -bin. This is something
that we should take special consideration of, since we could be
unwittingly be introducing binaries that aren't hardened when
building.
(I could be wrong on this one, since it for some reason vendors
many well-known packages inside of it. Good job for not pulling it
those vendored deps :D)
- I'm confused as to why gam.py needs to be put inside
/usr/share/gam and add a .sh entrypoint for it in /usr/bin. The
file seems to have a shebang and be executable...
- I see that here you *also* are providing a patch. I also could
find that you submitted an issue upstream for said patch (but not
the patch itself)[1]. I like your initiative! Do try to keep the
number of backported patches to a minimum to keep things
manageable.
- I noticed that you didn't add a LICENSE file for this package.
Of all the packages you had to click on that one. :(I know it doesn't really excuse it but gam is sort of a "WIP" because it's... oddly written. I've been meaning to set aside some time to get some patches in to make it more palatable for packaging. The patch is a complete hack right now just to make the package "work" (when I inherited it it was FUBAR).
I will probably send more feedback, but I also don't want to overwhelm you with this and all the other reviews around.
I really appreciate the feedback! It always sucks when so many little things become so glaring after the fact but
signature.asc
Description: PGP signature
