PKGBUILD review request: vgmtrans-git

TheSheepGuy · 2020-04-13 20:13:54

The compile instructions can be found on the VGMTrans GitHub wiki.

There are a couple of things I'm unsure about:

The developers use the "interim" prefix for their version numbers, should this be kept?
Should I keep the sha256sums if it's a git package I'm using? Some -git AUR packages do, others don't.
Probably the most important one: the developers don't seem to have anticipated anyone to use "make install", because the makefile (which is generated after running "cmake .." as specified in the GitHub repo wiki) doesn't seem to give appropriate measures in the "install:" bit; I don't have experience using makefiles, so I don't really know what to do about that. The solution I have at the minute is that I just copy the executable to /usr/share, which kind of works?
Should I keep the comment on line 26? Usually from what I've read AUR packages tend to just use "make" and expect you to manually add "-j", but as I've said in the comment the devs recommend to use the option.
Line 22 "rm -rf build" makes sure there's no pre-existing "build" folder. I think that's unnecessary? It would be a safe precaution but I'm not sure whether or not to include it.

# Maintainer: Patrick Ausel <TheSheepGuy1@gmail.com>

pkgname=vgmtrans-git
pkgver=interim.20170225.r342.gad1d0f2
pkgrel=1
pkgdesc="Convert console video games music files to standard MIDI and DSL/SF2 files."
arch=("any")
url="https://github.com/vgmtrans/vgmtrans"
license=('ZLIB')
depends=("qt5-base" "fluidsynth")
makedepends=("qt5-tools" "cmake")
source=("$pkgname::git+git://github.com/vgmtrans/vgmtrans.git#branch=refactor")
sha256sums=("SKIP")

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

build() {
	cd "$srcdir/$pkgname"
	rm -rf build
	mkdir build
	cd build
	cmake ..
	# The developers recommend to use "-j$(nproc)".
	make -j$(nproc)
}

package() {
	cd "$srcdir/$pkgname"
	install -Dm755 "build/src/vgmtrans-qt/vgmtrans" "$pkgdir/usr/share/$pkgname"
	install -Dm644 "LICENSE.txt" "$pkgdir/usr/share/licenses/$pkgname/LICENSE"
}

Last edited by TheSheepGuy (2020-04-13 20:20:48)

Slithery · 2020-04-13 20:22:25

For a start you should get rid of the -j$(nproc).

If users want to use multiple cores to build then they should set their own makepkg.conf appropriately.

eschwartz · 2020-04-13 22:32:21

TheSheepGuy wrote:

