[aur-general] TU Application: David Runge
Eli Schwartz
eschwartz at archlinux.org
Mon Oct 23 03:44:39 UTC 2017
On 10/17/2017 07:07 PM, David Runge wrote:
> my name is David Runge. You may have come across me somewhere on the
> wiki [1], the forums [2] (not so recently), the AUR [3] or the mailing
> lists (mainly arch-general). I'm sponsored by Ray Rashif.
General commentary on your AUR packages, since xxarhtna is being a
slacker who slacks:
crypted-backups-git:
- empty variables and arrays are messy
- unquoted $pkgdir, though $srcdir is consistently quoted
- the pkgver() fallback for non-tagged repos can be simplified
- build() does nothing, why does it exist?
- source urls for git repositories should be cloned over https:// rather
than git:// in order to take advantage of cacert verification
- source urls do not need to be prepended with "${_pkg}::" when they
don't actually need to be renamed
- there is no need to `install -d ... && install -Dm644 ...`, just do
the second part
easytranscript:
- unquoted $pkgdir, though $srcdir is consistently quoted
- inconsistent quoting of array elements; why does license have double
quotes?
- there is no need to `install -d ... && install -Dm644 ...`, just do
the second part
firefox-extension-tab-tree:
- pkgdesc is self-referential, see the wiki for guidance on a good
pkgdesc: https://wiki.archlinux.org/index.php/PKGBUILD#pkgdesc
- depends does not need to have a versioned Firefox requirement
- why is "${source##*/}" obfuscated when it will always be the same
thing?
grub-customizer:
- empty variables and arrays are messy
- unquoted $pkgdir, though $srcdir is consistently quoted
- there is no need to `install -d ... && install -Dm644 ...`, just do
the second part
- package() also runs the compilation and `sed`s the desktop file, there
is a good reason we now do split build/package functions...
- the use of `|| return 1` is deprecated, makepkg sets errexit
- inconsistent quoting of array elements; why is provides=() alone in
not quoting this? why does license have double-quotes?
- why is there a backslash escape for a multiline source=() array?
- there is no need for pkgbranch, use ${pkgver%.*} instead, but if you
do use it you should name it _pkgbranch to mark it as a custom var
- the install file is outdated, we have pacman hooks for that now
- consider specifying a reason for the optdepends
jackcpp-git:
- unquoted $pkgdir and $srcdir
- source urls for git repositories should be cloned over https:// rather
than git:// in order to take advantage of cacert verification
- source urls do not need to be prepended with "$_basename::" when they
don't actually need to be renamed
- the url should use HTTPS
- this is marked as an "any" package when it contains compiled ELF
binaries which is a huge no-no
- by the way, the nonstandard ordering of makepkg variables had me
looking for a second to make sure you actually listed everything
khard-git:
- inconsistent quoting of array elements; why does license have double
quotes?
- source urls do not need to be prepended with "$_basename::" when they
don't actually need to be renamed
- by the way, the nonstandard ordering of makepkg variables had me
looking for a second to make sure you actually listed everything
- unquoted $pkgdir
- build() does nothing, it should run python setup.py build
- setuptools installations usually also use --skip-build --optimize=1 in
the package() function
- why do you explicitly use !emptydirs instead of the makepkg.conf
default, does the package depend on it?
liblo-git:
- unquoted $pkgdir and $srcdir
- source urls for git repositories should be cloned over https:// rather
than git:// in order to take advantage of cacert verification; in
sourceforge's case that means using the following url:
https://git.code.sf.net/p/liblo/git
- source urls do not need to be prepended with "$_basename::" when they
don't actually need to be renamed, but see the above point
- provides a hardcoded version of liblo, $pkgver should be fine but if
not, consider using ${pkgver%%.r*}
- autogen should really go in prepare(), also autogen should die and be
replaced by autoreconf in nearly every case. If they really have weird
special sauce in there, at least tell autogen to not run ./configure
mantisbt:
- this incredibly gross install script messes around with unpackaged
files, please don't do this
nextcloud-news-updater:
owncloud-news-updater:
- what is a "parllel" feed updater anyway?
- source tarball violates shared SRCDEST, $pkgver.tar.gz clashes with
other packages that have the same version.
- unquoted $pkgdir and $srcdir
- `python setup.p install --skip-build` in package() should be preceded
by `python setup.py build` in build() the same way `make` is separated
from `make install` in Makefile powered packages.
- I'm not sure why your lengthy post_install script which does nothing
other than echo documentation instructions, needs to also tell the
user how to systemctl, this is probably massive overkill. If there's
something unusual about this package more than any other systemd
services, then that's probably okay to warn the user about.
patchbook-git
- cloning a git source to a $pkgver specific directory more or less
destroys the purpose of using git in the first place. Moreso for a
foo-git package since the pkgver will update on every revision,
invalidating the cached source
- there is no need to `install -d ... && install -Dm644 ...`, just do
the second part
- when cat'ing and echo'ing text around to create the primary
executable, this belongs in prepare() not build() -- also noticed you
*did* file an issue upstream to add this
pd-git:
- unquoted $pkgdir and $srcdir
- source urls for git repositories should be cloned over https:// rather
than git:// in order to take advantage of cacert verification; in
- autogen should really go in prepare(), also autogen should die and be
replaced by autoreconf in nearly every case. But then their compat
script does nothing but run `autoreconf -fi` anyway, so you should
*definitely* be all modern and use it properly.
- why do you override the user's choice of buildflags and strip, does
the package break if you try stripping it or using non-upstream
CFLAGS? If you just want to provide debugging symbols by default,
don't as you should not be taking that choice away from the user
prosody-mod-admin-message-hg:
prosody-mod-admin-web-hg:
- must install a copy of the MIT license.
- get_deps.sh seems kind of ugly, I want to say you should do that
yourself in source=() and it doesn't seem like it has changed since
2014 anyway...
python-iwlib:
- source tarball violates shared SRCDEST, $pkgver.tar.gz clashes with
other packages that have the same version.
- `python setup.p install --skip-build` in package() should be preceded
by `python setup.py build` in build() the same way `make` is separated
from `make install` in Makefile powered packages.
- `python setup.py test` is obviously a check(), not a build()
- this is marked as an "any" package when it contains compiled C
extensions which is a huge no-no
python-osc:
- source tarball violates shared SRCDEST, $pkgver.tar.gz clashes with
other packages that have the same version.
- `python setup.p install --skip-build` in package() should be preceded
by `python setup.py build` in build() the same way `make` is separated
from `make install` in Makefile powered packages.
- `python setup.py test` is obviously a check(), not a build()
roundcube-rcmcarddav:
- unquoted $pkgdir
rts-git:
- pkgdesc is really really ugly
- empty variables and arrays are messy
- unquoted $pkgdir, though $srcdir is consistently quoted
- source urls do not need to be prepended with "${_basename}::" when
they don't actually need to be renamed
ssr:
- provides, conflicts, and replaces itself.
- pkgdesc is self-referential, which it should not be
- versioned depends are bloat
- generally boost-libs is a depends and boost is a makedepends but here
only boost-libs is present and as a makedepends, not sure that that is
correct
- why are you tee'ing everything to logfiles? that's why makepkg has a
dedicated log option
ssr-git:
- source urls do not need to be prepended with "${_pkgname}::" when
they don't actually need to be renamed
- pkgdesc is self-referential, which it should not be
- dependencies differ from non -git package, one or the other is
probably wrong and should be fixed
- autogen should really go in prepare(), also autogen should die and be
replaced by autoreconf in nearly every case. But then their compat
script does nothing but run `autoreconf -fi` anyway, so you should
*definitely* be all modern and use it properly.
- patching should also go in prepare(), and come before autogen on
principle
- why are you exporting LIBS, which is not set by makepkg and indicates
something wrong with the build system (which should probably be
patched properly)
- is the patch for gcc7 necessary for the non-git version?
supercollider-git:
- source urls do not need to be prepended with "${_name}::" when
they don't actually need to be renamed
- why are there commented-out functions with legacy patches still
in-tree? why is the pkgvere() commented out and replaced by one that
`git describe`s HEAD?
- Why does build() update submodules that are not cloned in source=()?
See
https://wiki.archlinux.org/index.php/VCS_package_guidelines#Git_Submodules
- why are you tee'ing everything to logfiles? that's why makepkg has a
dedicated log option
- why some big complicated switch for configuring based on the CARCH,
why don't you just use the $CARCH variable you already have available,
and if you actually have to do different things depending on the
CARCH, then you can make this a lot easier to read by including extra
options in a bash array for safe tokenization.
- why are you overriding the user's MAKEFLAGS from makepkg.conf by
specifying the number of concurrent jobs to run? Is the upstream build
system bugged and you must use that exact number of jobs to avoid
erros due to race conditions and a badly-defined targets graph?
uenv:
- empty variables and arrays are messy
- provides itself
- unquoted $pkgdir
- there is no need to `install -d ... && install -Dm644 ...`, just do
the second part
- this appears to be your personal configurations, does that really
belong in the AUR?
uenv-git:
- mostly the same as uenv
- source urls do not need to be prepended with "${_basename}::" when
they don't actually need to be renamed
veejay-client-git:
veejay-server-git:
veejay-utils-git:
- pkgdesc is self-referential
- in no sane world does a -git package "replaces" the non-git version
just because it is a drop-in replacement
- autogen should really go in prepare(), also autogen should die and be
replaced by autoreconf in nearly every case. But then their compat
script does nothing but run `autoreconf -fi` anyway, so you should
*definitely* be all modern and use it properly.
- why are you tee'ing everything to logfiles? that's why makepkg has a
dedicated log option
- consider making these a split package, since they all have the same
version and build from the same sources
vrpn-git:
- source urls do not need to be prepended with "${_pkg}::" when
they don't actually need to be renamed
- Why does prepare() update submodules that are not cloned in source=()?
See
https://wiki.archlinux.org/index.php/VCS_package_guidelines#Git_Submodules
- there is no need to `msg` the user using makepkg's private API every
time you run a sed command
- CMake builds may need to be out-of-tree, but there is no rule that
says you have to rm -rf an incremental build, unless this package is
special somehow?
--
Eli Schwartz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20171022/ce89d339/attachment.asc>
More information about the aur-general
mailing list