PKGBUILD review request: darkradiant from github repo

codereader · 2016-12-28 12:05:16

Hello there,

I'd like to get some feedback about the PKGBUILD below, it's packaging a level editor (for Doom3 and The Dark Mod). It's been available for Debian/Ubuntu already for a long time. Since I've met a few Archers providing feedback about compilation errors and such, I thought I might try my luck at building a proper package for Arch myself. Please let me know if there's anything wrong with it, it's my first attempt:

# Maintainer: codereader <greebo[AT]angua[DOT]at>
pkgname=darkradiant
pkgver=2.1.1
pkgrel=1
epoch=
pkgdesc="Level Editor for Doom 3 (idTech4) and The Dark Mod"
arch=("x86_64")
url="http://darkradiant.sourceforge.net/"
license=('GPL')
depends=("wxgtk>=3.0.0", 
         "ftgl>=2.0.0", 
         "glew>=1.0.0", 
         "boost-libs>=1.46.1", 
         "freealut>=1.0.0", 
         "libvorbis>=1.3.0", 
         "python>=3.5.0", 
         "libsigc++>=2.0.0")
makedepends=("git>=2.0.0", 
             "automake>=1.14", 
             "libtool>=2.4.0", 
             "gcc>=6.0.0", 
             "boost>=1.46.1", 
             "webkitgtk2>=2.4.0")
install=
changelog=
source=("$pkgname-$pkgver::git+https://github.com/codereader/DarkRadiant.git")
md5sums=("SKIP")

build() {
        cd "$pkgname-$pkgver"
        ./autogen.sh
        ./configure --prefix=/usr --enable-darkmod-plugins
        make --jobs=4
}

check() {
        cd "$pkgname-$pkgver"
        make -k check
}

package() {
        cd "$pkgname-$pkgver"
        make DESTDIR="$pkgdir/" install
}

The intention was to get the packaging working until the next DarkRadiant release is ready in a few weeks/months (which will most likely be labeled 2.2.0).

Thanks!

Last edited by codereader (2016-12-28 12:08:44)

Trilby · 2016-12-28 14:04:49

1) Remove the commas from the arrays. This manages to work (some of the time) because the commas are treated as part of the version number. But then it changes the number and makes it harder for the conditional to be met. Also, you almost certainly don't need all those versioned dependencies. Get rid of any version requirements that aren't necessary (I don't see any that are). Just list the packages.

2) Get rid of empty variables (epoch, install, changeLog)

3) Definitely do not specify a number of jobs for make. This is set in each user's makepkg.conf, do not try to override their settings with what happens to be best on your hardware.

4) Are you sure autogen.sh is necessary? Usually this would be done upstream and one would start with configure.

5) Append "-git" to the pkgname as this is a vcs build.

Scimmia · 2016-12-28 14:39:38

To expand on Trilby's #5, please read the VCS package guidelines wiki page. At the least, you need a pkgver function and need to change the way you're renaming the source.

Anything in the base-devel group is assumed to be installed at build time. You should remove them from the makedepends.

Last edited by Scimmia (2016-12-28 14:48:58)

Lone_Wolf · 2016-12-28 15:00:01

The intention was to get the packaging working until the next DarkRadiant release is ready in a few weeks/months (which will most likely be labeled 2.2.0).

codereader, that sounds like you don't want this to be a *-git package, but based on a tarball.
If that is correct, maybe you could release a pre-2.2.0 snapshot to use as source tarball ?

codereader · 2016-12-28 15:51:04

Thanks for the quick responses!

Trilby wrote:

1) Remove the commas from the arrays. This manages to work (some of the time) because the commas are treated as part of the version number. But then it changes the number and makes it harder for the conditional to be met. Also, you almost certainly don't need all those versioned dependencies. Get rid of any version requirements that aren't necessary (I don't see any that are). Just list the packages.

I tried that first (just specifying the package names), but then makepkg complained about missing dependencies (even though all of them are installed on my system, I wouldn't be able to compile and run it otherwise). Running makepkg -s didn't make a difference. I tried to google that error and all forum posts or blogs I could find were related to dependencies pointing in the AUR - but I don't depend on anything in the AUR, it's all in the official package repos. Adding that ">=X.Y.Z" requirements was the only workaround I could find, so I went with that. Any ideas about what I might have done wrong to cause makepkg to fail?

Trilby wrote:

2) Get rid of empty variables (epoch, install, changeLog)
3) Definitely do not specify a number of jobs for make. This is set in each user's makepkg.conf, do not try to override their settings with what happens to be best on your hardware.

