On 07/13/2018 10:11 AM, Filipe Laíns via aur-general wrote:
Hey,
First of all I just want to say that I have 58 packages on AUR and most of the PKGBUILDs (written by me) were written before I knew some of this. I tried to update most of them but as it's a really monotonous task, I missed some things. Eli, thanks for pointing them out. Also, most of these packages were orphan and I adopted them, I did not fix some of the mistakes right away because I didn't know these were indeed mistakes. With the time I learned about them but I didn't fix some of the packages because I have a lot of them. I have been fixing them as people point it out or when the PKGBUILD needs to be manually updated. Lately I have been making an effort to fix everything but apparently it wasn't enough.
Very few people are "good enough" when I really get on a roll... I generally consider it sufficient that people learn from my critiques, fix them promptly, and incorporate that feedback into further efforts. The only thing that really startled me, actually, were the packages which build ELF binaries but were marked as "any" packages.
cellular-network-configs-git: - unquoted srcdir/pkgdirThis was fixed in commit 4a4273f72a93824a16a2c1e86308986b26d9df54[1] This was fixed by commit 4a4273f72a93824a16a2c1e86308986b26d9df54[1] which is dated to 11 days ago so I don't understand.
As with the other unquoted variables issues... it may have been committed 11 days ago but my git pull from when I started my review did not have that yet, perhaps you never pushed it? I did say some of those issues were already fixed before I sent off my email -- I did not go back to re-analyze the packages I'd already looked at based on the fixed versions. ;)
cm256cc: - are the mv commands needed or not? - depends on boost but may only need that as makedepends, see if runtime depends could get away with only boost-libs The package installs the 64bit libraries in 'lib64' and 32bit ones in 'lib'. I am not comfortable enough to edit the CMakeLists file but if anyone wants to submit a patch, I would be happy to accept it :)
Then I would guard this by if [[ $CARCH = x86_64 ]]; then do_64_bit_things else do_32_bit_things fi Don't ignore unexpected errors, if there's no lib64 folder on x86_64 builds then something went wrong and either should be fixed, or old workarounds should be removed -- either way using || true, means you'll never know.
nodejs-nan: - should build from source tarball instead of pulling from the server at buildtime - nodejs packages need to fix non-deterministic chmod 777 on directories, see https://wiki.archlinux.org/index.php/Node.js_package_guidelines and https://github.com/npm/npm/issues/9359 Oh my god, this guiidelines are extremely wrong. Npm uses symlinks by default. If you follow this guidelines, "$pkgdir"/usr/lib/node_modules/module_name will be symlinked to "$srcdir"/$pkgname-$pkgver/module_name-module_version.
A correct approach would be:
noextract=("$pkgname-$pkgver.tar.gz") ...
package() { npm install -g --user root --prefix "$pkgdir"/usr "$srcdir"/$pkgname-$pkgver.tar.gz
... }
nodejs packaging confuses me incredibly. I've only got one package which uses nodejs, and this is what I do: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=rapydscript-ng-git#n3... I build it in build() and then I manually cp things to $pkgdir and write my own symlink for /usr/bin scripts. It seems to work okay...
vr180-creator: - electron app with no links to source is marked as MIT for the electron component, source archive contains binary node modules so cannot debundle electron without source, cannot find license for app itself Google hasn't released the source yet afaik. I will rename the package as -bin. Fixed the license issue.
writefull: - proprietary app using electron is marked as MIT, app.asar contains binary robotjs and spellchecker modules which can probably be rebuilt against and use system electron package - arch-dependent binaries should be installed to /usr/lib not /usr/share Fixed the license issue. I will rename the package as -bin as I don't whish to rebuild the modules.
For your three electron apps, you get to have fun, because electron is fun! Haha, I lied, it's not fun at all. They're basically always bin packages because electron. Debundling is complicatedly weird, but I've successfully done so for e.g. community/keybase-gui. When I get around to it I need to create a new wiki page documenting how to handle this correctly... I definitely understand if you've got no will to make proper non-bin packages, that being said it would be neat if you could do so anyway since having dozens of outdated, insecure copies of prebuilt electron (thanks, electron devs!) is pretty dreary and also wasteful of disk space. -- Eli Schwartz Bug Wrangler and Trusted User