PKGBUILD review request: bombadillo-git

vestingz · 2020-12-03 16:13:17

Hi everyone,

I made this following PKGBUILD:

# Maintainer: Emilio Reggi <nag@mailbox.org>
pkgname=bombadillo-git
_pkgname=bombadillo
_source=https://tildegit.org/sloum/bombadillo.git
pkgver=r462.757305d
pkgrel=2
pkgdesc="Bombabillo is a non-web client for the terminal, supporting Gopher, Gemini and much more."
arch=('x86_64')
url="https://tildegit.org/sloum/bombadillo.git"
license=('GPL')
makedepends=('go' 'git')
optdepends=('desktop-file-utils: create desktop entry')
provides=("${_pkgname}")
conflicts=("${_pkgname}")
source=("${_pkgname}"::"git+${_source}")
md5sums=('SKIP')

pkgver() {
	cd "$_pkgname"
	printf "r%s.%s" "$(git rev-list --count HEAD)" "$(git rev-parse --short HEAD)"
}

build() {
	cd "$_pkgname"
	make PREFIX=/usr
}

package() {
	cd "$_pkgname"
	make VERSION=$pkgver DESTDIR="$pkgdir" PREFIX=/usr install
}

The package builds fine, although I am getting a warning:

==> WARNING: Package contains reference to $srcdir
usr/bin/bombadillo

Also, namcap <package> produces some outputs which I am struggling to interpret:

bombadillo-git W: ELF file ('usr/bin/bombadillo') lacks FULL RELRO, check LDFLAGS.
bombadillo-git W: ELF file ('usr/bin/bombadillo') lacks PIE.
bombadillo-git W: Dependency glibc detected but optional (libraries ['usr/lib/libpthread.so.0', 'usr/lib/libc.so.6'] needed in files ['usr/bin/bombadillo'])

Lastly, I would be very happy to receive helpful comments on the PKGBUILD itself, regarding soundness, style or best practices

Thank you!

PS: I actually created more packages, which I would be happy to be reviewed, but atm I mostly care about the one above due to the afromentioned errors. Others are scholarref-git, mastorss-git and mastodonpp-git.

Lone_Wolf · 2020-12-04 16:15:41

optdepends=('desktop-file-utils: create desktop entry')

Are you sure that should be an optdepend ?

It's also weird to see pacakges without any runtime dependencies, are go binaries completely standalone ?

bombadillo-git W: ELF file ('usr/bin/bombadillo') lacks FULL RELRO, check LDFLAGS.
bombadillo-git W: ELF file ('usr/bin/bombadillo') lacks PIE.

software build with go has a bad habit of not honoring locally set build flags.
See https://wiki.archlinux.org/index.php/Go … guidelines for info how to get them to comply.

nitpick
the _source variable is only used in 1 place (source= ) , why use a var ?

Using _pkgname in many places makes the PKGBUILD harder to read for me.
Are you using some kind of template for creating PKGBUILDs ?

vestingz · 2020-12-04 17:41:17

Thank you so much for your input!

Lone_Wolf wrote:

optdepends=('desktop-file-utils: create desktop entry')
Are you sure that should be an optdepend ?

Good question. I think when I originally wrote the PKGBUILD a couple of months back, IIRC it used to only be an option in the Makefile - but I am not so sure anymore what my original motivation really was. Maybe namcap'ing the package gave me a hint? I should probably include it in the regular depends array.

Lone_Wolf wrote:

It's also weird to see pacakges without any runtime dependencies, are go binaries completely standalone ?

Yes, I think that is one specific property of go binaries that they include all runtime libraries in the binary itself, which is why they use to turn out comparably big.

Lone_Wolf wrote:

bombadillo-git W: ELF file ('usr/bin/bombadillo') lacks FULL RELRO, check LDFLAGS.
bombadillo-git W: ELF file ('usr/bin/bombadillo') lacks PIE.
software build with go has a bad habit of not honoring locally set build flags.
See https://wiki.archlinux.org/index.php/Go … guidelines for info how to get them to comply.

Great, I should have done that earlier - thanks for the hint!

Lone_Wolf wrote:

nitpick
the _source variable is only used in 1 place (source= ) , why use a var ?
Using _pkgname in many places makes the PKGBUILD harder to read for me.
Are you using some kind of template for creating PKGBUILDs ?

Yeah, it is sort of the idea to have a little generalized boilerplate for git VCS packages, so I don't have to make too many changes when creating a new PKGBUILD. But I am just playing around with it in the last few days, also to increase my understanding how things are actually happening. Would you suggest to comment these decisions in the PKGBUILD so it becomes more clear?

