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

Add systemd service file #633

Merged

Conversation

DimitriPapadopoulos
Copy link
Collaborator

@DimitriPapadopoulos DimitriPapadopoulos commented Apr 8, 2020

Fixes #623.

The configure.ac changes are documented in the systemd man page:
https://www.freedesktop.org/software/systemd/man/daemon.html#Installing%20systemd%20Service%20Files

Allows openfortivpn to act as system service.

There are a couple issues that bother me:

  • openfortivpn used to be entirely installed under ${prefix}, including the default configuration file under $(sysconfdir)/openfortivpn$(prefix)/etc/openfortivpn. With this change openfortivpn installs its systemd service file outside $(prefix), under $(systemdsystemunitdir)/lib/systemd/system/. That's by design of course but I thought I'd mention it.
  • I'm not certain I have got the Makefile.am right, on the contrary. My concern is conditionals in the Makefile - with/without systemd service files. The Makefile should function properly whatever the conditions. It seems to be working but I'm not convinced. Some help appreciated here.

Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Dimitri,

I would tend to not package the systemd service file here:

  1. Although I'm a big supporter of systemd, not all GNU/Linux distributions use it. If we add it to solve Systemd service file #623, I hope users won't ask files for sysvinit, OpenRC, supervisord, etc.
  2. I won't have time to maintain it if users experience problems on fancy distros.

But since you've already done it why not!

Copy link
Collaborator

@mrbaseman mrbaseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found a hard coded path in lib/systemd/system/[email protected]. I would recommend to fix this.

Apart from that I'm not sure if the @ in the file name could cause problems with git, with the file system, with packaging tools, with web tools that show the source code, automake that processes it (or the path of the input file)... Perhaps I'm too skeptic in this regard...

Anyhow, I'm not very familiar with systemd. I see on my Ubuntu that only a few services follow this convention, whereas most of them have file names consisting of alphanumeric characters. Does it have a special meaning?

Perhaps the source can be named lib/systemd/system/openfortivpn.service.in and we install to the final destination.

Also are we sure that we want to install into the systemd directory opposed to the strategy of providing a sample file like the ppp scripts that we have included but don't install and the config file template that we install to the share directory?

lib/systemd/system/[email protected] Outdated Show resolved Hide resolved
if ! test -f $(DESTDIR)$(confdir)/config ; then \
$(MKDIR_P) $(DESTDIR)$(confdir) ; \
$(INSTALL) -m 600 etc/openfortivpn/config \
$(DESTDIR)$(confdir)/config ; \
fi
if HAVE_SYSTEMD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware that such an if construct works in the .am file, but if it works, why not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've seen it used in other Makefiles, not certain how standard this is. If you have alternatives I'm interested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have used AC_SUBST in configure.ac and compare it as a string with test inside Makefile.am - but I'm fine with the if HAVE_SYSTEMD. It's more elegant, I just didn't know about this possibility.

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Apr 15, 2020

By curiosity, have you compared with other C programs that create systemd service files? For example here is what they do for sshd systemd service:
https://src.fedoraproject.org/rpms/openssh/blob/4e7cdec/f/openssh-7.4p1-systemd.patch

That's a good idea. I had looked into this rapidly. Perhaps I can have a look again.

However my concern is that there are multiple ways to "support systemd":

  • Link with systemd libraries: we have been doing that to notify with sd_notify(). That's what the OpenSSH example above does. Classic stuff.
  • Install system level-files, typically outside $(prefix). I find this more intrusive.

The main concern is that I am using HAVE_SYSTEM_D for both:

  • Indeed HAVE_SYSTEM_D means systemd headers and libraries are available and that we can compile and link against them.
  • However I am also using it to decide whether we should install the systemd service file. In theory we may well install this file under /lib/systemd/system/ even if systemd headers and development libraries were not available at build-time - but then HAVE_SYSTEM_D is not true. On the other hand HAVE_SYSTEM_D might indicate we can compile and link against systemd, but we might want to avoid installing the systemd service file.

Perhaps as a first step we can skip these subtleties. If using a single conditional for both goals later appears to be a problem later on, I will fix that.

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Apr 15, 2020

Apart from that I'm not sure if the @ in the file name could cause problems with git, with the file system, with packaging tools, with web tools that show the source code, automake that processes it (or the path of the input file)... Perhaps I'm too skeptic in this regard...

It doesn't appear to be an issue. Matlab also has files with @and I can recall it used be a nuisance at some point (maybe with Subversion?) but in this context it seems OK. Besides the @ is expected in the name of the final file so we don't really have a choice.

Anyhow, I'm not very familiar with systemd. I see on my Ubuntu that only a few services follow this convention, whereas most of them have file names consisting of alphanumeric characters. Does it have a special meaning?

Yes, the @is a way to instantiate multiple services from a single template configuration file. You could have two configuration files:

$(sysconfdir)/etc/openfortivpn/TEST.conf
$(sysconfdir)/etc/openfortivpn/PROD.conf

and use them in distinct services [email protected] and [email protected].

Here is short and well-written introduction to systemd service files
Systemd: Service File Examples

Perhaps the source can be named lib/systemd/system/openfortivpn.service.in and we install to the final destination.

If it works as is with the @ I'd rather keep it to make it clear it's a template file.

Also are we sure that we want to install into the systemd directory opposed to the strategy of providing a sample file like the ppp scripts that we have included but don't install and the config file template that we install to the share directory?

I don't know. It will mean more work for packagers to patch the Makefile to add the service file in packages. What if I investigate what other software do? Let me see how they handle this.

Copy link
Collaborator

@mrbaseman mrbaseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, @BINDIR@ is the correct solution

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the systemd.service branch 3 times, most recently from a2d466f to dab3bd1 Compare April 16, 2020 11:04
@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Apr 16, 2020

The case of util-linux is interesting:

I will try to improve the Makefile based on this.

Allows openfortivpn to act as system service.
The configuration is expected under $(sysconfdir)/openfortivpn/<NAME>.conf.

https://www.freedesktop.org/software/systemd/man/daemon.html#Installing%20systemd%20Service%20Files
@DimitriPapadopoulos DimitriPapadopoulos merged commit 8ea4d6c into adrienverge:master Apr 16, 2020
@DimitriPapadopoulos DimitriPapadopoulos deleted the systemd.service branch April 16, 2020 14:47
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.

Systemd service file
3 participants