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