Lone_Wolf · 2020-12-04 19:58:09

One additonal issue : description shouldn't have the packagename in it [1]
You should also try to clarify what it's used for*.

Maybe something like "terminal client with vim-support for gopher and other http alternatives" ?

vestingz wrote:

Yeah, it is sort of the idea to have a little generalized boilerplate for git VCS packages, so I don't have to make too many changes when creating a new PKGBUILD. But I am just playing around with it in the last few days, also to increase my understanding how things are actually happening. Would you suggest to comment these decisions in the PKGBUILD so it becomes more clear?

I've looked at it a bit closer :
local variables in source= are rather common and seems to be more fo a personal choice.
cd commands in the prepare() , build() etc functions are very normal.

However I can't remember seeing vars used in depends / makedepends / optdepends / checkdepends or provides / conflicts / replaces

It's rare for them to change and they must be tailored to the package.
Seeing vars in there makes me wonder if the maintainer has spent time thinking about what that specific package needs.

I suggest you change provides and conflicts to use the actual package names.

[1] https://wiki.archlinux.org/index.php/PKGBUILD#pkgdesc

* I do remember gopher protocol vaguely amd may have used it in the early 90s along with mosaic , pcboard BBS , zmodem protocol and fidonet .

Gemini only rings the following bells :
- twins
- a space program that has some relation with the apollo space program
- an astrology sign

Last edited by Lone_Wolf (2020-12-04 19:58:48)

vestingz · 2020-12-06 13:14:21

Lone_Wolf wrote:

One additonal issue : description shouldn't have the packagename in it [1]
You should also try to clarify what it's used for*.
Maybe something like "terminal client with vim-support for gopher and other http alternatives" ?

Thank you, I just changed the description

Lone_Wolf wrote:

vestingz wrote:
Yeah, it is sort of the idea to have a little generalized boilerplate for git VCS packages, so I don't have to make too many changes when creating a new PKGBUILD. But I am just playing around with it in the last few days, also to increase my understanding how things are actually happening. Would you suggest to comment these decisions in the PKGBUILD so it becomes more clear?
I've looked at it a bit closer :
local variables in source= are rather common and seems to be more fo a personal choice.
cd commands in the prepare() , build() etc functions are very normal.
However I can't remember seeing vars used in depends / makedepends / optdepends / checkdepends or provides / conflicts / replaces
It's rare for them to change and they must be tailored to the package.
Seeing vars in there makes me wonder if the maintainer has spent time thinking about what that specific package needs.
I suggest you change provides and conflicts to use the actual package names.

Fair point, indeed. However, git VCS packages - to my understanding - will always have the _pkgname variable in provides and conflicts, because the VCS package guidelines require you to add a suffix to the pkgname variable, e.g. -git. Using a _pkgname variable without the suffix will then always be indicated because otherwise the ABS will not detect conflicts and dependencies with non-git releases.

Long story short: I thought it was a fair detail to generalize for my boilerplate

Lone_Wolf wrote:

* I do remember gopher protocol vaguely amd may have used it in the early 90s along with mosaic , pcboard BBS , zmodem protocol and fidonet .
Gemini only rings the following bells :
- twins
- a space program that has some relation with the apollo space program
- an astrology sign

Yeah, Gemini is a relatively recent development which, to my understanding, grew out of the tilde movement. Pretty nerdy, but cool
You can read about it more here.

Lone_Wolf · 2020-12-06 14:53:05

However, git VCS packages - to my understanding - will always have the _pkgname variable in provides and conflicts, because the VCS package guidelines require you to add a suffix to the pkgname variable, e.g. -git.

Correct.

Using a _pkgname variable without the suffix will then always be indicated because otherwise the ABS will not detect conflicts and dependencies with non-git releases.

Incorrect

https://wiki.archlinux.org/index.php/VCS_package_guidelines#Guidelines wrote:

Include what the package conflicts with and provides (e.g. for fluxbox-gitAUR: conflicts=('fluxbox') and provides=('fluxbox')).Include what the package conflicts with and provides (e.g. for fluxbox-gitAUR: conflicts=('fluxbox') and provides=('fluxbox')).

No mention of using vars at all.

I checked a bit further and many maintainers (including some archlinux TUs and devs) do use _pkgname or ${pkgname%-git} .

I do think this is a personal preference and there's no right or wrong way.
Use what you feel comfortable with.

