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