On Sat, 6 Oct 2018 22:41:43 +0700 Tom Hale wrote:

I took over the disowned package nixnote2-git.

Attached is my first attempt at a PKGBUILD. I would appreciate constructive feedback.

Thanks in advance,

-- Tom Hale

-git PKGBUILDS need to build from the latest commit on master or the default branch, not from a tag. I didn't review the whole thing because the entire concept is invalid.

On Sat, 6 Oct 2018 23:09:57 +0700 Tom Hale wrote:

I want avoid the latest commit for greater stability.

Updated attachment: I've dropped the "-git" from the pkgname as I build from the latest annotated tag.

On 6/10/18 10:48 pm, Doug Newgard via aur-general wrote:

...

-git PKGBUILDS need to build from the latest commit on master or the default branch, not from a tag. I didn't review the whole thing because the entire concept is invalid.

Which is also invalid, as you're now passing them off as stable releases when they are not. You'd also have to manually update the AUR every time there's a release anyway, so it's pointless.

On Sun, Oct 07, 2018 at 04:32:04PM +0700, Tom Hale wrote:

Comments please.

Some random stuff I noticed:

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

Probably not needed when you only need it in source.

provides=("nixnote=${pkgver%.r*}" "nixnote2=${pkgver%.r*}")

Guess you could use $_pkgname here for nixnote2. Also not sure specifying the version there is a good idea.

set -o pipefail

That seems like a good idea, but I'm not sure a PKGBUILD should set it. Maybe makepkg even does it by itself?

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

makepkg already should give you output like that.

/usr/bin/qmake PREFIX=/usr

Why /usr/bin/qmake and not just qmake?

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

makepkg should already do that.

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

What's _pkgdir? That sounds like you mean _pkgname. Florian -- https://www.qutebrowser.org | me@the-compiler.org (Mail/XMPP) GPG: 916E B0C8 FD55 A072 | https://the-compiler.org/pubkey.asc I love long mails! | https://email.is-not-s.ms/

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

Thanks all for the great reviews. Thank you in particular to those who said WHY changes were suggested. I now feel empowered to go on to package rambox-os. I believe I have incorporated all the feedback I received in the below. If I missed something it is by mistake - and I ask for your generosity in pointing it out once again. ========================================================= # Maintainer: Tom Hale # Contributor: twa022 <twa022 at gmail dot com> _pkgname=nixnote2 pkgname=${_pkgname}-git pkgver=2.1.0.beta3.r95.g8f235769 pkgrel=2 pkgdesc='Evernote clone (formerly Nevernote) - git checkout' url="https://github.com/robert7/$_pkgname" arch=(x86_64) license=(GPL3) depends=(java-runtime hicolor-icon-theme poppler-qt5 tidy qt5-webkit) makedepends=(git) provides=("nixnote=${pkgver%.r*}" "$_pkgname=${pkgver%.r*}") replaces=(nevernote nixnote nixnote-beta) source=("git+${url}.git") sha256sums=('SKIP') pkgver() { cd "$srcdir/$_pkgname" git describe --long | sed -E 's/^v//;s/([^-]*-g)/r\1/;s/-/./g' } build() { cd "$srcdir/$_pkgname" qmake PREFIX=/usr make } package() { cd "$_pkgname" make INSTALL_ROOT="$pkgdir" install install -Dm644 -t "${pkgdir}/usr/share/doc/${_pkgname}/" docs/{shortcuts-howto,CHANGELOG}.md # Rename shortcuts.txt to align with shortcuts-howto.md: install -m644 shortcuts.txt "${pkgdir}/usr/share/doc/${_pkgname}/shortcuts_sample.txt" } -- Regards, Tom Hale