Last edited by Lone_Wolf (2020-12-06 14:54:20)

Scimmia · 2020-12-06 15:06:58

vestingz wrote:

Thank you so much for your input!
Lone_Wolf wrote:
optdepends=('desktop-file-utils: create desktop entry')
Are you sure that should be an optdepend ?
Good question. I think when I originally wrote the PKGBUILD a couple of months back, IIRC it used to only be an option in the Makefile - but I am not so sure anymore what my original motivation really was. Maybe namcap'ing the package gave me a hint? I should probably include it in the regular depends array.

Optional deps are those that are optional *at runtime*. If the makefile uses it, it appears to be a build time dep?

vestingz · 2020-12-06 15:51:01

Lone_Wolf wrote:

However, git VCS packages - to my understanding - will always have the _pkgname variable in provides and conflicts, because the VCS package guidelines require you to add a suffix to the pkgname variable, e.g. -git.
Correct.
Using a _pkgname variable without the suffix will then always be indicated because otherwise the ABS will not detect conflicts and dependencies with non-git releases.
Incorrect
https://wiki.archlinux.org/index.php/VCS_package_guidelines#Guidelines wrote:
Include what the package conflicts with and provides (e.g. for fluxbox-gitAUR: conflicts=('fluxbox') and provides=('fluxbox')).Include what the package conflicts with and provides (e.g. for fluxbox-gitAUR: conflicts=('fluxbox') and provides=('fluxbox')).
No mention of using vars at all.

True - I have not been making myself perfectly clear in my previous post, saying the usage of a _pkgname variable will always be indicated, while I meant the respective value of the _pkgname variable, hence e.g. "fluxbox". So because this value should always be included, I found it practical to introduce a variable.
Thank you for your critique - made my thoughts more clear

Scimmia wrote:

vestingz wrote:
Thank you so much for your input!
Lone_Wolf wrote:
optdepends=('desktop-file-utils: create desktop entry')
Are you sure that should be an optdepend ?
Good question. I think when I originally wrote the PKGBUILD a couple of months back, IIRC it used to only be an option in the Makefile - but I am not so sure anymore what my original motivation really was. Maybe namcap'ing the package gave me a hint? I should probably include it in the regular depends array.
Optional deps are those that are optional *at runtime*. If the makefile uses it, it appears to be a build time dep?

Yes, I didn't really understand that back then for some reason. It is already updated in the AUR. Thank you for your remarks

eschwartz · 2020-12-06 21:07:49

vestingz wrote:

_source=https://tildegit.org/sloum/bombadillo.git
url="https://tildegit.org/sloum/bombadillo.git"
source=("${_pkgname}"::"git+${_source}")

I don't see much point in a variable you only ever use once... on the other hand, you could do this:

url="https://tildegit.org/sloum/bombadillo.git"
source=("git+${url}")

There is no need to use ${_pkgname}:: to rename the download clone directory, since the remote url is bombadillo.git and it gets cloned per default to bombadillo/

Also, please remove the makedepends on desktop-file-utils and patch out the Makefile invocation of update-desktop-database:
https://tildegit.org/sloum/bombadillo/s … le#L33-L38

 	# They would likely work on BSD systems
	install -d ${DESTDIR}${DATAROOTDIR}/applications
	install -m 0644 ./bombadillo.desktop ${DESTDIR}${DATAROOTDIR}/applications
	install -d ${DESTDIR}${DATAROOTDIR}/pixmaps
	install -m 0644 ./bombadillo-icon.png ${DESTDIR}${DATAROOTDIR}/pixmaps
	-update-desktop-database 2> /dev/null

The desktop file is correctly installed using install -m 0644.

Due to an upstream bug, update-desktop-database is run when DESTDIR is set, but update-desktop-database does not index the file in ${DESTDIR}/usr/share/applications, it indexes the files in /usr/share/applications which is a PKGBUILD violation, fails unless you are root, and is performed by a pacman hook, not by individual packages.

So no package should *ever* use update-desktop-database during package() creation.

vestingz · 2020-12-15 11:09:00

Thank you very much for your in-depth remarks!

eschwartz wrote:

vestingz wrote:
_source=https://tildegit.org/sloum/bombadillo.git
url="https://tildegit.org/sloum/bombadillo.git"
source=("${_pkgname}"::"git+${_source}")
I don't see much point in a variable you only ever use once... on the other hand, you could do this:
url="https://tildegit.org/sloum/bombadillo.git"
source=("git+${url}")
There is no need to use ${_pkgname}:: to rename the download clone directory, since the remote url is bombadillo.git and it gets cloned per default to bombadillo/

