[aur-general] TU membership application

[Jean Lucas]

On Sat, 2019-08-17 at 22:51 -0400, Santiago Torres-Arias wrote:
> On Fri, Aug 16, 2019 at 03:19:56PM -0400, Jean Lucas via aur-general
> wrote:
> > Hi all,
> > 
> > Thank you for your time, and thank you to all who help make Arch a
> > great OS!
> 
> Always happy to help! :)
> 
> It's customary to review PKGBUILDS for new applicants.  This is
> somewhat
> of a quick/cursory review over 3 random packages as I've been in
> conferences for the whole week.

Thank you for the review!

> 
> == Overall ==
> 
> - It appears you need quote strings way more everywhere, from deps,
> to licenses
>   to variables....
> - Consider that base-devel is assumed to exist for makedepends and
> (iirc).

As Eli explained, the quotes are unnecessary unless you need to do
variable expansion, and makepkg yells at you if you include a space
somewhere. As for array keys, I think that no quotes looks way prettier
>:)

And yes, I've been working on the assumption that base-devel is
installed for makedepends.

>   
> == Beaker ==
> 
> - This depends array has to be wrong

I've been working on parsing namcap output to make my life a little
easier, so I'm basically throwing what namcap outputs w.r.t. deps at a
script I wrote that looks up the missing libraries and spits out the
needed packages, and I throw that into the depends of pkgbuilds. I do
know that some dependencies are subdependencies of more higher-level
packages (i.e. the higher-level packages already pull the
subdependencies in) but I haven't yet scripted a way to intelligently
omit those subdependencies.

I don't think it is harmful to be very verbose on those dependencies,
but I do make sure I work from an empty depends array to exactly what
namcap tells me, as well as interpreting readmes and reading through
the actual software being packaged. Then I test all my packages in
clean chroots (especially graphical applications) to ensure I have the
minimal amount of deps needed.

As for depending on Electron, Beaker builds a self-contained Electron
app, hence the specific need for all the dynamic dependencies (that
something like wire-desktop can leave for community/electron to
resolve, since it does not bundle Electron).

As for glibc/gcc-libs, yes this seems like quite an involved topic with
a lot of angles. Arch is glibc-based, they're both in base, so they
could *probably* be omitted - I'm working on the fact that namcap tells
me I need them :X

> - This makedepends array too. you should make sure things aren't
> depending on
>   py2 anymore

Py2 isn't officially EOL *yet* - that's in January 2020 :) but I prefer
to let upstream switch their dependency to Py3 because - I'm not a
Python expert here so please correct me if I'm wrong - there could be
some form of incompatibility when manually hacking in a Py3 build,
especially for something as complex as a browser.

> - I'm also a little confused, did you take over the namespace of
> another
>   project called beaker? Why not just call this beaker browser?

I don't have an airtight answer for this - I liked the named beaker
more, and saw it used officially just about everywhere except the
domain name and the GitHub user name. I also followed the train of
logic that Firefox isn't named firefox-browser, nor Chromium chromium-
browser - but then again, I was also unaware of the existence of an
existing project called Beaker. I didn't see it in the AUR nor the
official repos, so I went ahead and solicited the namespace change.

> 
> == Oxy ==
> 
> - I think you should document why you're cherry-picking that commit
> rather than
>   using a tag. Admittedly this is probably upstream's fault, but
> still, better
>   to be clear.

You're right - better to be clear. I will document my cherry-picks from
now on. But yeah, not tagging your releases is kind of annoying.

> - Again, I think your depends are either too verbose or wrong.  

This goes back to the glibc/gcc-libs point above :)

> 
> == stf ==
> 
> - This appears to me it's a -bin package

I use this package every day - its definitely not a bin / it builds the
platform.

> - npm -i -g --prefix seems like a good way to overwrite a bunch of
> system files
>   and/or cause a bunch of file conflicts

As Eli mentioned, this is basically the standard way of building NPM
packages. I customarily check the file tree of my packages and make
sure things are neat and don't collide.

> - I think you can use $pkgname more often, namely when resolving the
> url and
>   resolving the tgz file

After reading Eli's reply to this point, I can see a point for why one
would want to hardcore $pkgname everywhere (for namespace changes). I
basically use $pkgname if its shorter than typing the actual pkgname,
haha. But I really think a package maintainer should always be
reviewing build/packaging instructions, and a $pkgname change, as
normally glacial/infrequent as that is, should be very obvious to a
maintainer during rebuild.

For URLs - those can change a lot, and be radically different e.g.
switching from PyPI to GitHub, so hardcoding $pkgname there is IMO a
bad idea, and should be evaluated on a case-by-case basis.

For source naming schemas, you do want the source tarball to be
$pkgname, so I could see the usage there.

> - I'm curious to know where you got those depends arays, they seem to
> be a
>   little off... do you really need python, graphicksmagic and
> protobuf to
>   basically extract a tarball?

So yes, since this is a full build and not just a tarball extraction,
those are precisely the minimal amount of dependencies needed for
build.

> - I'm also not sure why *everything* is just blindly put on /usr

So this is basically the only pattern I've seen for Arch NPM packages,
it seems correct to me because system-wide node_modules go in /lib. As
Eli mentioned, its basically akin to the DESTDIR="$pkgdir" PREFIX=/usr
for Makefile pattern. Either way, as mentioned, I check my packages in
case of any lingering other-Linux-distro-specific files or otherwise,
although thats not so common IME, especially for NPM packages.

> 
> == Conclusion ==
> 
> - I think you are on the right path, but some decisions made me
> wonder whether
>   your sponsors actually reviewed the PKGBUILDS with you. 

I believe that my profile was posted for review among the TUs when I
solicited Alexander's sponsorship, and he and Robin (although later
confirmed not a sponsor) relayed to me one package with more obvious
errors - generating keys which affected reproducible builds, as well as
grepping the system for a binary which was considered questionable from
a security POV - in searx-git. Robin also relayed to me a few other
less troublesome issues (like removing -git/-bin package variants from
conflicts, source naming and pkgver schema fixes).

I believe Sergej went over a few of my packages at least before giving
an OK for sponsorship, but that was 2 weeks after I had requested
sponsorship, at which point I had cleaned up a lot of the errors that
were raised :)

Still - I do acknowledge that I could've had much better communication
with everyone involved, and if I don't get accepted for TU this time
around, if I try again later, I'll be sure to do my best to not commit
the same mistakes again.

> 
> Hope this helps,
> -Santiago
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20190819/f62f217b/attachment.sig>