[aur-general] Perl PKGBUILD review
Hi all,
I'm new to Perl packaging. I wrote my first PKGBUILD [1] following the
wiki [2].
I would be very grateful if somebody could give it a read and suggest
improvements? namcap does not emit any warning.
|# Maintainer: François Freitag <mail at franek dot fr> # Contributor:
Victor van den Elzen
Hi François! Would it be possible for you to upload the PKGBUILD to a pastebin service (e.g. https://ptpb.pw/) and provide the link? Or better yet, does your package [1] have the latest changes? It looks like it does. Asking because the formatting was destroyed in your email. Regards, Andrew [1] https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=pgbadger On 12/2/17 4:02 PM, François Freitag wrote:
Hi all,
I'm new to Perl packaging. I wrote my first PKGBUILD [1] following the wiki [2]. I would be very grateful if somebody could give it a read and suggest improvements? namcap does not emit any warning.
|# Maintainer: François Freitag <mail at franek dot fr> # Contributor: Victor van den Elzen
pkgname=pgbadger pkgver=9.2 pkgrel=4 pkgdesc="A fast PostgreSQL Log Analyzer" arch=("any") url="https://github.com/dalibo/pgbadger" license=("custom:Dalibo") depends=('perl>=5.10.0') optdepends=( "perl-text-csv-xs: parse PostgreSQL CSV log files" "perl-json-xs: export statistics as JSON file" ) options=('!emptydirs') source=("https://github.com/dalibo/pgbadger/archive/v${pkgver}.tar.gz") sha256sums=('2107466309a409fb9e40f11bb77cac1f9ba7910d5328e7b2e08eb7a1c6d760ec') prepare() { # Override perl command line options we don't want. Source: # https://wiki.archlinux.org/index.php/Perl_Policy#Vendor export PERL_MM_USE_DEFAULT=1 PERL_AUTOINSTALL=--skipdeps \ PERL_MM_OPT="INSTALLDIRS=vendor DESTDIR='$pkgdir'" \ PERL_MB_OPT="--installdirs vendor --destdir '$pkgdir'" \ PERL5LIB="" PERL_LOCAL_LIB_ROOT="" \ MODULEBUILDRC=/dev/null } build() { cd "${srcdir}/pgbadger-${pkgver}" /usr/bin/perl Makefile.PL make } package() { cd "${srcdir}/pgbadger-${pkgver}" install -Dm644 LICENSE -t "${pkgdir}/usr/share/licenses/${pkgname}" make pure_vendor_install INSTALLDIRS=vendor DESTDIR="${pkgdir}" # Remove perllocal.pod and .packlist find "${pkgdir}" \( -name .packlist -o -name perllocal.pod \) -delete } | Thank you for your time, François
[1] https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=pgbadger [2] https://wiki.archlinux.org/index.php/Perl_Policy#Vendor
On 12/02/2017 04:02 PM, François Freitag wrote:
Hi all,
I'm new to Perl packaging. I wrote my first PKGBUILD [1] following the wiki [2]. I would be very grateful if somebody could give it a read and suggest improvements? namcap does not emit any warning.
Well, *if* I assume the PKGBUILD is the same one you uploaded to the AUR, then there are a few odd things about it. You do not need to obfuscate your email address, alternatively you already have older versions that are quite discoverable and contain the unobfuscated version. That being said, you used the single most common "obfuscation", which AFAIK all spambot scrapers know how to interpret. Versioned per dependencies are wrong, we only use versioned dependencies in exceptional cases e.g. gcc and gcc-libs are tightly bound to identical $pkgver-$pkgrel releases. You moved some environment variables from build and package, where they will be run, to prepare, where they may very well not be run. Consider what happens when you use makepkg --nobuild && makepkg --noextract. Then consider than some popular AUR helpers do this automatically, ensuring many users will be affected by this. Please revert that change back to the working version. Do not depend on variables set in one function to be available in other functions. Why did you remove the check() function? Does it not work anymore? You do not meed to remove perllocal.pod and .packlist, as that is already done by the default purge option in makepkg.conf -- admittedly it can be removed from the PKGBUILD since the defaults should work. ... Basically, you updated the pkgver and broke a few things. Note that one actual issue which existed beforehand and which you did *not* fix, is that the source array will download v${pkgver}.tar.gz which clashes with many other packages if you use a shared $SRCDEST. So you should switch that to use: source=("${pkgname}-${pkgver}.tar.gz::${url}/archive/v${pkgver}.tar.gz") Thereby giving the download filename a unique name specific to this package. I use ${url} by habit, you don't have to though. -- Eli Schwartz
On Sun, Dec 03, 2017 at 01:47:51PM -0500, Eli Schwartz via aur-general wrote:
You do not need to obfuscate your email address, alternatively you already have older versions that are quite discoverable and contain the unobfuscated version. That being said, you used the single most common "obfuscation", which AFAIK all spambot scrapers know how to interpret.
In fairness, this has been advised per the AUR wiki page (with accompanying "user at domain dot tld" examples) for a while now. It's pretty pointless, but I doubt anyone reading PKGBUILDs will be confused by it. I tend to follow prior contributor style and keep obfuscations just for giggles.
On 12/03/2017 07:29 PM, beest wrote:
On Sun, Dec 03, 2017 at 01:47:51PM -0500, Eli Schwartz via aur-general wrote:
You do not need to obfuscate your email address, alternatively you already have older versions that are quite discoverable and contain the unobfuscated version. That being said, you used the single most common "obfuscation", which AFAIK all spambot scrapers know how to interpret.
In fairness, this has been advised per the AUR wiki page (with accompanying "user at domain dot tld" examples) for a while now. It's pretty pointless, but I doubt anyone reading PKGBUILDs will be confused by it. I tend to follow prior contributor style and keep obfuscations just for giggles.
Well, at least some people use proper rot13 encryption... I'd definitely agree as far as leaving others' email addresses the way they wrote them. But my email address is already readable in git metadata so I don't really see the point of bothering at all on my own behalf. -- Eli Schwartz
Hi Eli, Thank you very much for the feedback!
On 2017-12-03 10:47 AM, Eli Schwartz via aur-general wrote>
Versioned peer dependencies are wrong, we only use versioned dependencies in exceptional cases e.g. gcc and gcc-libs are tightly bound to identical $pkgver-$pkgrel releases.
I added 'perl>=5.10.0' to the deps because https://wiki.archlinux.org/index.php/Perl_Policy#Vendor_installation recommends: A depends on perl (>= 5.10.0) is required in order ensure that the module is correctly installed into the new @INC path. IIUC, perl directory hierarchy in Arch before perl 5.10 shared the site and vendor directories, resulting in conflicts for users. Did I miss something?
Why did you remove the check() function? Does it not work anymore?
There are no tests for pgbadger, running `make test` in the source directory results in: No tests defined for pgBadger extension. Removing the check function makes the absence of tests clearer IMHO.
You do not need to remove perllocal.pod and .packlist, as that is already done by the default purge option in makepkg.conf -- admittedly it can be removed from the PKGBUILD since the defaults should work.
The purge does not seem to be working as I expect: when I build the PKGBUILD (with the purge option enabled), the package contains an empty directory: $ namcap pgbadger-9.2-5-any.pkg.tar.xz pgbadger W: Directory (usr/lib/perl5/5.26/vendor_perl/auto/pgBadger) is empty I'm using the default OPTIONS in /etc/makepkg.conf: OPTIONS=(strip docs !libtool !staticlibs emptydirs zipman purge !optipng !upx !debug) # purge is active When I look at the state of the pkg directory after the build() function, the only content of ${pkgdir}/usr/lib/perl5/5.26/vendor_perl/auto/pgBadger is a .packlist. After the makepkg command completes, the .packlist is removed from ${pkgdir}, leaving an empty directory. I expected it to be removed by the !emptydirs option. Based on makepkg's output, I see that empty directories are stripped before the purge. ... -> Removing empty directories... -> Removing libtool files... -> Purging unwanted files... ... Maybe that's why the directory is not removed? If that's correct, is there a better alternative than the `find` command to remove unwanted files?
[...] you should switch that to use: source=("${pkgname}-${pkgver}.tar.gz::${url}/archive/v${pkgver}.tar.gz")
Thereby giving the download filename a unique name specific to this> package. I use ${url} by habit, you don't have to though.
I like the ${url} trick, it avoids verifying the url twice for users. Thanks again for the help! François
participants (4)
-
Andrew Crerar
-
beest
-
Eli Schwartz
-
François Freitag