[aur-general] PKGBUILD review

[Eli Schwartz]

On 10/18/2017 06:13 PM, Garrett Battaglia via aur-general wrote:
> This is the first time I am packaging for the AUR I would like someone
> to look over my PKGBUILD below.
> 
> Thanks,
> Garrett
> 
> #Maintainer: Garrett Battaglia <garrett at garrettbattaglia.info>
> pkgname=sysdig-falco
> pkgver=0.8.1
> _sysdigver=0.19.1
> pkgrel=1
> pkgdesc="Behavioral Activity Monitoring With Container Support"
> arch=('x86_64')

Why x86_64 only? It is available for both, even the prebuilt binaries.

> url=https://github.com/draios/falco
> license=('GPL')
> depends=('dkms')
> makedepends=('git' 'cmake' 'linux-headers')
> provides=('sysdig-falco')
> 
> source=(
> "http://download.draios.com/stable/rpm/${arch}/falco-${pkgver}-${arch}.rpm"
> "https://github.com/draios/falco/archive/${pkgver}.tar.gz"
> "https://github.com/draios/sysdig/archive/${_sysdigver}.tar.gz"
> "sysdig-falco.install"
> "sysdig-falco.service")
> 
> install=$pkgname.install

You haven't shown us the install file or the service file. install files
are not supposed to be listed in source=()

More than that, files named eg. 0.8.1.tar.gz should be renamed, by
listing the source as e.g.
"${pkgname}-${pkgver}.tar.gz::http://github.com/draios/falco/archive/${pkgver}.tar.gz"

Why on earth are you downloading an rpm for a package that has source
code available in git?

If for some reason you desperately have to, DO NOT use ${arch} in the
filename, it doesn't do what you think it does and only worked through
sheer accident. That gets you the first array element of arch=() which
only works as intended when there is ONLY one element and is not
documented for use, it also breaks sane handling of multi-arch sources. Use:
source_i686=("http://download.draios.com/stable/rpm/i386/falco-${pkgver}-i686.rpm")
source_x86_64=("http://download.draios.com/stable/rpm/x86_64/falco-${pkgver}-x86_64.rpm")

> md5sums=(
> '5e017c747184101a0cc93ffc5b19ca47'
> 'f3c654ded00f3186f3ff92320204a747'
> '6ad8b4a7d1b0aa10cd62397318117a67'
> '8bdb4c61dadd116f4901fa15c20da728'
> '58fe0d406874c6f565f648d3b10da62a')
> 
> prepare() {
> cd ${srcdir}
> mv sysdig-${_sysdigver} sysdig
> mv falco-${pkgver} falco
> rm -rf ./etc/rc.d
> cd ./falco
> mkdir build
> 
> }

All this moving around is ugly, if upstream depends on out-of-tree
sources from another repo you should probably either clone a git tag or
perhaps more ideally `ln -sf sysdig-${_sysdigver} sysdig`.

Currently building a new version without clearing the old version will
break, while that maybe shouldn't be done at all, it is always nice to
do things in a way that plays nicely anyway.

> build() {
> cd $srcdir/falco/build
> cmake ..
> make driver
> }
> 
> package() {
> 
> cd $srcdir
> mkdir $pkgdir/usr
> cp -r ./usr/* $pkgdir/usr/
> mkdir $pkgdir/etc
> cp -r ./etc/* $pkgdir/etc/
> mkdir -p "$pkgdir/var/lib/dkms/falco/${pkgver}/${kernelVer}/${arch}/module/"
> cp "$srcdir/falco/build/driver/falco-probe.ko"
> "$pkgdir/var/lib/dkms/falco/${pkgver}/${kernelVer}/${arch}/module/falco-probe.ko"
> mkdir -p $pkgdir/usr/lib/systemd/system/
> cp sysdig-falco.service $pkgdir/usr/lib/systemd/system/
> }

If I read this correctly, the only thing you ever did here was build a
copy of sysdig-probe.ko (provided by the sysdig package via dkms) named
falco-probe.ko, and then use the prebuilt binary artifacts from some rpm.

This should be built properly, against system libraries. Either that or
name this package "sysdig-falco-bin" to indicate it is a package
containing prebuilt binaries, and to clear the namespace for someone
else to upload a proper built-from-source package.

Also you repeatedly fail to quote $srcdir and $pkgdir, which means your
PKGBUILD will fail to compile if built from a directory with spaces e.g.
"~/AUR packages/".
And on a stylistic level, you erratically switch from $variable to
${variable}, please choose one style and stick to it.

-- 
Eli Schwartz

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/aur-general/attachments/20171018/fda60c4d/attachment.asc>