On 11/7/18 9:28 PM, Santiago Torres-Arias via aur-general 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. - The optdepends description on wine is a bit confusing in my opinion. - 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. - I noticed that you didn't add a LICENSE file for this package.
This checks out, Artistic2.0 is a common license.
- 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.
I don't see why you'd need the overhead of git for this, and that url is not going to change. They "document" it here: https://blog.github.com/2011-10-21-github-secrets/#diff-patch They've provided it for a very long time specifically in order to let people do this, they're not going to change the scheme and render it useless for the very purpose it was created for. :p ... cgit and gitweb let you download patch files too, GitHub just doesn't expose a visible link for it.
- I noticed that you didn't add a LICENSE file for this package.
PKGBUILD claims to be GPL2, which is a common license, but upstream has no licensing information I can find...
- gam-git:
Am I missing something? I only see a "gam" package.
- I'm not sure if this would work when built in a chroot due to those touch calls.
Are you referring to the ones in package()? Not sure why upstream code needs such weird things but AFAICT it should not break just because of a chroot.
- 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)
Looks like it just copies a couple python modules into a directory and then creates a wrapper script to run them. What would you suggest running in build(), exactly? Devendoring looks good to me too, though.
- 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'm not sure what the script does either. gam.py does do: import utils from var import * Which should really be explicit relative imports, but it's actually legal in python2, and there doesn't seem to be any other reason to want to cd to the directory, but the script does not cd there anyway.
- 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 see two mostly similar issues, the other one has a diff referenced. https://github.com/jay0lee/GAM/issues/created_by/ainola Worth noting, it's much easier to get upstream projects to accept these if you provide a button they can click on to implement it -- that means pull requests.
- I noticed that you didn't add a LICENSE file for this package.
Once again, Apache license is a common license and doesn't need to be installed. -- Eli Schwartz Bug Wrangler and Trusted User