Post by Daniel Bermond via aur-generalAfter felling confident with Arch itself, I've started to contribute
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