[aur-general] TU Membership Application

[Eli Schwartz]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20181107/3205dfe0/attachment.asc>