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
signature.asc
Description: This is a digitally signed message part