On 8/17/19 10:51 PM, Santiago Torres-Arias via aur-general 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.
I haven't looked at Jean's packages myself, but I'm not sure some of these things you point to are actually problems.
== Overall ==
- It appears you need quote strings way more everywhere, from deps, to licenses to variables....
Quote strings are only necessary for variable expansions which could potentially undergo word splitting. For declaring an array (deps, licenses) it's completely unnecessary as you know when writing the PKGBUILD if there are spaces (and most makepkg fields don't permit spaces anyway). Admittedly I think single-quoting array keys looks prettier. That's a personal opinion though.
- Consider that base-devel is assumed to exist for makedepends and (iirc).
This is not great if it is in makedepends, but honestly we still haven't fully fixed the official repos for this.
== Beaker ==
- This depends array has to be wrong - This makedepends array too. you should make sure things aren't depending on py2 anymore
What's necessarily wrong with this? I don't like py2 either, but just because something uses it doesn't mean it has no reason to. What specifically made you think it isn't needed? What is wrong with the dependencies that it "must" be wrong? From a cursory inspection it seems to be some sort of electron thingy, which would hopefully use community/electron but life isn't perfect. Depending on glibc and gcc-libs is a bikeshed topic that TUs/Devs don't agree on. The rest of the dependencies could plausibly be linked to by whichever version of prebuilt electron is being downloaded by the build system.
- I'm also a little confused, did you take over the namespace of another project called beaker? Why not just call this beaker browser?
== 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.
Upstream is amazing and doesn't use git tag. The cherry-picked commit has the commit message "Call it a version". This is obvious enough causes that I don't actually feel bad about the lack of comments. :)
- Again, I think your depends are either too verbose or wrong.
There's exactly one depends, which is gcc-libs. Again, a bikeshed topic. I will loudly proclaim my own belief in not depending on gcc-libs or anything else in *base*, but I won't tell anyone they are *wrong* for doing so themselves. (Obviously makedepends on base-devel is still against the packaging guidelines.)
== stf ==
- This appears to me it's a -bin package
Why? It looks like some sort of standard js-based source package on the NPM registry.
- npm -i -g --prefix seems like a good way to overwrite a bunch of system files and/or cause a bunch of file conflicts
npm install -g --prefix="$pkgdir" is actually how you are supposed to install npm stuff -- it "globally" installs it to the packaging directory so that pacman will install it to /usr, so it should never conflict with anything. This seems fine to me. (I have personal issues with npm as a technology, and prefer to npm install into $srcdir then use cp because it feels at least mildly cleaner -- see my rapydscript-ng package -- but stf doesn't seem any less valid and some official packages do the same thing he does).
- I think you can use $pkgname more often, namely when resolving the url and resolving the tgz file
I've seen it both ways extremely often. I think some people actually insist on hardcoding $pkgname everywhere, because they want to preserve the possibility of users forking the PKGBUILD, modifying the pkgname, and still have everything work without having to fix up all pkgname references.
- 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?
Not to extract a tarball. This is npm. It's not just extracting a tarball, it is also probably downloading half the internet during build, and maybe compiling G-d-knows-what after the unauthenticated download. Because npm. And npm sucks. But the package itself seems fine, and I'd need to actually look in depth at the build in order to decide if those makedepends raise a red flag.
- I'm also not sure why *everything* is just blindly put on /usr
It's not? npm install (like make install except that npm is obviously terrible because javascript desktop programming) is responsible when given --prefix="$pkgdir/usr" to place "everything" in the places where npm thinks they should go. It's like passing DESTDIR="$PKGDIR" PREFIX=/usr to a Makefile. It seems totally like the correct thing to do for an npm project.
== Conclusion ==
- I think you are on the right path, but some decisions made me wonder whether your sponsors actually reviewed the PKGBUILDS with you.
If this is the worst that the applicant has, then that's not very bad at all. :D :D -- Eli Schwartz Bug Wrangler and Trusted User