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

[Greg Troxel]

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.

> 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.

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.

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

> 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.

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.


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