Will do, thanks.

Trilby wrote:

4) Are you sure autogen.sh is necessary? Usually this would be done upstream and one would start with configure.

I'm not 100% sure (not an automake expert), but I think the freshly-cloned souces lack a few files which are generated by ./autogen.sh. That's why I added it. If it's best practice to include those generated files, then I'll see to getting them into git and remove the ./autogen.sh from PKGBUILD.

Trilby wrote:

5) Append "-git" to the pkgname as this is a vcs build.

It's not my intention to be a VCS build. I intend to create the package from a defined release tag once 2.2.0 is released. I haven't googled that yet, though, so I don't know (yet) how to proceed.

Scimmia wrote:

Anything in the base-devel group is assumed to be installed at build time. You should remove them from the makedepends.

Thanks, I'll have to check which these are and remove them.

Lone_Wolf wrote:

codereader, that sounds like you don't want this to be a *-git package, but based on a tarball.
If that is correct, maybe you could release a pre-2.2.0 snapshot to use as source tarball ?

So I do get this right, if PKGBUILD is drawing the version from the latest in the master branch then I have to add -git and a pkgver function? If I put a defined git commit into a tarball, upload and reference that, then I'll be allowed to name it something like 2.2.0pre1?

Trilby · 2016-12-28 16:02:07

codereader wrote:

