Re: [PATCH] systemd/*: Use @SBINDIR@.

[Ladislav Michl]

Hi Greg,

On Mon, Aug 03, 2020 at 06:35:04AM -0400, Greg Troxel wrote:
> Ladislav Michl <ladis@linux-mips.org> writes:
> 
> 
> This patch isn't explained well enough.  It's particularly not explained
> why it won't break anything else.

I'll explain in this reply sending v2 eventually if we come to conclusion.

> > diff --git a/.gitignore b/.gitignore
> > index fd0b32d8e..5331d4548 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -197,6 +197,7 @@ tmp*
> >  # config file created by gpsd.php
> >  gpsd_config.inc
> >  # for systemd(umb)
> > +systemd/gpsd.service
> >  systemd/gpsdctl@.service
> >  systemd/gpsd.socket
> >  # for 3rd party packaging
> 
> This hunk I don't object to.
> 
> > diff --git a/SConstruct b/SConstruct
> > index 9385b96d9..d91f5b067 100644
> > --- a/SConstruct
> > +++ b/SConstruct
> > @@ -2080,6 +2080,7 @@ substmap = (
> >      ('@PYPACKETH@',  pythonized_header),
> >      ('@QTVERSIONED@', env['qt_versioned']),
> >      ('@RUNDIR@',     env['rundir']),
> > +    ('@SBINDIR@',    installdir('sbindir', add_destdir=False)),
> >      ('@SCPUPLOAD@',  scpupload),
> >      ('@SHAREPATH@',  installdir('sharedir')),
> >      ('@SITENAME@',   sitename),
> 
> I'm really not sure what is going on here.  It is a core principle that
> "make install", however spelled, respects destdir and does not try to
> write outside of it.

Yes, "make install" respects DESTDIR, but here we want to use @SBINDIR@ the
same way @LIBDIR@ is used - to contain a path on target system.

> The commit needs a clear explanation of the problem, what the solution
> is, how it works, and why it won't break anybody else.  Yes, I can sort
> of back that out from the diff, but not quite.
> 
> If you are just trying to add a variable SBINDIR so you can use it, then
> I don't see why the destdir comment applies; destdir is used to install,
> but isn't part of the directory paths.

And that's exactly why add_destdir is set to False.

> Do you find that SBINDIR ends up with the destdir prepended?   If so
> that feels like a bug.

Without second argument add_destdir defaults to True as current SConstruct
carries this implementation:
def installdir(idir, add_destdir=True):
    # use os.path.join to handle absolute paths properly.
    wrapped = os.path.join(env['prefix'], env[idir])
    if add_destdir:
        wrapped = os.path.normpath(DESTDIR + os.path.sep + wrapped)
    wrapped.replace("/usr/etc", "/etc")
    wrapped.replace("/usr/lib/systemd", "/lib/systemd")
    return wrapped

So yes, @SBINDIR@ would end with DESTDIR prepended. Not a bug, it just
depends how installdir is used.

> > diff --git a/systemd/gpsd.service b/systemd/gpsd.service.in
> > similarity index 82%
> > rename from systemd/gpsd.service
> > rename to systemd/gpsd.service.in
> > index c1f193cc6..768e3dcd6 100644
> > --- a/systemd/gpsd.service
> > +++ b/systemd/gpsd.service.in
> > @@ -8,7 +8,7 @@ After=chronyd.service
> >  Type=forking
> >  EnvironmentFile=-/etc/default/gpsd
> >  EnvironmentFile=-/etc/sysconfig/gpsd
> > -ExecStart=/usr/local/sbin/gpsd $GPSD_OPTIONS $OPTIONS $DEVICES
> > +ExecStart=@SBINDIR@/gpsd $GPSD_OPTIONS $OPTIONS $DEVICES
> >  
> >  [Install]
> >  WantedBy=multi-user.target
> 
> I don't use systemd but this seems ok.  I am surprised that it was ever
> working before as my understanding is that GNU/Linux norms are to put
> everything in /usr.

Well, gpsd default is /usr/local prefix and...

> Do systemd-using GNU/Linux packaging systems patch this file, because
> they put gpsd in /usr vs /usr/local?  That would be nice to have in the
> commit message too.

...package maintainers often do things at their own, so it worked.
I just wanted to make things running out of the box for whatever
prefix.

In theory, we could also do something like this:

diff --git a/packaging/deb/etc_init.d_gpsd.in b/packaging/deb/etc_init.d_gpsd.in
index dcba68c93..6d6e27139 100644
--- a/packaging/deb/etc_init.d_gpsd.in
+++ b/packaging/deb/etc_init.d_gpsd.in
@@ -26,7 +26,7 @@
 PATH=/sbin:/usr/sbin:/bin:/usr/bin
 DESC="GPS (Global Positioning System) daemon"
 NAME=gpsd
-DAEMON=/usr/sbin/$NAME
+DAEMON=@SBINDIR@/$NAME
 PIDFILE=@RUNDIR@/$NAME.pid
 SCRIPTNAME=/etc/init.d/$NAME

But I do not think it is worth doing as distros have paths set in stone.
Bernd?

> I'm scared of touching SConstruct this close to a release, too.

This is not anything important and can wait easily. I'm also using
my own service files ;-)

Best regards,
        ladis