Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: add LIBDIR to SYSTEMD_SERVICE_DIR config #2304

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

mattmundell
Copy link
Contributor

What

Prefix the SYSTEMD_SERVICE_DIR config path with LIBDIR.

Why

The default install was always putting the systemd files in /lib.

For one this can cause permission problems, for example if you're installing to your home dir.

Note that it's still possible to specify an exact location by passing -DSYSTEMD_SERVICE_DIR to cmake.

Verify

On main, before the PR:

#### 1 plain
$ cmake MANY_ARGS && make install
-- Installing: /lib/systemd/system/gvmd.service
CMake Error at config/cmake_install.cmake:54 (file):
  file INSTALL cannot copy file
  "/home/matt/fresh/main/build/gvmd/config/gvmd.service" to
  "/lib/systemd/system/gvmd.service": Permission denied.
Call Stack (most recent call first):
  cmake_install.cmake:424 (include)
make: *** [Makefile:130: install] Error 1

#### 2 specify LIBDIR
$ cmake -DLIBDIR=/tmp/ MANY_ARGS && make install
-- Installing: /lib/systemd/system/gvmd.service
CMake Error at config/cmake_install.cmake:54 (file):
  file INSTALL cannot copy file
  "/home/matt/fresh/main/build/gvmd/config/gvmd.service" to
  "/lib/systemd/system/gvmd.service": Permission denied.
Call Stack (most recent call first):
  cmake_install.cmake:424 (include)
make: *** [Makefile:130: install] Error 1

With the PR:

#### plain
$ cmake MANY_ARGS && make install
-- Installing: /home/matt/alts/main/lib/systemd/system/gvmd.service
Compilation finished at Tue Oct  8 11:12:02

#### specify LIBDIR
$ cmake -DLIBDIR=/tmp/ MANY_ARGS && make install
-- Installing: /tmp/systemd/system/gvmd.service
Compilation finished at Tue Oct  8 11:12:57

References

Closes #2296.

@mattmundell mattmundell requested a review from a team as a code owner October 8, 2024 09:25
Copy link

github-actions bot commented Oct 8, 2024

Conventional Commits Report

Type Number
Bug Fixes 1

🚀 Conventional commits found.

This comment was marked as spam.

@cfi-gb
Copy link
Member

cfi-gb commented Oct 8, 2024

Only a remark that it AFAICT was the purpose to always place the file to /lib/systemd/system/gvmd.service independent from any configuration option like e.g. LIBDIR according to #1662. IIRC the rationale given in the past was that it doesn't make any sense to place a systemd file to e.g. /home/matt/alts/main/lib/systemd/system/gvmd.service.

@mattmundell
Copy link
Contributor Author

@cfi-gb I see. That explains why my cmake command has built up so many flags over the years. This seems wrong to me because:

  1. I should not have to pass -DSYSTEMD_SERVICE_DIR=$INSTALL_PREFIX/lib/systemd/system to avoid permission errors when installing to my home dir.
  2. Similarly, if I use root to install a second version to an arbitrary location it should not overwrite the /lib/systemd files from my main install.
  3. According to the manpage it makes sense to place systemd files in /usr/local/lib/systemd/system/gvmd.service, which is not happening when I install to /usr/local.

@cfi-gb
Copy link
Member

cfi-gb commented Oct 8, 2024

Just to note that i'm not against this change, just wanted to mention why this might have been done like that in the past

Especially as e.g. gsad is also using the previous defaults since greenbone/gsa#3045 and changing only gvmd might be even more confusing.

@mattmundell
Copy link
Contributor Author

OK, maybe needs a broader approach, I'll look again next week.

@Narrat
Copy link

Narrat commented Oct 8, 2024

What may play a role for hardcoding /lib in 2021. The usr-merge wasn't covered by all major players like it is now. IIRC Debian had still a separate /lib back then and it would be the location for systemd. Other distros were earlier and for those /lib/ is only a symlink to /usr/lib/ and files shouldn't be installed into /lib anymore. Some package managers may even refuse to install such a package (contains a directory for a location which is only a symlink).
From experience as someone who dabs into maintaing recipes to create packages for package manager it is fairly normal, that specifying LIBDIR places the files relative to that position even if those are for another piece of software. The commit in question even allows to modify /etc to whatever, which may make sense or not as a location.
Imo if someone wants to manually create a systemd-nspawn container or the like, all files should be placeable where one wants to place those.

I also noticed that the same happens for another package, but I only opened it for this package to see what happens. If the reaction was positive a follow up issue would have been made. But that seems obsolete now?

@bjoernricks
Copy link
Contributor

Hey, my intentions with #1662 were to use sane defaults that work out of the box without adjusting any paths even when using the default prefix /usr/local. The standard setting should just work without having to set a lot of variables or to tweak system configurations. IMHO this behavior should be kept. Of course paths should be adjusted if they aren't valid anymore like if /lib/systemd is /usr/lib/systemd nowadays. I am also fine with making paths adjustable and more flexible by introducing new CMake configuration variables.

@mattmundell
Copy link
Contributor Author

Thanks for the context @bjoernricks and @Narrat.

From what I can see, the standard settings just work, but only if you install as root, and only if you install to one place.

It's great that the standard settings are sane. Can we also have sane behaviour when I install to my home dir? For example, when INSTALL_PREFIX is $HOME/alts/main then SYSCONFDIR is being set to /etc. This to me is very poor behaviour. It will try to install somewhere that needs root, and it will overwrite any version that I've installed to /.

Maybe there's some conflict of requirements here because when installing to /usr/local the standard setting is to install to /etc. What is the reason for this? Why does a /usr/local install need to put config files like gvm/gvmd_log.conf in /etc? Does this make it easier for people following docs?

@Narrat
Copy link

Narrat commented Oct 23, 2024

/etc is kinda special as it is the location for system-wide configuration. There is not really a different location (ignoring the oddball location /opt which can contain /opt/etc, IIRC. But /usr/local/etc is as far as I know not a thing.
But a program should not expect or need /etc. And with the rise of immutable distributions there may be further changes in the future (see for example this issue)
So in general not a problem if the configuration cannot be placed at the default location? As the program can be supplied with a path to a config?

And the difference between /usr/local/{bin,lib,(...)} and /usr/{bin,lib,(...)} stems nowadays mostly from the fact, that the latter is managed by the distribution specific package manager and no one should manually install files into that location. But /etc is kinda ignored and is "valid" for managed and unmanaged installations. With possible side-effects.
With that in mind I did look up again what locations systemd searches for unit files. And the hierarchy includes /usr/local/lib/systemd/.
https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html#System%20Unit%20Search%20Path

System Unit Search Path

/etc/systemd/system.control/*
--
/run/systemd/system.control/*
/run/systemd/transient/*
/run/systemd/generator.early/*
/etc/systemd/system/*
/etc/systemd/system.attached/*
/run/systemd/system/*
/run/systemd/system.attached/*
/run/systemd/generator/*
…
/usr/local/lib/systemd/system/*  <---
/usr/lib/systemd/system/*
/run/systemd/generator.late/*

So it isn't necessary to hardcode /lib or update it to just /usr/lib. And from the viewpoint of distributions it shouldn't potentially place files in a managed area on a manual installation.

@timopollmeier timopollmeier merged commit 21dec6c into main Dec 2, 2024
14 checks passed
@timopollmeier timopollmeier deleted the cmake-systemd-libdir branch December 2, 2024 15:10
@mattmundell
Copy link
Contributor Author

Thanks again for the context, @Narrat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config/CMakeLists.txt: SYSTEMD_SYSTEM_DIR should respect LIBDIR
5 participants