Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Finalising]
On 10/7/18 10:45 PM, Tom Hale wrote: > 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. Looks good to me. :) -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Finalising]
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
_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
Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Take 3]
On Sun, Oct 07, 2018 at 10:46:56AM -0400, Eli Schwartz via aur-general wrote: > 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? I wasn't considering that it'd stay enabled for makepkg as a whole. What I was trying to say is that I consider it a good idea in bash scripts in general. > 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? It might be (either of those). Like you say, hard to judge without knowing the makepkg code in detail. Florian -- https://www.qutebrowser.org | m...@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/ signature.asc Description: PGP signature
Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Take 3]
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
Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Take 3]
On 7/10/18 4:58 pm, Florian Bruhin wrote:
On Sun, Oct 07, 2018 at 04:32:04PM +0700, Tom Hale wrote:
Guess you could use $_pkgname here for nixnote2. Also not sure
specifying the version there is a good idea.
https://wiki.archlinux.org/index.php/PKGBUILD#provides says:
Note: The version that the package provides should be mentioned (pkgver
and potentially the pkgrel), in case packages referencing the software
require one. For instance, a modified qt package version 3.3.8, named
qt-foobar, should use provides=('qt=3.3.8'); omitting the version number
would cause the dependencies that require a specific version of qt to
fail. Do not add pkgname to the provides array, as it is done automatically
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?
AFAIK it only sets -e.
Cheers to you and Luca Weiss for the reviews.
My first package will be going live soon. *happy-dance*.
--
Tom Hale
Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Take 3]
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 | m...@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/
signature.asc
Description: PGP signature
Re: [aur-general] RFC: PKGBUILD for nixnote2-git [Take 3]
Here's a version which builds from HEAD of the default branch.
Comments please.
--
Tom Hale
# Maintainer: Tom Hale
# Contributor: twa022
_pkgname=nixnote2
_repo_url="https://github.com/robert7/${_pkgname}.git;
pkgname=${_pkgname}-git
pkgver=v2.1.0.beta3.r95.g8f235769
pkgrel=1
pkgdesc="Evernote clone (formerly Nevernote) - git checkout"
url="https://github.com/robert7/nixnote2;
arch=(x86_64)
license=('GPL3')
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)
sha256sums=('SKIP')
set -o pipefail
pkgver() {
cd "$srcdir/$_pkgname"
git describe --long | sed 's/\([^-]*-g\)/r\1/;s/-/./g'
}
build() {
echo "Building package: $_pkgname-$pkgver" >&2
cd "$srcdir/$_pkgname"
/usr/bin/qmake PREFIX=/usr
make
# Strip the binary to save 160MB of disk
strip qmake-build-release/"$_pkgname"
}
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/"
}
Re: [aur-general] RFC: PKGBUILD for nixnote2-git
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.
Re: [aur-general] RFC: PKGBUILD for nixnote2-git
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.
# Maintainer: Tom Hale
# Contributor: twa022
# Build the highest versioned tag of nixnote2.
# For versioning, see https://github.com/robert7/nixnote2/issues/28
_pkgname=nixnote2
_repo_url="https://github.com/robert7/${_pkgname}.git;
pkgname=$_pkgname
pkgver=2.1.0.beta4g
pkgrel=1
pkgdesc="Evernote clone (formerly Nevernote) - latest tagged version"
url="https://github.com/robert7/nixnote2;
arch=(x86_64)
license=('GPL3')
depends=(tidy java-runtime hicolor-icon-theme poppler-qt5 qt5-webkit)
makedepends=(git)
provides=("nixnote=${pkgver%.r*}" "nixnote2=${pkgver%.r*}")
replaces=(nevernote nixnote nixnote-beta)
source=("${_pkgname}"::git+$_repo_url)
sha256sums=('SKIP')
set -o pipefail
# Get the tag of the commit to use
# Separated out to allow for `makepkg -e` not running prepare()
_get_tag() {
_tag=$(git -c 'versionsort.suffix=-' ls-remote -t --exit-code --refs
--sort=-v:refname \
"$_repo_url" 'v*' \
| sed -E 's/^[[:xdigit:]]+[[:space:]]+refs\/tags\/(.+)/\1/g' | head -n1)
echo "Selected git tag: $_tag" >&2 # To STDERR as called from pkgver()
}
prepare() {
_get_tag
cd "$srcdir/$_pkgname"
git reset --hard "$_tag"
}
pkgver() {
cd "$srcdir/$_pkgname"
[[ -z ${_tag-} ]] && _get_tag
# Example: v2.1.0-beta3 -> 2.1.0.beta3
echo "$_tag" | sed -E 's/^v//;s/-/./'
}
build() {
echo "Building package: $_pkgname-$pkgver" >&2
cd "$srcdir/$_pkgname"
/usr/bin/qmake PREFIX=/usr
make
# Strip the binary to save 160MB of disk
strip qmake-build-release/"$_pkgname"
}
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/"
}
Re: [aur-general] RFC: PKGBUILD for nixnote2-git
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.