My idea was to have some sort of generalized boilerplate for git VCS packages which requires the least amount of modification for the individual case, ideally only in the variables. Therefore I chose this combination of variables.

eschwartz wrote:

Also, please remove the makedepends on desktop-file-utils and patch out the Makefile invocation of update-desktop-database:
https://tildegit.org/sloum/bombadillo/s … le#L33-L38
 	# They would likely work on BSD systems
	install -d ${DESTDIR}${DATAROOTDIR}/applications
	install -m 0644 ./bombadillo.desktop ${DESTDIR}${DATAROOTDIR}/applications
	install -d ${DESTDIR}${DATAROOTDIR}/pixmaps
	install -m 0644 ./bombadillo-icon.png ${DESTDIR}${DATAROOTDIR}/pixmaps
	-update-desktop-database 2> /dev/null 
The desktop file is correctly installed using install -m 0644.
Due to an upstream bug, update-desktop-database is run when DESTDIR is set, but update-desktop-database does not index the file in ${DESTDIR}/usr/share/applications, it indexes the files in /usr/share/applications which is a PKGBUILD violation, fails unless you are root, and is performed by a pacman hook, not by individual packages.
So no package should *ever* use update-desktop-database during package() creation.

Thank you for these helpful remarks - I wasn't aware of these things. Will make your suggested changes to the package as soon as I find the necessary time.

eschwartz · 2020-12-17 02:17:50

vestingz wrote:

Thank you very much for your in-depth remarks!
eschwartz wrote:
vestingz wrote:
_source=https://tildegit.org/sloum/bombadillo.git
url="https://tildegit.org/sloum/bombadillo.git"
source=("${_pkgname}"::"git+${_source}")
I don't see much point in a variable you only ever use once... on the other hand, you could do this:
url="https://tildegit.org/sloum/bombadillo.git"
source=("git+${url}")
There is no need to use ${_pkgname}:: to rename the download clone directory, since the remote url is bombadillo.git and it gets cloned per default to bombadillo/
My idea was to have some sort of generalized boilerplate for git VCS packages which requires the least amount of modification for the individual case, ideally only in the variables. Therefore I chose this combination of variables.

My boilerplate is git+${url}
Renaming the download clone directory is rarely needed, as it's usually already $_pkgname (or ${pkgname%-git}), I instead usually chain together this:

pkgname=someproject
url="https://github.com/username/${pkgname%-git}"
source=("git+${url}")

Most git hosting sites provide identical project pages and clone urls.

vestingz · 2020-12-17 13:03:10

eschwartz wrote:

My boilerplate is git+${url}
Renaming the download clone directory is rarely needed, as it's usually already $_pkgname (or ${pkgname%-git}), I instead usually chain together this:
pkgname=someproject
url="https://github.com/username/${pkgname%-git}"
source=("git+${url}")

Yeah, it makes sense, it is less cluttery Thanks for the snippet!

eschwartz wrote:

Most git hosting sites provide identical project pages and clone urls.

I remember at least one example where this wasn't the case exactly: https://aur.archlinux.org/cgit/aur.git/ … larref-git
Although one could probably argue that I could have just used the clone url as the project url - but it made think about edge cases
Anyways: with the lockdown now I should have plenty of time to redo the PKGBUILDs according to all the valuable input from this thread. Thanks people!

Arch Linux

#1 2020-12-03 16:13:17

PKGBUILD review request: bombadillo-git

#2 2020-12-04 16:15:41

Re: PKGBUILD review request: bombadillo-git

#3 2020-12-04 17:41:17

Re: PKGBUILD review request: bombadillo-git

#4 2020-12-04 19:58:09

Re: PKGBUILD review request: bombadillo-git

#5 2020-12-06 13:14:21

Re: PKGBUILD review request: bombadillo-git

#6 2020-12-06 14:53:05

Re: PKGBUILD review request: bombadillo-git

#7 2020-12-06 15:06:58

Re: PKGBUILD review request: bombadillo-git

#8 2020-12-06 15:51:01

Re: PKGBUILD review request: bombadillo-git

#9 2020-12-06 21:07:49

Re: PKGBUILD review request: bombadillo-git

#10 2020-12-15 11:09:00

Re: PKGBUILD review request: bombadillo-git

#11 2020-12-17 02:17:50

Re: PKGBUILD review request: bombadillo-git

#12 2020-12-17 13:03:10

Re: PKGBUILD review request: bombadillo-git

Board footer