The compile instructions can be found on the VGMTrans GitHub wiki.
There are a couple of things I'm unsure about:
[*]The developers use the "interim" prefix for their version numbers, should this be kept?[/*]

Does this prefix mean anything? It's not special in the semver sense, it is in fact a waste of space.

TheSheepGuy wrote:

[*]Should I keep the sha256sums if it's a git package I'm using? Some -git AUR packages do, others don't.[/*]

You cannot sha256sum a git repository directory, so that particular element of the sha256sums will be marked as SKIP. This is perfectly normal -- in fact, it's the only way to do it. Same goes for alternative checksum algorithms, no matter which one you use it will be skipped anyway.

(If you have additional sources which are files, not git repos, then you should checksum them as usual.)

TheSheepGuy wrote:

[*]Probably the most important one: the developers don't seem to have anticipated anyone to use "make install", because the makefile (which is generated after running "cmake .." as specified in the GitHub repo wiki) doesn't seem to give appropriate measures in the "install:" bit; I don't have experience using makefiles, so I don't really know what to do about that. The solution I have at the minute is that I just copy the executable to /usr/share, which kind of works?[/*]

This is perfectly normal for upstream software which doesn't specify a make install target, except...

Wait. Why on earth are you using /usr/share instead of /usr/bin, this makes no sense and users won't be able to use it???

TheSheepGuy wrote:

[*]Should I keep the comment on line 26? Usually from what I've read AUR packages tend to just use "make" and expect you to manually add "-j", but as I've said in the comment the devs recommend to use the option.[/*]

Definitely not, if users wish to follow the devs' recommendation they will do as Slithery said and update the MAKEFLAGS in their makepg.conf -- make(1) automatically acts as though the contents of $MAKEFLAGS were specified as command-line options to make.

TheSheepGuy wrote:

[*]Line 22 "rm -rf build" makes sure there's no pre-existing "build" folder. I think that's unnecessary? It would be a safe precaution but I'm not sure whether or not to include it.[/*]

This breaks incremental builds, I'd get rid of it. cmake should already figure out when it needs to rebuild any existing, but stale, source files.

Lone_Wolf · 2020-04-14 12:15:35

eschwartz wrote:

TheSheepGuy wrote:
[*]Line 22 "rm -rf build" makes sure there's no pre-existing "build" folder. I think that's unnecessary? It would be a safe precaution but I'm not sure whether or not to include it.[/*]
This breaks incremental builds, I'd get rid of it. cmake should already figure out when it needs to rebuild any existing, but stale, source files.

Some build systems fail with an incremental build but succeed on a clean build.*
I remove _build folder in PKGBUILD, but switched to doing that in prepare so incremental builds are still possible.

* Have encountered that with autotools, cmake and meson .

Trilby · 2020-04-14 12:56:23

Lone_Wolf wrote:

Some build systems fail with an incremental build but succeed on a clean build.

Every such occurrence should be reported as a bug to either the build system if it is failing when it shouldn't, or to the build target maintainers if they are using the build system incorrectly.

TheSheepGuy · 2020-04-16 11:07:26

Thank you for your help, here's the updated PKGBUILD with the changes mentioned above. I'll wait a couple of days in case there are any other concerns and then I'll submit it to the AUR.

# Maintainer: Patrick Ausel <TheSheepGuy1@gmail.com>

pkgname=vgmtrans-git
pkgver=20170225.r342.gad1d0f2
pkgrel=1
pkgdesc="Convert console video games music files to standard MIDI and DSL/SF2 files."
arch=("any")
url="https://github.com/vgmtrans/vgmtrans"
license=('ZLIB')
depends=("qt5-base" "fluidsynth")
makedepends=("qt5-tools" "cmake")
source=("$pkgname::git+git://github.com/vgmtrans/vgmtrans.git#branch=refactor")
sha256sums=("SKIP")

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

build() {
	cd "$srcdir/$pkgname"
	mkdir build
	cd build
	cmake ..
	make -j$(nproc)
}

package() {
	cd "$srcdir/$pkgname"
	install -Dm755 "build/src/vgmtrans-qt/vgmtrans" "$pkgdir/usr/bin/$pkgname"
	install -Dm644 "LICENSE.txt" "$pkgdir/usr/share/licenses/$pkgname/LICENSE"
}

Slithery · 2020-04-16 11:10:42

arch=("any")

is incorrect...

https://wiki.archlinux.org/index.php/PKGBUILD#arch

loqs · 2020-04-16 12:16:09

Missing makedepends for git. It is still specifying the number of jobs.
Edit:
You can shorten the build function to:

build() {
	cmake -B build $pkgname
	make -C build
}

package() {
	install -Dm755 "build/src/vgmtrans-qt/vgmtrans" "$pkgdir/usr/bin/$pkgname"
	install -Dm644 "$pkgname/LICENSE.txt" "$pkgdir/usr/share/licenses/$pkgname/LICENSE"
}

Last edited by loqs (2020-04-16 13:02:35)

eschwartz · 2020-04-19 06:15:45

I usually use:

    mkdir -p "${srcdir}"/${pkgname}-${pkgver}/build
    cd "${srcdir}"/${pkgname}-${pkgver}/build

    cmake -G "Unix Makefiles" \
        ..
    make

Arch Linux

#1 2020-04-13 20:13:54

PKGBUILD review request: vgmtrans-git

#2 2020-04-13 20:22:25

Re: PKGBUILD review request: vgmtrans-git

#3 2020-04-13 22:32:21

Re: PKGBUILD review request: vgmtrans-git

#4 2020-04-14 12:15:35

Re: PKGBUILD review request: vgmtrans-git

#5 2020-04-14 12:56:23

Re: PKGBUILD review request: vgmtrans-git

#6 2020-04-16 11:07:26

Re: PKGBUILD review request: vgmtrans-git

#7 2020-04-16 11:10:42

Re: PKGBUILD review request: vgmtrans-git

#8 2020-04-16 12:16:09

Re: PKGBUILD review request: vgmtrans-git

#9 2020-04-19 06:15:45

Re: PKGBUILD review request: vgmtrans-git

Board footer