[aur-general] RFC: PKGBUILD for nixnote2-git [Take 3]

[Eli Schwartz]

On 10/7/18 5:32 AM, Tom Hale wrote:
> Here's a version which builds from HEAD of the default branch.
> 
> Comments please.

Well, by convention you would post this inline instead of as an
attachment since it is easier to review that way...

> _pkgname=nixnote2
> _repo_url="https://github.com/robert7/${_pkgname}.git"

This is just "${url}.git", and you only use it for source.

> pkgname=${_pkgname}-git
> pkgver=v2.1.0.beta3.r95.g8f235769

pkgver leading with a "v" is wrong, but see below

> pkgrel=1
> pkgdesc="Evernote clone (formerly Nevernote) - git checkout"
> url="https://github.com/robert7/nixnote2"
> arch=(x86_64)
> license=('GPL3')

You specifically don't single-quote most things, but then you do
single-quote the license array, which is a bit inconsistent and
stylistically weird.

> depends=(hunspell java-runtime hicolor-icon-theme poppler-qt5 tidy qt5-webkit)
> makedepends=(git)
> provides=("nixnote=${pkgver%.r*}" "nixnote2=${pkgver%.r*}")
> replaces=(nevernote nixnote nixnote-beta)
> source=("${_pkgname}"::git+$_repo_url)

There's no need to explicitly rename the source to something it already
is. The $_repo_url ends in ${_pkgname}.git, so it is already going to be
cloned to ${_pkgname}...

This is also an issue with the previous version of the PKGBUILD, so now
is your chance to improve it!

But you should just use source=("git+${url}.git") since ${_repo_url} and
${url} are the same.

> sha256sums=('SKIP')
> 
> set -o pipefail

This is extremely dangerous and general bad practice for something that
injects itself into the runtime of another program. You're setting
pipefail globally for the entire makepkg script, and there's only two
possibilities what it does: it either modifies the entire makepkg
program rather than just the function you are trying to run, or else
gets reset by makepkg and your functions never see pipefail at all.

Why do you need it anyway? You use exactly one pipe, and if the pkgver()
function breaks and as a result the pkgver is empty, makepkg will abort
with:

==> ERROR: pkgver is not allowed to be empty.
==> ERROR: pkgver() generated an invalid version:

On 10/7/18 5:58 AM, Florian Bruhin wrote:
> That seems like a good idea, but I'm not sure a PKGBUILD should set it.
> Maybe makepkg even does it by itself?
Who says it's a good idea? When is it a good idea? How do you know that
it *should* be enabled in this case? Maybe there's things that depend on
it being unset? How do you know without doing a thorough review of the
makepkg codebase?

Are you suggesting it would be a good idea for makepkg to do it itself,
globally? Or just as part of the environment it sets up for
user-supplied functions?

I will leave this review and analysis of whether it is reset in makepkg
itself, as an exercise to the reader. But I will note, that makepkg
explicitly handles any sort of set or shopt during a
prepare/pkgver/build/package function, by saving the shell state before
running the user-supplied function then restoring it afterwards.

The correct way to handle this is to set it when and where you need it,
and if people think that makepkg needs this change too, they should
submit a bugreport with their analysis and rationale.

On 10/7/18 6:23 AM, Tom Hale wrote:
> AFAIK it only sets -e.

It doesn't set either one.

It does set -e locally for each user-supplied
prepare/pkgver/build/package function, but then it resets that to
disable errexit once it leaves the user-supplied function and returns to
the main makepkg code.

> pkgver() {
>   cd "$srcdir/$_pkgname"
>   git describe --long | sed 's/\([^-]*-g\)/r\1/;s/-/./g'
> }

pkgver() function should strip the "v"

The existing PKGBUILD used:

git describe --long --tags | sed -r "s/^v//;s/([^-]*-g)/r\1/;s/-/./g"

Not sure why you would have changed that.

But it looks like you've fixed this in the update you actually uploaded
to the AUR.

> build() {
>   echo "Building package: $_pkgname-$pkgver" >&2
>   cd "$srcdir/$_pkgname"
>   /usr/bin/qmake PREFIX=/usr

As previously mentioned, both this echo and this hardcoded path are
wrong, but thanks for fixing it in the version you uploaded to the AUR.

>   make
>   # Strip the binary to save 160MB of disk
>   strip qmake-build-release/"$_pkgname"
> }

On 10/7/18 5:58 AM, Florian Bruhin wrote:
> makepkg should already do that.

For a given value of "already". It's actually considerably worse.
Something makepkg already does, like:

echo "Building package: $_pkgname-$pkgver" >&2

would simply be redundant and pointless. Stripping the binaries
yourself, is actively bad and harmful to the user.

makepkg does't just strip the binary itself. It offers a configurable
user choice whether to strip binaries or not, and doing it manually in
build or package will rob the user of choice.

Furthermore, makepkg has a debug option, which even adds additional
debugging CFLAGS to the build while also producing a detached package
nixnote2-git-debug containing the resulting debug symbols. This gives
users a great deal of flexibility in installing optional debug symbols,
or creating and hosting repositories that by default are no-frills
light(er)weight packages without large debug symbols, but allow you to
separately install the debug symbols if you decide you need them.

> package() {
>   cd "$_pkgname"
>   make INSTALL_ROOT="${pkgdir}" install
>   install -Dm644 shortcuts.txt "${pkgdir}/usr/share/doc/$_pkgdir/shortcuts_sample.txt"
>   install -Dm644 docs/{shortcuts-howto,CHANGELOG}.md "${pkgdir}/usr/share/doc/$_pkgdir/"
This is is pre-existing code from the person who maintained the package
before you, but it's just asking for trouble down the road. You
implicitly create the doc directory in the first install command, but if
you ever remove it then the second install command will unexpectedly
die. That also means the -D in the second command is pointless -- it's
already known to exist, and if it doesn't exist then the command fails
because it tries to copy multiple files at once -- at least if it didn't
use -D, you'd know the command needs modifying...

I'd suggest instead using:

install Dm644 -t "${pkgdir}/usr/share/doc/${_pkgname}/" shortcuts.txt \
    docs/{shortcuts-howto,CHANGELOG}.md

(and of course as mentioned previously, _pkgdir => _pkgname)

...

I'm not sure why you asked for review here, but when someone else
responded with points about the stripping and the pipefail, you didn't
incorporate this feedback and also didn't respond to explain your
reasons for keeping it. Presumably when asking for a review, you're
acknowledging that the members of this list have a lot of experience in
packaging and might know things you don't, so if they suggest something
you don't understand, it would be wise to engage in further dialogue to
discover if there's something you missed.

Anyway I've added my additional thoughts on why I believe both changes
are something to fix by taking out again.

-- 
Eli Schwartz
Bug Wrangler and Trusted User

-------------- 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/20181007/37d1865d/attachment.asc>