I tried that first (just specifying the package names), but then makepkg complained about missing dependencies (even though all of them are installed on my system

That's because of the commas, get rid of them. Bash arrays do not use commas, they are treated as part of the array element. So pacman couldn't find a package called "wxgtk," because there is no such package (try `pacman -S wxgtk,` it will fail). With your current versioned dependencies wxgtk version 3.0.0 would not satisfy the requirement as 3.0.0 is not => "3.0.0,". If 3.0.0 was what was in the repos, this would fail. But we have 3.0.2 in the repos, and 3.0.2 > "3.0.0," with or without the comma.

eschwartz · 2016-12-28 18:19:44

autogen.sh should be used for development packages, for releases you should make a dist tarball and upload that with the uncommitted generated files included. (You cannot rely on GitHub's source snapshots which are automatically created for tags/commits using git-archive(1) in this case.)

If it is appropriate for users to get a non-VCS build that isn't the latest release, tag a prerelease and update the PKGBUILD to that. Or just use the latest release.
Any PKGBUILD that pulls from a git branch is already a VCS package, whether you intended it or not. So it must have an appropriate pkgname and a useful pkgver.

Git sources must not contain $pkgver in the source array (i.e. "$pkgname-$pkgver::$url"), since this will cause the repo to be freshly cloned into a new directory for each new pkgver (which will be different for each and every commit).

codereader · 2016-12-28 18:37:45

Trilby wrote:

That's because of the commas, get rid of them. Bash arrays do not use commas, they are treated as part of the array element. So pacman couldn't find a package called "wxgtk," because there is no such package (try `pacman -S wxgtk,` it will fail). With your current versioned dependencies wxgtk version 3.0.0 would not satisfy the requirement as 3.0.0 is not => "3.0.0,". If 3.0.0 was what was in the repos, this would fail. But we have 3.0.2 in the repos, and 3.0.2 > "3.0.0," with or without the comma.

Oh, the commas outside the quotation marks are added to the string? Didn't see that one coming, thanks for the hint!

I think I'm getting the idea of the different versioning and naming conventions for VCS-based packages and the tarball ones. I want to keep it simple for myself and just provide a PKGBUILD which provides the recent 2.1.0 release, and whenever 2.2.0 comes out I'll adjust the PKGBUILD.

So, this is the working state of my PKGBUILD now, aimed to deliver the latest release 2.1.0,. Is this ok? It's using a defined branch in git to pull the 2.1.0 sources including the necessary fixes for Arch that weren't in the original 2.1.0 release tag (which doesn't compile in Arch). Is it acceptable to base the 2.1.0 release package on this git branch or does that qualify as a VCS-package only?

# Maintainer: codereader <greebo[AT]angua[DOT]at>
pkgname=darkradiant
pkgver=2.1.0
pkgrel=1
pkgdesc="Level Editor for Doom 3 (idTech4) and The Dark Mod"
arch=("x86_64")
url="http://darkradiant.sourceforge.net/"
license=("GPL")
depends=(wxgtk ftgl glew boost-libs freealut libvorbis python libsigc++)
makedepends=(git boost webkitgtk2)
source=("$pkgname-$pkgver::git+https://github.com/codereader/DarkRadiant.git#branch=2.1.0arch")
md5sums=("SKIP")

build() {
	cd "$pkgname-$pkgver"
	./configure --prefix=/usr --enable-darkmod-plugins
	make
}

check() {
	cd "$pkgname-$pkgver"
	make -k check
}

package() {
	cd "$pkgname-$pkgver"
	make DESTDIR="$pkgdir/" install
}

eschwartz · 2016-12-28 18:47:40

Branches are for updating, tags are for releases.

Sounds like you want to cherry-pick specific commits on top of a release, you can do that too -- just use git sources, at "#tag=$pkgver", and in prepare() you can cherry-pick some backports. The non-git version of that would be to apply patches.

You still need to stop adding $pkgver to the downloaded source name. Unique filenames are ONLY for tarballs and the like. Git pulls new commits into the same repo (duh) and even the "$srcdir" checkout has more than sufficient consistency checks supplied by git itself.

codereader · 2016-12-28 19:05:08

Eschwartz wrote:

Sounds like you want to cherry-pick specific commits on top of a release, you can do that too -- just use git sources, at "#tag=$pkgver", and in prepare() you can cherry-pick some backports. The non-git version of that would be to apply patches.

Is it absolutely required to use a tag instead of a branch to base the 2.1.0 arch package on, and apply the required fixes separately in the prepare() function? That's basically what I already did in the source repo, I cherry-picked 3 or 4 commits on top of the existing "2.1.0" release tag and named it "2.1.0arch", for reference by the PKGBUILD.

Eschwartz wrote:

You still need to stop adding $pkgver to the downloaded source name. Unique filenames are ONLY for tarballs and the like. Git pulls new commits into the same repo (duh) and even the "$srcdir" checkout has more than sufficient consistency checks supplied by git itself.

Got it, I'll change it to clone into the same $pkgname folder now.

This is the current state of the PKGBUILD file:

# Maintainer: codereader <greebo[AT]angua[DOT]at>
pkgname=darkradiant
pkgver=2.1.0
pkgrel=1
pkgdesc="Level Editor for Doom 3 (idTech4) and The Dark Mod"
arch=("x86_64")
url="http://darkradiant.sourceforge.net/"
license=("GPL")
depends=(wxgtk ftgl glew boost-libs freealut libvorbis python libsigc++)
makedepends=(git boost webkitgtk2)
source=("$pkgname::git+https://github.com/codereader/DarkRadiant.git#branch=2.1.0arch")
md5sums=("SKIP")

build() {
	cd "$pkgname"
	./configure --prefix=/usr --enable-darkmod-plugins
	make
}

check() {
	cd "$pkgname"
	make -k check
}

package() {
	cd "$pkgname"
	make DESTDIR="$pkgdir/" install
}

Trilby · 2016-12-28 19:18:51

That looks great. Eschwartz can follow up on the best way to use a branch/tag whatever. But that now looks like a good quality PKGBUILD to me.

One further revision is that if this is a versioned release, please include a checksum. Makepkg can generate this for you. You only use 'SKIP' for vcs builds.

Also, I just realized this was your project and you are working on this just to make it more accessible to archers. It's great that you're taking the time to do so - thanks. If you regularly use arch linux great. But if you're tinkering with arch just to make this PKGBUILD it might be best to let an archer who uses darkradiant maintain the PKGBUILD as they will be rebuilding and actually using it with each release. I'm sure they'd appreciate your assitance in keeping it working, but their more properly "dog-fooded" PKGBUILD might prove more robust if archlinux isn't your regular OS/distro: upstream being responsive to questions in input from packagers is wonderful - in contrast upstream developers trying to maintain packages for distros they don't regularly use can be messy.

codereader · 2016-12-28 19:48:46

Trilby wrote:

That looks great. Eschwartz can follow up on the best way to use a branch/tag whatever. But that now looks like a good quality PKGBUILD to me.

Thanks

Trilby wrote:

One further revision is that if this is a versioned release, please include a checksum. Makepkg can generate this for you. You only use 'SKIP' for vcs builds.

I do this using the updpkgsums command, right? I ran it in the same directory as PKGBUILD, but it just added "md5sums=('SKIP')" to my file? Does it choke on the fact that I'm referring to a git repo in the sources?

Trilby wrote:

Also, I just realized this was your project and you are working on this just to make it more accessible to archers. It's great that you're taking the time to do so - thanks.

It's more or less "my" project, yes. There's been two of us working on this, but nowadays I'm mostly working on my own. Here and then somebody stops by the Dark Mod forums or bugtracker and points out that the current code in git is not compiling in their distribution. In some cases they were running Arch Linux, and since they were both helpful and grateful, I figured it would be a nice move to provide an Arch package. Usually, I just try to make it compile in Debian/Ubuntu, since that's the one with the largest userbase.

Trilby wrote:

If you regularly use arch linux great. But if you're tinkering with arch just to make this PKGBUILD it might be best to let an archer who uses darkradiant maintain the PKGBUILD as they will be rebuilding and actually using it with each release. I'm sure they'd appreciate your assitance in keeping it working, but their more properly "dog-fooded" PKGBUILD might prove more robust if archlinux isn't your regular OS/distro: upstream being responsive to questions in input from packagers is wonderful - in contrast upstream developers trying to maintain packages for distros they don't regularly use can be messy.

I don't use any Linux distro regularly, I mainly run them in VMs (I must have 6 of them by now). If there's somebody volunteering I'm more than happy for them to take over that PKGBUILD script, but are there people willing to do that?

By the way, how should I proceed now with that PKGBUILD file? Is this https://wiki.archlinux.org/index.php/Ar … g_packages the correct article?

Last edited by codereader (2016-12-28 19:51:40)

eschwartz · 2016-12-28 19:55:08

Well, using a tag and cherry-picking makes more sense to me, because 2.1.0arch can unexpectedly get more commits in which case you really need a pkgver() again. Whereas you can and should bump the pkgrel whenever adding a backported buildfix or whatever. See the systemd PKGBUILD in [core] for a good example of doing this both right and clean.

Granted you are upstream in this case, which means you have the authority to push a new branch in git... but that doesn't necessarily mean you should, it leaves the question "what happens to this hotfix branch (technically 2.1.1) after the next release", and turns this into part of your release engineering (in which case you might as well just tag 2.1.1 outright with all that that entails.)
Whereas pulling in upstream patches to fix a build in advance of a release, is a respected Arch tradition.

...

As Trilby said, your new PKGBUILD essentially looks good (+- my policy arguments), with the following caveats:
I don't know whether you intended to say that it only works on x86_64, or if that was merely an oversight (because fewer and fewer people are using i686).
Your git checkout is skipping autogen.sh -- did you commit the results? That is not always the best idea, since it is mostly generated code and can change in diff-killing ways, although if you are okay with that I won't say anything further.

Last edited by eschwartz (2016-12-28 20:22:15)

Arch Linux

#1 2016-12-28 12:05:16

PKGBUILD review request: darkradiant from github repo

#2 2016-12-28 14:04:49

Re: PKGBUILD review request: darkradiant from github repo

#3 2016-12-28 14:39:38

Re: PKGBUILD review request: darkradiant from github repo

#4 2016-12-28 15:00:01

Re: PKGBUILD review request: darkradiant from github repo

#5 2016-12-28 15:51:04

Re: PKGBUILD review request: darkradiant from github repo

#6 2016-12-28 16:02:07

Re: PKGBUILD review request: darkradiant from github repo

#7 2016-12-28 18:19:44

Re: PKGBUILD review request: darkradiant from github repo

#8 2016-12-28 18:37:45

Re: PKGBUILD review request: darkradiant from github repo

#9 2016-12-28 18:47:40

Re: PKGBUILD review request: darkradiant from github repo

#10 2016-12-28 19:05:08

Re: PKGBUILD review request: darkradiant from github repo

#11 2016-12-28 19:18:51

Re: PKGBUILD review request: darkradiant from github repo

#12 2016-12-28 19:48:46

Re: PKGBUILD review request: darkradiant from github repo

#13 2016-12-28 19:55:08

Re: PKGBUILD review request: darkradiant from github repo

Board footer