Hey again, multiple occurences are not quoted again. On 2017-10-22 23:44:39 (-0400), Eli Schwartz wrote:
crypted-backups-git: - empty variables and arrays are messy Fixed.
- unquoted $pkgdir, though $srcdir is consistently quoted Fixed.
- the pkgver() fallback for non-tagged repos can be simplified Fixed.
- build() does nothing, why does it exist? Reasons unknown. Deleted.
- source urls for git repositories should be cloned over https:// rather than git:// in order to take advantage of cacert verification Thanks! Completely forgot to enable that on my server and used git:// ever since...
- source urls do not need to be prepended with "${_pkg}::" when they don't actually need to be renamed Fixed.
- there is no need to `install -d ... && install -Dm644 ...`, just do the second part Ah... the source's expansion prevented a successful -Dm***, so I used -d to create the destination first. I should've used the -t switch. Fixed.
easytranscript: - inconsistent quoting of array elements; why does license have double quotes? Reasons unknown. Fixed.
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 Fixed.
- depends does not need to have a versioned Firefox requirement Fixed.
- why is "${source##*/}" obfuscated when it will always be the same thing? I'm not sure I get, what you mean by that. "${source##*/}" retrieves the .xpi file name from the link stored in ${source}. I could of course also write "${_plugin_name}-${pkgver}-fx-linux.xpi", but does that make it better?
grub-customizer: - 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... Fixed.
- the use of `|| return 1` is deprecated, makepkg sets errexit Fixed.
- inconsistent quoting of array elements; why is provides=() alone in not quoting this? why does license have double-quotes? Fixed.
- why is there a backslash escape for a multiline source=() array? Fixed.
- 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 Fixed.
- the install file is outdated, we have pacman hooks for that now Fixed.
- consider specifying a reason for the optdepends Will have to ask upstream about that one.
jackcpp-git: - unquoted $pkgdir and $srcdir Fixed.
- source urls for git repositories should be cloned over https:// rather than git:// in order to take advantage of cacert verification Fixed.
- source urls do not need to be prepended with "$_basename::" when they don't actually need to be renamed Fixed.
- the url should use HTTPS Fixed.
- this is marked as an "any" package when it contains compiled ELF binaries which is a huge no-no Fixed.
- by the way, the nonstandard ordering of makepkg variables had me looking for a second to make sure you actually listed everything Security through obscurity ;-) Fixed.
khard-git: - build() does nothing, it should run python setup.py build Fixed.
- setuptools installations usually also use --skip-build --optimize=1 in the package() function Fixed.
- why do you explicitly use !emptydirs instead of the makepkg.conf default, does the package depend on it? Legacy problem. Removed.
liblo-git: - 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 Now it does.
- provides a hardcoded version of liblo, $pkgver should be fine but if not, consider using ${pkgver%%.r*} Okay.
- 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 Switched to autoreconf, seems to work fine with minor fix. Thanks for that hint!
mantisbt: - this incredibly gross install script messes around with unpackaged files, please don't do this Okay. What would be a good alternative though? Using the application during upgrade might potentially break the database. However, the mantis_offline.php file prevents users from interacting with the application. I know that hooks such as the one introduced for nextcloud quite recently (for a while) won't work either.
nextcloud-news-updater: owncloud-news-updater: - what is a "parllel" feed updater anyway? A typo. Nice one.
- source tarball violates shared SRCDEST, $pkgver.tar.gz clashes with other packages that have the same version. Fixed.
- `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. Fixed.
- 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. True. Now only the 'Note:' is left.
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 Fixed.
- 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 Fixed. Noted, but it hasn't changed (yet).
pd-git: - 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. Fixed.
- 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 It was mainly because of the following bug: https://bugs.archlinux.org/task/45006 This seems to not apply to pd>0.48 anymore. Yay! Removed.
Also: Debugging seems to be activated in the main repository as well. oO
prosody-mod-admin-message-hg: prosody-mod-admin-web-hg: - must install a copy of the MIT license. Fixed.
- 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... It's unclear, if the package still works as expected with the current version of prosody. I might drop or have it deleted soon.
python-iwlib: - this is marked as an "any" package when it contains compiled C extensions which is a huge no-no Fixed.
python-osc: Fixed.
roundcube-rcmcarddav: Fixed
rts-git: - pkgdesc is really really ugly I made it more beautiful. ;-)
ssr: - provides, conflicts, and replaces itself. Fixed.
- versioned depends are bloat Fixed.
- 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 Fixed.
- why are you tee'ing everything to logfiles? that's why makepkg has a dedicated log option Bad habit. Fixed.
ssr-git: - 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 No. boost-libs has been dropped in favor of asio.
- patching should also go in prepare(), and come before autogen on principle Sadly I have to patch configure. Upstream is veeeeeeeeeeeery slow on merging pull requests, so the switch to qt5, that would make this all go away, is still not upon us :( Moved.
- 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) There is something wrong with the build system. If not exporting LIBS, make will complain about missing -lpthread (undefined reference to symbol). Semi-recent changes in Arch's build system has caused this to fail. Still about to investigate further, when I have time to do so.
- is the patch for gcc7 necessary for the non-git version? No. The patch is necessary, because the IP interface of the application was ported from boost to asio. A pull request was made and approved, but it's still not merged.
supercollider-git: - why are there commented-out functions with legacy patches still Fixed.
in-tree? why is the pkgvere() commented out and replaced by one that `git describe`s HEAD? There is no backporting of patches per version. The SuperCollider devs additionally tend to use lightweight and normal tags chaoticly. https://github.com/supercollider/supercollider/issues/2857
- Why does build() update submodules that are not cloned in source=()? See https://wiki.archlinux.org/index.php/VCS_package_guidelines#Git_Submodules Fixed.
- 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. Sounds interesting. Will rework this, as soon as I have my Pis up and running again. Note however, that some hacks are necessary for supercollider to function on a headless system (which embedded systems often are).
- 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? Fixed.
uenv: - provides itself Fixed. - this appears to be your personal configurations, does that really belong in the AUR? On the contrary, according to its pkgdesc ("Useful scripts, systemd timer/service units and their configuration"), this is not personal configurations, but pretty generic unit files, etc.
uenv-git: Fixed.
veejay-client-git: veejay-server-git: veejay-utils-git: - in no sane world does a -git package "replaces" the non-git version just because it is a drop-in replacement 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. - consider making these a split package, since they all have the same version and build from the same sources Yes, that's my idea eventually. I'm working on a split package, that also includes the plugins of that project.
vrpn-git: I adopted this quite recently to see if I can make it work properly. It is not very beautiful...
- there is no need to `msg` the user using makepkg's private API every time you run a sed command Fixed.
- 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? Fixed.
Thanks again for the valuable input. Best, David -- https://sleepmap.de