PKGBUILD review request

YoshiRulz · 2018-12-04 04:55:05

Long-time package installer, first-time package creator! I'm currently porting the BizHawk emulator to Linux, and that means a runtime dependency on this small library. This is what I came up with:

# Maintainer: James Groom <OSSYoshiRulz at gmail dot com>
# Contributor: Shay Green <gblargg at gmail dot com>
pkgname=libblip_buf
pkgver=1.1.1
pkgrel=1
pkgdesc="A small waveform synthesis library meant for use in emulating sound chips of classic consoles."
arch=('x86_64')
url="https://gitlab.com/TASVideos/libblip_buf"
license=('LGPL2.1')
depends=('glibc')
source=('https://gitlab.com/TASVideos/libblip_buf/uploads/6cb86d0a9e8aa43c90bc953dc96d51cd/libblip_buf.tar.xz')
sha512sums=('1fc46d7365edf369071e6680175e1637f7126bdce9854c9c52f858509b0a3d1eadad1688f14158ad64784bcb75c8f6f14a5db17de37b3005c2394c6de7d2a531')

build() {
	cd "$srcdir/$pkgname"
	make so
}

package() {
	temp="$pkgdir/usr/lib"
	mkdir -p "$temp" && cd "$temp"
	temp1="$pkgname.so.$pkgver"
	mv "$srcdir/$pkgname/$temp1" .
	temp2="$pkgname.so.$(printf "%s" "$pkgver" | sed "s/\(\\.[0-9]\)\+//")"
	ln -s "$temp1" "$temp2"
	ln -s "$temp2" "$pkgname.so"
}

The two questions I have are:

Is there a more idiomatic way to add the links in /usr/lib? And
Are the contribution comments for contributors to the PKGBUILD or the package contents?

Trilby · 2018-12-04 05:15:29

What are all those pointless temp variables for?

package() {
	cd "${srcdir}/${pkgname}"
	install -Dm755 libblip_buf.so.${pkgver} ${pkgdir}/usr/lib/libblip_buf.so.${pkgver}
	#ln ...
}

Contributors in the comments are for the PKGBUILD (e.g., previous maintainers)

Last edited by Trilby (2018-12-04 05:23:19)

eschwartz · 2018-12-04 05:42:41

1) This should be performed by the Makefile, really, but you can skip the printf piped to sed, by simply using "${pkgver%%.*}" to remove all components starting with the first dot. (And as Trilby observed, you don't really need all those variables, but using the variables does add confusion and make the PKGBUILD harder to read.)

2) Contributors to the PKGBUILD.

3) You didn't ask this, but I'll say it anyway: Don't use the unfriendly checksummed download link which requires nontrivial updating in future rather than deriving it from the pkgver, use this instead: https://gitlab.com/TASVideos/libblip_bu … 1.1.tar.gz (and interpolate the pkgver as appropriate).

See https://gitlab.com/gitlab-org/gitlab-ce/issues/38830 and https://gitlab.com/gitlab-org/gitlab-ce/issues/41766 for context around gitlab making the archive url more friendly, and having an open issue to make the checksummed download link more friendly too.

4) Looking at the upstream Makefile, it does not respect either CFLAGS or LDFLAGS. Please ask the upstream developer to use CFLAGS ?= ... to only use those optimizing CFLAGS when the user did not specify their own CFLAGS, to stop using LFLAGS in order to hardcode -shared as this abstraction is pointless (just pass -shared explicitly), and to include LDFLAGS.

Actually by depending on blip_buf.o instead of the .c file, the builtin pattern rules for Make would be used to build the .o file from the .c file, which would automatically handle support for CFLAGS. However, there is no builtin pattern rule to build a shared library, mostly because there isn't a decent way to pattern this I guess.

Even so, building the .o file and then the shared library is somewhat traditional. I guess it matters more for incremental builds when there is more than one source file...

Last edited by eschwartz (2018-12-04 05:51:05)

Trilby · 2018-12-04 13:44:11

eschwartz wrote:

However, there is no builtin pattern rule to build a shared library, mostly because there isn't a decent way to pattern this I guess.

Sure there is. And a very trivial makefile would be more than sufficient for this library:

$ ls
Makefile  mylib.c

$ cat Makefile

%.so: %.o
        $(CC) -shared $(LDFLAGS) -o $@ $<

$ make mylib.so
cc    -c -o mylib.o mylib.c
cc -shared -o mylib.so mylib.o
rm mylib.o

$ ls
Makefile  mylib.c   mylib.so*

