You are not logged in.

#1 2014-02-20 00:13:21

djraymondnm
Member
From: Socorro, NM, USA
Registered: 2011-12-28
Posts: 59
Website

First package (lprng) request review

Hello,

I recently took over the maintenance of the lprng package in the AUR, and I could use a review of my packaging.

Particular issues:

1. I put the configuration stuff in /etc/lprng rather than /etc, as lprng normally does, in order to reduce clutter in /etc.  Does this make sense?

2. I put some sample print filters and a README file in /etc/lprng/lpd rather than in /usr/lib, in order to make them more accessible to editing.

3. I had to create a service file for lpd, but I'm not sure whether my install procedure meets Arch standards.  Can we assume that the user has systemd installed?

Here is my PKGBUILD file:

# Maintainer: David Raymond <raymond at kestrel dot nmt dot edu>
# Contributor: Alex Suykov <axs at ukr dot net>
# Contributor: Frank Thieme <frank at fthieme dot net>
pkgname=lprng
pkgver=3.8.C
pkgrel=2
pkgdesc="An Enhanced Printer Spooler"
arch=('i686' 'x86_64')
url="http://www.lprng.com"
license=('custom:Artistic')
depends=(openssl bash)
optdepends=(
	'xpdf: pdf to ps conversion in filters'
	'enscript: text to ps conversion in filters'
	'ghostscript: convert ps to device language'
)
conflicts=(cups)
source=(http://sourceforge.net/projects/lprng/files/lprng/lprng-3.8.C.tar.gz lpd.service gsfilter psfilter printcap_remote printcap_server README lpd.conf.simple lpd.perms.simple lprng.install)
install=lprng.install
md5sums=('5901bed95e61d2bea3ba3056056af432'
         '990745083e4f627a714569dc20816b0a'
         'dfe47679d1f1289f8f54e2925f99dad7'
         '58a4bf1a740b67fea83f7f68336f20e7'
         '496dfbb160f2a0308b7847541b03c8db'
         'b03c00aeb5f92b7510343ad3d046aa98'
         '5e8f71feba4048818221352afbafff8d'
         '3ef9f91dc0de273da1f5f1b20d49cd17'
         '3f861c75c2d68c0e45b4095ab39ba1c8'
         '0656eb5410652b654cddbad672755d77')

build() {
  cd "$srcdir/lprng-$pkgver"

  ./configure --prefix=/usr --sysconfdir=/etc/lprng --localstatedir=/var\
      --mandir=/usr/share/man --libexecdir=/usr/lib/lprng\
      --with-userid=daemon --with-groupid=lp\
      --enable-ssl
  make -j1 || return 1
}

package() {
  cd "$srcdir/lprng-$pkgver"

  make -j1 MAKEPACKAGE=YES DESTDIR="$pkgdir/" install
  mkdir $pkgdir/usr/lib/systemd
  mkdir $pkgdir/usr/lib/systemd/system/
  install -D -m 0644 ../lpd.service $pkgdir/usr/lib/systemd/system/
  install -D -m 0644 COPYRIGHT $pkgdir/usr/share/licenses/lprng/COPYRIGHT
  install -D -m 0755 ../gsfilter $pkgdir/etc/lprng/lpd/
  install -D -m 0755 ../psfilter $pkgdir/etc/lprng/lpd/
  install -D -m 0644 ../printcap_remote $pkgdir/etc/lprng/
  install -D -m 0644 ../printcap_server $pkgdir/etc/lprng/
  install -D -m 0644 ../README $pkgdir/etc/lprng/
  install -D -m 0644 ../lpd.conf.simple $pkgdir/etc/lprng/lpd/
  install -D -m 0644 ../lpd.perms.simple $pkgdir/etc/lprng/lpd/
}

here is lprng.install

post_install() {
	       mkdir /var/spool/lpd
	       chown daemon /var/spool/lpd
	       chgrp lp /var/spool/lpd
	       chmod 0700 /var/spool/lpd
	       echo 'See /etc/lprng/README to configure'
}


post_upgrade() {
		/usr/bin/systemctl reenable lpd.service
		/usr/bin/systemctl restart lpd.service
}

pre_remove() {
	     /usr/bin/systemctl stop lpd.service
	     /usr/bin/systemctl disable lpd.service
}

and here is lpd.service:

[Unit]
Description=Lprng Printer Daemon
After=network.target

[Service]
Type=forking
ExecStart=/usr/sbin/lpd
ExecReload=/bin/kill -HUP $MAINPID
KillMode=process

[Install]
WantedBy=multi-user.target

Offline

#2 2014-02-20 01:04:26

HalosGhost
Forum Moderator
From: Twin Cities, MN
Registered: 2012-06-22
Posts: 2,089
Website

Re: First package (lprng) request review

Okay, rule 1 of ALUR packages is, "if it builds and works, then it's 'right'." Having said that, there are some things I would recommend changing.

First, if moving default configuration files does not affect how the program runs, then perhaps a place other than /etc would be better. A lot of packages (and packagers) have been pushing to have the default (and example) configurations in /usr/lib (and /usr/share). Then, the notion is, sysadmins place overriding configs in /etc/ and endusers put overriding configs in ~/. If you can follow this style without anything messing up for the program, I would recommend it.

Second, yes, you can assume the user has systemd installed. There are Arch users that do not use systemd, but they will be the ones who modify the PKGBUILD to their setup anyway, so you should not feel the need to deal with their corner case.

Third, put your source array on multiple lines (like you have your optdepends or md5sums). The way it is now is really quite unreadable.

Fourth, you do not need "|| return 1" at the end of the make command; that is really old syntax. You also probably do not need -j1 (that is handled by makepkg's configuration, and you should only override it if there is a good reason to do so).

Fifth, quote all your variables (and be consistent about it). Some places, you have $pkgdir and some places you have "$pkgdir". Personally, I like to be incredibly verbose, so I always use the following:

"${pkgdir}"

The braces are not necessary (though they can be helpful), but you really should quote all variables to get proper expansions.

Sixth, you can clean up your package() function significantly. For example, avoid using `mkdir` to make directories in "${pkgdir}". Do something like this:

install -d "${pkgdir}/usr/lib/systemd/system"

Note that this one command makes both of the directories you made separately. Furthermore, to clean up the install statements you make afterward, you can `cd` back to the "${srcdir}" and then collapse the `install` options. For example:

install -Dm644 COPYRIGHT "${pkgdir}/usr/share/licenses/lprng/COPYRIGHT"
cd "${srcdir}"
install -Dm644 lpd.service "${pkgdir}/usr/lib/systemd/system/"

et cetera.

Most of what I've recommended do not seriously affect how the package will build (though some really might). But, following conventions like the ones above will make your stuff more readable and reusable in the future.

All the best,

-HG

Last edited by HalosGhost (2014-02-20 01:06:06)

Offline

#3 2014-02-20 01:21:45

progandy
Member
Registered: 2012-05-17
Posts: 5,184

Re: First package (lprng) request review

HalosGhost wrote:

First, if moving default configuration files does not affect how the program runs, then perhaps a place other than /etc would be better. A lot of packages (and packagers) have been pushing to have the default (and example) configurations in /usr/lib (and /usr/share). Then, the notion is, sysadmins place overriding configs in /etc/ and endusers put overriding configs in ~/. If you can follow this style without anything messing up for the program, I would recommend it.

I feel unused example configuration and README files belong in /usr/share/doc/lprng and if it is common practice to modify the config regularily add a symlink as e.g. /etc/lprng/examples.


| alias CUTF='LANG=en_XX.UTF-8@POSIX ' |

Offline

#4 2014-02-20 01:41:30

Allan
Pacman
From: Brisbane, AU
Registered: 2007-06-09
Posts: 11,365
Website

Re: First package (lprng) request review

djraymondnm wrote:

here is lprng.install

post_install() {
	       mkdir /var/spool/lpd
	       chown daemon /var/spool/lpd
	       chgrp lp /var/spool/lpd
	       chmod 0700 /var/spool/lpd
	       echo 'See /etc/lprng/README to configure'
}


post_upgrade() {
		/usr/bin/systemctl reenable lpd.service
		/usr/bin/systemctl restart lpd.service
}

pre_remove() {
	     /usr/bin/systemctl stop lpd.service
	     /usr/bin/systemctl disable lpd.service
}

Making the /var/spool/lpd and setting permissions should be done in the PKGBUILD.   Also, don't restart services.

One more thing:

install -D -m 0755 ../gsfilter $pkgdir/etc/lprng/lpd/

Explicitly refer to $srcdir/gsfilter.

Offline

#5 2014-02-20 20:33:38

djraymondnm
Member
From: Socorro, NM, USA
Registered: 2011-12-28
Posts: 59
Website

Re: First package (lprng) request review

Thanks to all for the good advice on packaging.

I still have one question.  Allan said

Also, don't restart services.

If a daemon is upgraded (such as cupsd), the daemon seems to get restarted automatically.  This would also seem to be desirable after upgrading lprng.  First, is this a good assumption?  Second, if it is, how would one do it without doing a systemctl rstart?

Offline

#6 2014-02-20 20:37:32

Spider.007
Member
Registered: 2004-06-20
Posts: 1,175

Re: First package (lprng) request review

djraymondnm wrote:

Thanks to all for the good advice on packaging.

I still have one question.  Allan said

Also, don't restart services.

If a daemon is upgraded (such as cupsd), the daemon seems to get restarted automatically.  This would also seem to be desirable after upgrading lprng.  First, is this a good assumption?  Second, if it is, how would one do it without doing a systemctl rstart?

No; there are no daemons that reload themselves and that is a good thing. It is up to the user to decide when to upgrade; and when to restart something. Maybe the user knows an application is using the package he's upgrading and wants to schedule a restart at a different time. There isn't a single package that restarts itself (it might not even be running) so you should follow the standards here smile

Offline

#7 2014-02-20 20:45:29

WorMzy
Forum Moderator
From: Scotland
Registered: 2010-06-16
Posts: 11,787
Website

Re: First package (lprng) request review

If a daemon is upgraded (such as cupsd), the daemon seems to get restarted automatically.

This is not something I have ever noticed. If it does happen, then systemd does it automagically, and you don't need to do it in the .install file anyway.

Personally, I would leave it up to the user to {re,}enable services too. Print a message to stdout if you think that the user needs to be aware that they need to enable the service to run it at boot, but that should be pretty obvious.

Minor niggle: what's with the mix of tabs and spaces and the inconsistent indentation? Pick one, tabs or spaces, and decide on a indentation level (e.g. 1 tab, or two spaces, or whatever) to use throughout your files.


Sakura:-
Mobo: MSI MAG X570S TORPEDO MAX // Processor: AMD Ryzen 9 5950X @4.9GHz // GFX: AMD Radeon RX 5700 XT // RAM: 32GB (4x 8GB) Corsair DDR4 (@ 3000MHz) // Storage: 1x 3TB HDD, 6x 1TB SSD, 2x 120GB SSD, 1x 275GB M2 SSD

Making lemonade from lemons since 2015.

Offline

Board footer

Powered by FluxBB