Date: Mon, 21 Aug 2017 00:16:38 +0200
From: =?iso-8859-1?Q?S=F6ren?= Tempel <soeren@soeren-tempel.net>
To: alpine-aports@lists.alpinelinux.org
Cc: jumanjiman@gmail.com
Subject: Re: [alpine-aports] [PATCH] testing/createrepo_c: new aport
Message-ID: <20170820221638.o5yexog4gaclifv4@francium>
References: <20170820211152.GA20563@gmail.com>
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20170820211152.GA20563@gmail.com>

Hey,

First of all: Thanks for your patch. I added some feedback below:

On 20.08.17, Paul Morgan wrote:
> +# This is not ideal, but it allows to create package with recent commits.
> +# git_describe=3D"0.10.0-48-g0cac54d"
> +treeish=3D"0cac54d09b5a2140b625cabad95dc48898e25cdd"
> +upstream_tag=3D0.10.0
> +commits_since=3D48

Is there any reason why you use the tarball created for the commit
instead of using the tarball created for the git-tag? Normally, we
prefer the latter.

For this release the tarball is located at: https://github.com/rpm-software=
-management/createrepo_c/archive/0.10.0.tar.gz

> +pkgver=3D${upstream_tag}.${commits_since}

I would suggest setting the $pkgver to '0.10.0'.

> +makedepends=3D"
> +	alpine-sdk
> +	bash-completion
> +	bzip2-dev
> +	cmake
> +	curl-dev
> +	doxygen
> +	expat-dev
> +	file-dev
> +	glib-dev
> +	libressl-dev
> +	libxml2-dev
> +	musl-dev
> +	py2-sphinx
> +	python2-dev
> +	rpm-dev
> +	scanelf
> +	sqlite-dev
> +	xz-dev
> +	zlib-dev
> +	"

The alpine-sdk and musl-dev dependency are always pulled in by default,
they don't need to be added to $makedepends explicitly.

Besides, we usually group multiple dependencies in one line, at least as
long as that line doesn't exceed 80 characters.

> +subpackages=3D"
> +	${pkgname}-dev-doc:devdoc:noarch

We don't offer doc subpackages for subpackages. A package should only
have a single doc subpackage.

> +	# https://cmake.org/Wiki/CMake_RPATH_handling#Always_full_RPATH
> +	cmake .. \
> +		-DCMAKE_BUILD_TYPE=3DRelease \
> +		-DCMAKE_INSTALL_PREFIX=3D/usr \
> +		-DCMAKE_INSTALL_LIBDIR=3Dlib \
> +		-DCMAKE_SKIP_BUILD_RPATH=3Dfalse \
> +		-DCMAKE_BUILD_WITH_INSTALL_RPATH=3Dfalse \
> +		-DCMAKE_INSTALL_RPATH=3D/usr/lib \
> +		-DCMAKE_INSTALL_RPATH_USE_LINK_PATH=3Dtrue \
> +		-DPYTHON_DESIRED=3D2 \
> +		|| return 1
> +
> +	# Build binaries plus user docs.
> +	make || return 1
> +
> +	# Build dev docs.
> +	make doc || return 1

Explicit `|| return 1` statements are no longer needed since abuild runs
with `set -e` nowadays.

Cheers,
S=F6ren


---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---