Last edited by Trilby (2018-12-04 13:44:54)

eschwartz · 2018-12-04 14:03:09

At least, that would need to be "libmylib.so" -- but I was thinking of "libmylib.so.1.0.0" which is not something Make's builtin rules can predict without also attempting to standardize a $(SONAME) variable too...

Trilby · 2018-12-04 14:13:46

A bit OT, but just discovered that this actually works:

lib%.so: %.o
        $(CC) -shared $(LDFLAGS) -o $@ $<

One might just prefer their source/object file names start with 'lib' instead, but it's nice this option works.

eschwartz · 2018-12-04 18:37:39

Yeah, the % stem can have both prefixes and suffixes -- it is just by far the most common use, to use a stem plus a file extension suffix and no prefix at all.

This does not handle the soname though. Neither via -Wl,-soname nor by naming the output file. The latter you could handle with your own rename rules, and the former you could handle using private LDFLAGS+= scoped to a Makefile target.

But at this point it is sort of not generic anymore.

YoshiRulz · 2018-12-05 08:36:22

Trilby wrote:

What are all those pointless temp variables for?
...
install

eschwartz wrote:

a lot

Thanks so much! I'll pass on the stuff about the Makefile to someone who knows what it means, for now I've made your suggested changes and I feel ready to submit.

# Maintainer: James Groom <OSSYoshiRulz at gmail dot com>
pkgname=libblip_buf
pkgver=1.1.1
pkgrel=1
pkgdesc="A small waveform synthesis library meant for use in emulating sound chips of classic consoles."
arch=('x86_64')
url="https://gitlab.com/TASVideos/libblip_buf"
license=('LGPL2.1')
depends=('glibc')
source=('https://gitlab.com/TASVideos/libblip_buf/-/archive/1.1.1/libblip_buf-1.1.1.tar.gz')
sha512sums=('228199524521a27e29ebede20a0815029b1c60d79786ce98b7596629e3853d70f7e2f880aba142fd5d221670ab3e95db27289412546d7d339b5039a899d7ba76')

build() {
	cd "$pkgname-$pkgver"
	make so
}

package() {
	install -Dm755 "$pkgname-$pkgver/libblip_buf.so.$pkgver" "$pkgdir/usr/lib/libblip_buf.so.$pkgver"
	cd "$pkgdir/usr/lib"
	ln -fs "$pkgname.so.$pkgver" "$pkgname.so"
	ln -fs "$pkgname.so.${pkgver%%.*}" "$pkgname.so"
}

eschwartz · 2018-12-05 12:49:57

eschwartz wrote:

use this instead: https://gitlab.com/TASVideos/libblip_bu … 1.1.tar.gz (and interpolate the pkgver as appropriate).

You did not interpolate the pkgver:

source=('https://gitlab.com/TASVideos/libblip_buf/-/archive/${pkgver}/libblip_buf-${pkgver}.tar.gz')

Doing it like this means there are fewer things to modify on each new version. Same reason you used the ${pkgver} variable later on when deciding which directory to cd to.

YoshiRulz wrote:

	ln -fs "$pkgname.so.$pkgver" "$pkgname.so"
	ln -fs "$pkgname.so.${pkgver%%.*}" "$pkgname.so"

This installs two different symlinks as $pkgname.so and results in the second one overwriting the first one. The first symlink should be:

	ln -fs "$pkgname.so.$pkgver" "$pkgname.so.${pkgver%%.*}"

YoshiRulz · 2018-12-05 17:48:30

Revision 2 already...

eschwartz · 2018-12-05 19:53:50

Looks fine I guess, except that the .SRCINFO is malformed and contains literal ${pkgver} instead of being calculated by makepkg --printsrcinfo and printed as 1.1.1

Arch Linux

#1 2018-12-04 04:55:05

PKGBUILD review request

#2 2018-12-04 05:15:29

Re: PKGBUILD review request

#3 2018-12-04 05:42:41

Re: PKGBUILD review request

#4 2018-12-04 13:44:11

Re: PKGBUILD review request

#5 2018-12-04 14:03:09

Re: PKGBUILD review request

#6 2018-12-04 14:13:46

Re: PKGBUILD review request

#7 2018-12-04 18:37:39

Re: PKGBUILD review request

#8 2018-12-05 08:36:22

Re: PKGBUILD review request

#9 2018-12-05 12:49:57

Re: PKGBUILD review request

#10 2018-12-05 17:48:30

Re: PKGBUILD review request

#11 2018-12-05 19:53:50

Re: PKGBUILD review request

Board footer