[aur-general] TU Application: David Runge

[David Runge]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20171025/02d808b0/attachment-0001.asc>