[aur-general] TU Membership Application

[Brett Cornwall]

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 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 248 bytes
Desc: not available
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20181107/f422d5b4/attachment.asc>