Sender: =?UTF-8?Q?Timo_Ter=C3=A4s?= <timo.teras@gmail.com>
Date: Wed, 6 May 2015 22:27:37 +0300
From: Timo Teras <timo.teras@iki.fi>
To: Marian Buschsieweke <mari.hahn@wwu.de>
Cc: alpine-aports@lists.alpinelinux.org
Subject: Re: [alpine-aports] [PATCH] testing/raspicomm-module: new aport
Message-ID: <20150506222737.74a4521d@vostro>
In-Reply-To: <1430504324-5520-1-git-send-email-mari.hahn@wwu.de>
References: <1430504324-5520-1-git-send-email-mari.hahn@wwu.de>
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hi,

Comments inline.

On Fri,  1 May 2015 20:18:44 +0200
Marian Buschsieweke <mari.hahn@wwu.de> wrote:

> $pkgurl  "http://www.amescon.com/products/raspicomm/"
> $pkgdesc "Support Amescon's RasPiComm module: Kernel module for
> RS-485 Port and OpenRC services to set up RTC, Joystick and LEDs"
> 
> The RasPiConn module is a board to attach on a Raspberry Pis GPIO
> PINs that provides an RS-232 Port, RS-485 Port, a real time clock, a
> joystick (digital), and two LEDs.
> ---
> 
> diff --git a/testing/raspicomm-module/APKBUILD
> b/testing/raspicomm-module/APKBUILD new file mode 100644
> index 0000000..e8ca347
> --- /dev/null
> +++ b/testing/raspicomm-module/APKBUILD
> @@ -0,0 +1,58 @@
> +# Contributor: Marian Buschsieweke (mari (dot) hahn (at) wwu (dot)
> de) +# Maintainer:
> +pkgname=raspicomm-module
> +pkgver=1.4
> +pkgrel=0
> +pkgdesc="Support Amescon's RasPiComm module: Kernel module for
> RS-485 Port and OpenRC services to set up RTC, Joystick and LEDs"
> +url="http://www.amescon.com/products/raspicomm/" +arch="all"
> +license="unknown"
> +depends="linux-rpi"
> +depends_dev="linux-rpi-dev"
> +makedepends="$depends_dev"
> +install="${pkgname}.post-install ${pkgname}.post-upgrade
> ${pkgname}.post-deinstall" +subpackages="" #"$pkgname-dev
> $pkgname-doc"
> +source="saveas-http://github.com/amescon/${pkgname}/archive/v${pkgver}.tar.gz/raspicomm-module-v${pkgver}.tar.gz
> +        raspicomm-rtc
> +        raspicomm-joystick
> +        raspicomm-leds"
> +
> +_builddir="${srcdir}/${pkgname}-${pkgver}"
> +_module_dir="/lib/modules/$(apk info linux-rpi | grep description |
> sed -e 's/ description://' -e 's/linux-rpi-//' -e 's/r//')-rpi"

You are not allowed to spawn sub-shells, or even worse call apk from
APKBUILD global context. See main/dahdi-linux-grsec for example how to
do kernel plugins.

> diff --git a/testing/raspicomm-module/raspicomm-module.post-deinstall
> b/testing/raspicomm-module/raspicomm-module.post-deinstall new file
> mode 100755 index 0000000..578448c
> --- /dev/null
> +++ b/testing/raspicomm-module/raspicomm-module.post-deinstall
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +echo "Deleting group \"gpio\""
> +delgroup gpio

Groups should not be deleted on uninstall. Please remove this.

> diff --git a/testing/raspicomm-module/raspicomm-module.post-install
> b/testing/raspicomm-module/raspicomm-module.post-install new file
> mode 100755 index 0000000..8bb6441
> --- /dev/null
> +++ b/testing/raspicomm-module/raspicomm-module.post-install
> @@ -0,0 +1,13 @@
> +#!/bin/sh
> +echo "Running depmod"
> +depmod

depmod is unneeded. apk will run busybox's trigger after all packages
are installed/upgraded; it'll run depmod for you. Remove this.

> +echo "Creating group \"gpio\""
> +addgroup gpio

This should be silent, and done with -S (to create system group). See
other packages how groups are created silently. You will also need
"pkggroups" in the main APKBUILD.

> +echo "The follwing OpenRC Services were installed:"
> +echo "  - \"raspicomm-rtc\": Sets up the real time clock"
> +echo "  - \"raspicomm-joystick\": (Un-)Exports the joystick's GPIOs"
> +echo "  - \"raspicomm-leds\": (Un-)Exports the LEDs GPIOs"
> +echo
> +echo "For convenience \"raspicomm-joystick\" and \"raspicomm-leds\"
> create" +echo "symlinks to corret GPIOs under \"/dev/raspicomm\". The
> GPIOs will be " +echo "readable and writable by users in group
> \"gpio\"." diff --git

> a/testing/raspicomm-module/raspicomm-module.post-upgrade
> b/testing/raspicomm-module/raspicomm-module.post-upgrade new file
> mode 100755 index 0000000..cf83677 --- /dev/null
> +++ b/testing/raspicomm-module/raspicomm-module.post-upgrade
> @@ -0,0 +1,3 @@
> +#!/bin/sh
> +echo "Running depmod"
> +depmod

Again this needs to go away. It's unneeded.


---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---