Received: from vps892.directvps.nl (ikke.info [178.21.113.177])
	by nld3-dev1.alpinelinux.org (Postfix) with ESMTPS id B1BB1782EFD
	for <alpine-aports@lists.alpinelinux.org>; Sat, 25 Jan 2020 15:28:39 +0000 (UTC)
Received: by vps892.directvps.nl (Postfix, from userid 1008)
	id DDFE54400FB; Sat, 25 Jan 2020 16:28:38 +0100 (CET)
Date: Sat, 25 Jan 2020 16:28:38 +0100
From: Kevin Daudt <kdaudt@alpinelinux.org>
To: Duncan Bellamy <dunk@denkimushi.com>
Cc: alpine-aports@lists.alpinelinux.org
Subject: Re: [PATCH] add dcc-servers.net dcc to testing, with sub packages
 for dccif and dccm
Message-ID: <20200125152838.GB1055246@alpha>
References: <20200124214654.13153-1-dunk@denkimushi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200124214654.13153-1-dunk@denkimushi.com>

Hello Duncan,

Welcome to the Alpine Linux community, and thank you for your
contribution. I have placed some feedback inline.

Besides that, aport uses a specific standard for commit messages:

1) A generally followed commit standard where you have a short commit
subject soft limitted to about 50 characters, followed by a blank line,
and then more details about the commit (focussing more on why than what.
See this [post][commit-message] for more information.
2) Commit messages for packages are prefixed with the repo and package
name like this: 'testing/dcc: '.

So the commit message for this patch would be:

```
testing/dcc: new aport
```

The fact that there are subpackages is not that interesting (most
packages will have subpackages).

I hope this is clear, otherwise, feel free to ask questions.

Kind regards, Kevin.

On Fri, Jan 24, 2020 at 09:46:54PM +0000, Duncan Bellamy wrote:
> ---
>  testing/dcc/APKBUILD | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 testing/dcc/APKBUILD
> 
> diff --git a/testing/dcc/APKBUILD b/testing/dcc/APKBUILD
> new file mode 100644
> index 0000000000..6efcfd6e3b
> --- /dev/null
> +++ b/testing/dcc/APKBUILD
> @@ -0,0 +1,61 @@
> +# Contributor: Duncan Bellamy <dunk@denkimushi.com>
> +# Maintainer: Duncan Bellamy <dunk@denkimushi.com>
> +pkgname="dcc"
> +pkgver="2.3.167"
> +pkgrel=1
> +pkgdesc="Distributed Checksum Clearinghouses or DCC spam filter"
> +url="https://www.dcc-servers.net/dcc/"
> +arch="all"
> +license="Distributed Checksum Clearinghouse"

If the license is not on the [spdx][spdx] list, this should be `custom`.

> +pkgusers="_dcc"
> +pkggroups="_dcc"

Any reason why these are prefixed with an '_'? In any case, this just
makes sure that they exist during building. You still need to add a
pre-install script to make sure they exist on the target system.

> +depends="wget"
> +makedepends="libmilter-dev"
> +subpackages="$pkgname-dccifd $pkgname-dccm $pkgname-doc"
> +source="https://www.dcc-servers.net/src/dcc/old/dcc-2.3.167.tar.Z"
> +options="!check"
> +
> +prepare() {
> +		default_prepare

Inconsistent indentation here

> +	sed -i 's+DCC_UNIX+DCC_UNIX\n#include <sys\/types.h>+g' "$srcdir"/$pkgname-$pkgver/include/dcc_types.h

Dependending how much is changed, we prefer using patches instead of
`sed -i`.

> +}
> +
> +unpack() {

Please call default_unpack here as well, as it also includes some other
functionality that is missing now.

Another option is to specify the archive name when downloading in the
source variable:

```sh
source="$pkgname-$pkgver.tar.gz::https://www.dcc-servers.net/src/dcc/old/dcc-2.3.167.tar.Z"
```

Then the default behaviour should suffice.

> +	cd "$srcdir"
> +	tar -xzf dcc*.tar.Z
> +}
> +
> +build() {
> +	./configure \
> +		--with-installroot="$pkgdir" \
> +		--bindir="/usr/bin" \
> +		--mandir="/usr/man" \
> +		--with-uid=_dcc
> +	make
> +}
> +
> +package() {
> +	make install
> +	install -Dm644 "$builddir"/LICENSE \
> +		"$pkgdir"/usr/share/licenses/$pkgname/LICENSE
> +
> +	chmod 755 "$pkgdir"/var/dcc/libexec/dccsight
> +	cd "$pkgdir"/usr/bin
> +	chmod 755 cdcc dccproc
> +}
> +
> +dccifd() {
> +	depends="dcc"
> +	mkdir -p "$subpkgdir"/var/dcc/libexec
> +	cd "$pkgdir"/var/dcc/libexec
> +	mv dccifd start-dccifd "$subpkgdir"/var/dcc/libexec/

abuild now has a function called `amove` that makes this easier. The
above 3 lines can be replaced with:

```sh
amove var/dcc/libexec/dccifd var/dcc/libexec/start-dccifd
```

> +}
> +
> +dccm() {
> +	depends="dcc"
> +	mkdir -p "$subpkgdir"/var/dcc/libexec
> +	cd "$pkgdir"/var/dcc/libexec
> +	mv dccm start-dccm "$subpkgdir"/var/dcc/libexec/

`amove` can be used here as well.

> +}
> +
> +sha512sums="384a572e5b18bed6aed08dce6ebc468d5737b0cb4774fe502f527b101a38b4bec1fdd73384c6fb437c21ae46aa56ae04c5c459737cdda6ab3ce186ff4f77cf98  dcc-2.3.167.tar.Z"
> -- 
> 2.24.1

[commit-message]:https://chris.beams.io/posts/git-commit/
[spdx]:https://spdx.org/licenses/