On 10/14/18 3:49 PM, Daniel Bermond via aur-general wrote:
After felling confident with Arch itself, I've started to contribute packages to the AUR. I can perfectly remember my first one: ffmpeg-full-git. It was, and still is, a pleasure to maintain it, firstly because I was in need for it, and, secondly, because I was contributing back to the community something that was useful for me. Things evolved quickly, I started to maintain more and more packages that I was also in need for, while adopting other orphaned ones, and currently I'm the maintainer of 170+ packages[2].
A bit late to a TU review once again, but I've got some reviews for your AUR packages here. I'd also like to acknowledge that some of these you've likely already fixed, especially the provides/conflicts on -git variants... but I cloned your PKGBUILDs at the beginning of the discussion period and haven't pulled your changes. Some packages were very similar to each other but enabled different build options, e.g. the ffmpeg family -- I may not have mentioned each by name but reviews may apply to multiple packages: adriconf: - should not conflict with the -git variant - inconsistent use of $variable vs. ${variable} apache-flex-sdk: - Uses prebuilt releases for open-source code, that's the job of -bin packages. At the very, very least, this is basically completely unsuitable for moving to community... - Why does it explicitly disable stripping? - Pointless prepare function creates copy of file instead of doing the exact same thing in package() autotrace-nomagick: - Pointless provides for libautotrace.so, nothing needs it and the main autotrace package doesn't provide it. - inconsistent use of $variable vs. ${variable} blackmagic-decklink-sdk: - This package contorts itself in order to download a file that should just be local:// and have the user download it. If you can make a simple DLAGENTS=() downloader to download it properly, even better -- but reinventing makepkg inside the prepare function is just not something that should be done. caffe2-cuda: - has the ugly "don't run make because it recompiles it in make install anyway" been reported upstream? Why not just patch the makefile to not depend on the default/all target? caffe2: - url supports https - While most variables are quoted, exclude_dirs and exclude_libs are not. - exclude_dirs and exclude_libs are created by doing word splitting on the output of find -- use print0 and mapfile -t -d '' caffe: - url supports https - Makefile mangling is ugly, try python -c 'import sys; print("%s.%s" % sys.version_info[0:2])' or better yet, complain loudly to upstream that source code should not need to be manually cofigured in a world where `pkg-config --cflags python3` exists. - Why are checks disabled? If you absolutely want to enforce them as off by default, use BUILDENV+=(!check), then let users opt in by running makepkg --check - provides/conflicts are ugly and backwards -- primary packages should not conflict with git versions, variants need to provide and conflict the primary package chromaprint-fftw: - chromaprint package in [extra] is LGPL, this package is GPL. Source code is in fact MIT and happens to mention that it's linked to an LGPL library -- that being ffmpeg, which this package does not use. - Why mention the possibility of linking to ffmpeg, when the whole point is to not be like the [extra] package which does so? - should not conflict the -git version of the base chromaprint package - Why does this provide libchromaprint.so, which nothing can depend on since the [extra] package does not provide it? - Eplicitly sets options to the default OFF state, for tests and tools. Would it be possible to enable this testsuite -- which the [extra] package does not. confu-git: - Why is this command-line tool provided as a split python2/python3 package? What does the python2 version actually benefit? crossc: - Upstream license is no longer unknown (!!!) and is now Apache. - base package should not conflict the -git version - - Update: current version is fixed davs2-git: - What is this if ./configure; then make; else cd "$srcdir" && rm -rf build-10bit; fi davs2: - What is this if ./configure; then make; else cd "$srcdir" && rm -rf build-10bit; fi deluge-git: - url supports https - no need to conflict the -stable package, everything should provide and conflict the base deluge package - install script should not delete users as that's a potential security hole - create users using sysusers.d, create data directories using tmpfiles.d and avoid reserving a static UID eglexternalplatform-git: - doesn't install MIT license; adopted but never fixed egl-wayland-git: - doesn't install MIT license - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build fann: - url supports HTTPS - should not conflict the -git version ffmpeg-decklink: - should not conflict -git version of base package - source supports HTTPS ffmpeg-full: - url supports HTTPS - conflicts is full of useless things - prepare does some sort of dance around profile.d, but current makepkg versions re-source /etc/profile after installing deps ffnvcodec-headers-git: - I'd create the LICENSE file during prepare, not package... firetools: - source url supports HTTPS flif: - instead of dancing around each install target, why not skip the messaging and simply build/install all targets in one go flite1-patched: - why does this even exist? I see there was a build error for the actual flite1 package, and every patch there seems to be something the main package should presumably use. fontconfig-ubuntu: - for patch in $(cat file) is bad shell scripting, why not while read line < file, instead? This is less wasteful than shelling out to cat, and avoids possibly unwanted word splitting fp16-git: - The commented-out makedepends and build/check functions are obviously in fact checkdepends instead... right? This should be unified in check(), and check should not be commented out since it should be a user choice to run it without requiring tedious modification of the PKGBUILD. Also makepkg has --nocheck/--check for those who want it. frei0r-plugins-git: - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build fs-uae-launcher: - adopted but never fixed many glaring issues, including being generally quite messy - build function does preparatory patching, referencing outside the BUILDDIR -- there's a comment to the effect since June, adopting packages without fixing their issues means no one else can fix them instead. fs-uae: - adopted, never updated to migrate off of post_install scripts and onto pacman hooks - url supports HTTPS fxdiv-git: - The commented-out makedepends and build/check functions are obviously in fact checkdepends instead... right? This should be unified in check(), and check should not be commented out since it should be a user choice to run it without requiring tedious modification of the PKGBUILD. Also makepkg has --nocheck/--check for those who want it. gst-plugins-intel-msdk: - should not conflict with -git variant gst-plugins-ugly-git: - style: source array indenting is a bit erratic htmldoc: - url supports https - should not conflict with -git variant imagemagick-full: - url supports https - should not conflict with -git variant - please add the check() function corresponding to the package in [extra] - why are these directories configured via a variable at the top of the PKGBUILD? I don't think they'll ever be anywhere else. intel-graphics-compiler-git: - only needs to conflict with one base package for compute-runtime - makedepends on things in base-devel intel-media-sdk-git: - cloning in build seems unfortunate, and looking at the git-lfs docs it seems like it should be entirely possible to configure the working copy created by makepkg, then run 'git checkout' for git-lfs to re-apply all filters and checkout the real file contents intel-media-sdk: - sed line in srcver could probably be replaced by bash substring expansion, unless the algorithm "drop the century" is likely to change - should not conflict with -git variant intel-media-server-studio: - instead of many printf's I suggest using cat << __EOF__ > foo.pc (it replaces variables unless you \escape them) intel-media-stack-bin: - only needs to conflict with one base package for intel-media-sdk intel-seapi-git: - only needs to conflict with one base package for intel-ittnotify intel-seapi: - instead of running 3 commands to get the python major.minor from an unstable --version meant for humans, use the API: python -c 'import sys; print("%s.%s" % sys.version_info[:2])' - should not conflict with -git variant isobmff-git: - why rename the submodules? If there's ever any reason to clone them for something else, a shared SRCDEST would allow deduplicating the clones. kde-gtk-config-git: - adopted but never updated; install script is deprecated by pacman hooks kitematic: - yay, an electron app... have you investigated trying to debundle and build it against the system electron package kvazaar: - should not conflict with -git variant laptop-mode-tools: - find command would be more efficient if it used + instead of ; and also prevents the need to quote in order to avoid being treated as a bash syntax token. No need to quote {} either way. lib32-lame: - sourceforge download supports https lib32-lzo: - gcc-multilib is deprecated, and was always part of the multilib-devel group. It's currently provided by the gcc package, which cannot actually generate -m32 code without lib32-gcc-libs installed. lib32-nvidia-utils-beta: - could use less hackish bash/subprocesses by e.g. echo "${_soname%.so.*}.so" to strip the sover, while read -rd '' line < <(find ... -print0), _soname=${_lib##*/}/... lib32-schroedinger: - source url should use https libde265-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build libemf: - sourceforge source supports https libfpx: - "avoid download errors when the upstream version is updated and this package is not yet updated." -- this package updates itself by curling the version every time you execute makepkg, pls revert immediately. libheif-git: - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build libilbc: - provides itself, conflicts with itself minus lib prefix, seems to be a mistake - no need to conflict -git variant libmfx: - prepare step explicitly overrides source extraction in order to strip the pkgver-versioned extracted sources and dump everything directly in srcdir -- why? - no need to enable staticlib, it doesn't strip static libraries unless they correspond to a shared library anyway - should not conflict with -git variant libmysofa-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. libmysofa: - should not conflict with -git variant libopenmpt-svn: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. libopenshot-audio-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. libopenshot-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - the imagemagick dance in build() seems odd, the package should depend on a specific interface if it's compiled into the package libpciaccess-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build - git clone over https to take advantage of TLS verification - url supports https libquvi0.4: - sourceforge download supports https libquvi-scripts0.4: - sourceforge download supports https libraqm-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build libreoffice-extension-vero: - url supports https - style: inconsistent use of tabs and spaces - noextract/prepare dance could be moved to package - unquoted srcdir libsvm: - url supports https - instead of running 3 commands to get the python major.minor from an unstable --version meant for humans, use the API: python -c 'import sys; print("%s.%s" % sys.version_info[:2])' - these scripts and python modules are top-level stuff using some rather generic names, which may not be the best idea... libtorrent-rasterbar-git: - autotool.sh should be done in prepare, see if it can be replaced by autoreconf libumem-git: - patch tries to do way too much, including trimming trailing whitespace, editing redhat spec files, etc. - resulting patchwork adds files and causes makepkg's automatic git checkout to not fully restore the incremental build tree to a patchable state. You can use git apply --index to apply the patch and stage it in git, then the next time makepkg resets the source tree to the current upstream HEAD, it will clean up the indexed files at the same time. libva-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - arch-meson passes --buildtype=release and a number of custom meson flags, but --prefix=/usr --buildtype=plain should do the trick in 95% of cases -- obviously the script would not exist if everyone agreed with me, but I do still consider it worth thinking about why you are using it. Arch typically does not provide complex wrappers and macros to obscure the fundamental underpinnings of the build system in a PKGBUILD. :) libva-intel-driver-git: - why does this "replaces" the non-git version??? - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - arch-meson passes --buildtype=release and a number of custom meson flags, but --prefix=/usr --buildtype=plain should do the trick in 95% of cases -- obviously the script would not exist if everyone agreed with me, but I do still consider it worth thinking about why you are using it. Arch typically does not provide complex wrappers and macros to obscure the fundamental underpinnings of the build system in a PKGBUILD. :) libva-utils-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - arch-meson passes --buildtype=release and a number of custom meson flags, but --prefix=/usr --buildtype=plain should do the trick in 95% of cases -- obviously the script would not exist if everyone agreed with me, but I do still consider it worth thinking about why you are using it. Arch typically does not provide complex wrappers and macros to obscure the fundamental underpinnings of the build system in a PKGBUILD. :) libvpx-full-git: - url supports https - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - should not conflict with -git variant libvpx-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - should not conflict with -git variant littleutils-full: - no comment, except I was startled to see both bash and dash in the dependencies. Is there really software which requires a bash installation but sometimes doesn't use it in favor of explicitly /bin/dash (and not even /bin/sh ?) lzma_alone: - both source and url support https mame-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. megaglest-data-git: - CCPL license is one of the common licenses, and definitely not custom - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. midori-bzr: - makedepends on base-devel packages (pkg-config actually does not exist anymore, it's provided by pkgconf) - why does this clean out the build directory, do incremental builds not work? - should not conflict with -git variant - why are there both bzr and git packages? which one has the canonical development? - post_install docs don't seem very useful mpv-full-git: - url supports https - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - should not conflict with -git variant mpv-full: - url supports https - should not conflict with -git variant mujs: - url supports https - should not conflict with -git variant mupen64plus-extraplugins-git: - url supports https - if these are all separate repos and you need to find the latest pkgver of the whole set, maybe it makes sense to package them as different PKGBUILDs... mupen64plus-git: - instead of unsetting _component, consider making that a local too. ;) - url supports https mupen64plus-gui-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. ncmpcpp-git: - url supports https - autoreconf should be run in prepare, then configure in build, instead of autogen.sh in build - should not conflict with variants ndi-sdk: - Just wanted to say I admire your care in deblobbing that giant *.run script in order to extract the contents without running it. +1 nnpack-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - staticlibs is not needed if the package only has static libs nvidia-beta: - url and source support https - I'm skeptical of using the deprecated $startdir to add patches to source=(), instead of explicating them... but either way, this breaks on directories with spaces in them. After carefully quoting "$startdir" you pass it through word splitting again via for i in $(...), but you could have just done standard globbing -- for i in "$startdir"/*.patch; do echo "patchfile is ${i##*/}"; done - install script is deprecated by 60-linux.hook which runs depmod for any module no matter who installs it. - users can probably take care of reloading their X session to get a new nvidia blob on their own... openshot-bzr: - url supports https - why is there both a git version and a bzr version? openshot-git: - url supports https - why is there both a git version and a bzr version? pax: - adopted but never updated, PKGBUILD metadata could use some cleaning up of unused things, self-provides, etc. - url and source support https pdfchain: - sourceforge source supports https pingo: - This package contorts itself in order to download a file that should just be local:// and have the user download it. If you can make a simple DLAGENTS=() downloader to download it properly, even better -- but reinventing makepkg inside the prepare function is just not something that should be done. pitivi-git: - package has been broken for a very long time now, cf. https://bbs.archlinux.org/viewtopic.php?id=237173, you adopted but did not fix it. pkgcacheclean: - you adopted someone else's paccache clone written in C and uploaded straight to the AUR (the AUR is not meant to provide *source code* hosting). I suggest having it be deleted instead... pngcrush-bundled: - pngcrush with bundled libpng/zlib of "quirky" usefulness doesn't seem like the sort of thing which would produce smaller png files without modifications to zlib/libpng itself, what's up with that? psimd-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo -- okay, I'll concede with this header-only repo it's unlikely to matter pthreadpool-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - staticlibs is not needed if the package only has static libs pybind11: - Why doesn't this package function simply call python setup.py install --root="${pkgdir}" - url supports https - not really necessary to copy python2/python3 copies of the build directory as the setup.py does not use cheap tricks like 2to3 - testsuite should not be disabled by default python2-vmaf: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. - _executable loop can just be while read -rd '' executable ... < <(find ... -print0), or really just for executable in scripts/*.py - should not conflict with -git variant - consider submitting pull request upstream for PEP 394 compliance, to use proper python2 shebang python-glog: - not really necessary to copy python2/python3 copies of the build directory as the setup.py does not use cheap tricks like 2to3 - I notice upstream has a testsuite, this should be run in check() python-leveldb: - BSD license must be installed - PyPI url contains unpredictable hashes, see https://wiki.archlinux.org/index.php/Python_package_guidelines#Source python-lmdb: - I notice upstream has tests, these should be run in check() - not really necessary to copy python2/python3 copies of the build directory as the setup.py does not use cheap tricks like 2to3 python-ninja-syntax: - this seems to be some sketchy kitware fork of private implementation details of the community/ninja package, why does its project page just say "UNKNOWN" and point urls at ninja itself - PyPI url contains unpredictable hashes, see https://wiki.archlinux.org/index.php/Python_package_guidelines#Source - not really necessary to copy python2/python3 copies of the build directory as the setup.py does not use cheap tricks like 2to3 python-nvd3: - PyPI url contains unpredictable hashes, see https://wiki.archlinux.org/index.php/Python_package_guidelines#Source - not really necessary to copy python2/python3 copies of the build directory as the setup.py does not use cheap tricks like 2to3 python-opcodes-git: - no need to rename source clone, let people with shared SRCDEST only download one copy of a given source repo. .... I didn't end up going further than this, but I noticed some common themes that I liked: - you're pretty reliable about quoting - you're pretty reliable about naming sources uniquely - your packages are usually pretty well written to work as expected And some that I didn't like: - oftentimes, urls and sources could and should be upgraded to use https, something that Devs/TUs are admittedly not historically careful about either, but we are working on it as indicated by this TODO list: https://www.archlinux.org/todo/use-gpg-signatures-and-https-sources/ Of course, the TODO list has been outstanding for like 2 years now, because it's rather boring administrivium to fix (I find it easier to do so when already modifying a PKGBUILD) - you often disable testsuites or don't include them at all, which is probably along the same logic as having previously removed PGP checks. I don't expect this to be a problem for community packages, but I think they're both issues that should be fixed in the AUR too. makepkg has options to disable both, if users don't want to waste time running these, and IMHO they should be opt-out. - personal nitpicks about some of the bash scripting you use to get the job done in exotic cases -- Eli Schwartz Bug Wrangler and Trusted User