Add configurable path variables for services #463
Conversation
bebehei
force-pushed
the
serviceprefix
branch
from
8cfab11 to
fc94f31
Compare
|
Shitty Ubuntu 14.04. I tested it in a container and I still don't know what the Problem is. It seems, that the version of make doesn't support the Shellstatus variable as expected. |
|
Finally got this fixed. Turns out Ubuntu 14 has the old GNU make major version. When reading the docs, it turns out that the |
bebehei
force-pushed
the
serviceprefix
branch
4 times, most recently
from
ba19c7e to
976f229
Compare
| VERSION := $(shell git describe --tags) | ||
| endif | ||
|
|
||
| SERVICEDIR_DBUS ?= $(shell pkg-config dbus-1 --variable=session_bus_services_dir) |
There was a problem hiding this comment.
I'm not sure about this one, dbus dirs are standardized in dbus-daemon(1) and using this ensures that any custom prefix (e.g. ~/.local/share) won't get the dbus service installed.
There was a problem hiding this comment.
You can set SERVICEDIR_DBUS by yourself and and it won't get overwritten then.
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
Another thing missing is some more documentation, if we're going to use some obscure variable that's not even obvious from the Makefile what affects it we should have some good documentation to back that up for confused users.
| mkdir -p ${DESTDIR}${PREFIX}/share/dbus-1/services/ | ||
| install -m644 org.knopwob.dunst.service ${DESTDIR}${PREFIX}/share/dbus-1/services | ||
| install -Dm644 dunst.systemd.service ${DESTDIR}${PREFIX}/lib/systemd/user/dunst.service | ||
| install -Dm644 org.knopwob.dunst.service ${DESTDIR}${SERVICEDIR_DBUS}/org.knopwob.dunst.service |
There was a problem hiding this comment.
It's a fatal error if one of these install calls fail, the error condition should be handled both here and in install-systemd and also in uninstall-{dbus,systemd} since it currently stops in the middle of the installation (or uninstallation).
Edit: Furthermore there should be a more user-friendly error message than a big "Permission Denied" error IMO. Especially for the systemd service which is not even necessary to begin with.
PS: Why is the install target called install-service but uninstall uninstall-dbus? weird.
There was a problem hiding this comment.
Naming issue: true, ack.
Permission issue: what!? It's common to start and not check. However it's handled as fatal (like every command in a Makefile).
It's not the makefile's job to check for succession before installation. Also it's (that'll sound hard) not our job to make error messages user friendly. Anyone who's capable to execute make should understand what a permission denied error is.
There was a problem hiding this comment.
Consider this scenario:
$ PREFIX=install/ SERVICEDIR_DBUS=install/dbus make install
install -Dm755 dunst install//bin/dunst
install -Dm644 docs/dunst.1 install//share/man/man1/dunst.1
install -Dm644 dunstrc install//share/dunst/dunstrc
install -Dm644 org.knopwob.dunst.service install/dbus/org.knopwob.dunst.service
install -Dm644 dunst.systemd.service /usr/lib/systemd/user/dunst.service
install: cannot remove '/usr/lib/systemd/user/dunst.service': Permission denied
Makefile:136: recipe for target 'install-service-systemd' failed
make: *** [install-service-systemd] Error 1
The fact that this means that the installation was successful but couldn't install a very optional part is weird right off the bat.
(Edit: And would probably trip many auto-builders that there is something very wrong if they trip on this for some reason.)
$ find install/
install/
install/bin
install/bin/dunst
install/share
install/share/man
install/share/man/man1
install/share/man/man1/dunst.1
install/share/dunst
install/share/dunst/dunstrc
install/dbus
install/dbus/org.knopwob.dunst.service
All normal here
$ PREFIX=install/ SERVICEDIR_DBUS=install/dbus make uninstall
rm -f install/dbus/org.knopwob.dunst.service
rm -f /usr/lib/systemd/user/dunst.service
rm: cannot remove '/usr/lib/systemd/user/dunst.service': Permission denied
Makefile:150: recipe for target 'uninstall-systemd' failed
make: *** [uninstall-systemd] Error 1
Looks like something didn't run here and indeed uninstallation finished halfway through
$ find install/
install/
install/bin
install/bin/dunst
install/share
install/share/man
install/share/man/man1
install/share/man/man1/dunst.1
install/share/dunst
install/share/dunst/dunstrc
install/dbus
The only reason the install command worked correctly is because install-systemd happened to be the last target to be run it would have exited and might've skipped some steps otherwise.
It's common not to check permissions when we would fail anyway if we can't install that specific part but it's not what's happening here, this is completely optional and it's making it look like something fatal happened.
There was a problem hiding this comment.
Sorry, but I have to fully disagree with you. Let me tell you, why:
Let's make an axiom test first.
- (1) In an unparametrized make call: systemd available <=> systemd service file installed,
- (2) If you have systemd installed, you want to get valuable information from
systemctl status --user dunst.service
Please validate all your statements of your previous comment against those two axioms.
There was a problem hiding this comment.
I was going to argue the other way but then I realised it's the exact same situation with the dbus service which is pretty crucial. So I give it a pass here.
What I still don't like though is the uninstallation failing mid-way, the same way we use -f to ignore errors on non-existent files we should check and only issue a big warning if there is a permission issue with one of the files not stop completely and leave junk files behind.
There was a problem hiding this comment.
What I still don't like though is the uninstallation failing mid-way,
Err... Failing the installation midway implies, that the installation did not succeed. So, you can't uninstall dunst successfully if your installation failed.
Packing all uninstall targets into a single one and in combination with some shell foo, this can get solved. This ignores all make principles, but it's the only possible solution. I've pushed a commit to this branch now. Let's discuss it.
|
About that docs thing: I thought about this, too. Undocumented vars won't be ever good. Where should I put this? Can you tell me a reasonable place? |
|
Best place I can think of is the only one we have installation instructions, aka the wiki but since with this PR local installation becomes a bit more complex I'd also add a link to the README pointing that way. |
bebehei
force-pushed
the
serviceprefix
branch
2 times, most recently
from
a9dc325 to
711864e
Compare
Setting PREFIX to a location different to /usr, the install routine fails to install the systemd and dbus service files. These are installed, but in the PREFIX directory and not /usr. DBus and systemd usually only read their files from /usr/ and ignore files in /usr/local. Now by default, we're asking pkg-config, where to install it. Mostly, this will be /usr and this conflicts the FHS. But it's the user's intent to install dunst and (possibly) override the package manager's files belonging to dunst. At the current point, even DBus ignores the PREFIX and installs its systemd service file to the location specified by pkg-config.
bebehei
force-pushed
the
serviceprefix
branch
from
711864e to
e10391c
Compare
Dunst does not neccessarily need systemd. Dunst gets started primarily via DBus. The systemd service is useful on systemd init based systems, but won't have any impact on non-systemd systems. To make systemd a soft dependency is neccessary, as pkg-config now also queries the systemd.pc file, which won't exist on non systemd systems.
bebehei
force-pushed
the
serviceprefix
branch
from
e10391c to
85da5d7
Compare
Makefile
Outdated
| rm -f ${DESTDIR}${MANPREFIX}/man1/dunst.1 || ec=1; \ | ||
| rm -rf ${DESTDIR}${PREFIX}/share/dunst || ec=1; \ | ||
| rm -f ${DESTDIR}${SERVICEDIR_DBUS}/org.knopwob.dunst.service || ec=1; \ | ||
| if [ "0" != "${SYSTEMD}" ]; then rm -f ${DESTDIR}${SERVICEDIR_SYSTEMD}/dunst.service || ec=1; fi; \ |
There was a problem hiding this comment.
That wasn't exactly what I meant, more like a "|| echo "WARNING: Failed to remove the {"systemd service file", "dbus service file"}"
|
So, ok. Let's please remind, how manual installation and removal of software works. I'll come back later on this systemd/dbus stuff. The software gets configured with To remove the software again, you have to either:
In any case, a successful removal can only be assured, when the last configure call in the compilation directory matches the configure call of the installation. And here, in dunst's case, this is the same. The advantage of GNU autotools is, that you do not have to actively remember the configure parameters for calling When we install dunst we have to give the same parameters for Back on the systemd file removal issue: At this point now, given that make wants to remove the file when the So, the fact, that It makes no sense, treating some failed removals as fatal and some others not. If dunst fails to remove this file, it fails to remove this file, because you either did not specify the params correctly or have forgot sudo. |
|
I agree with what you said but there's just one point demonstrated by the example above:
Or option 2 is that an installation was attempted (but failed because the user forgot to set the right variables, usual mistake given they aren't that common) and now they can't remove the already installed files because uninstallation tries to remove the systemd stuff first (and fails). Sure, you can just sudo it or dig the Makefile to look for everything that's being installed and remove it manually but that misses the point of the uninstall target. EDIT: I guess I agree that it doesn't make much sense to treat some files special like this but I feel it's justified for these 2 since a) they are controlled by some pretty obscure variables and b) there is no easy to use configure script for dunst so everything's in environment variables. |
tsipinakis
force-pushed
the
serviceprefix
branch
from
dad9189 to
85da5d7
Compare
|
We've already delayed the release of 1.3 by a lot and I've been arguing about a very rare scenario for a few days now so I'm approving this (I've removed the last experimental commit you pushed in response to my argument). I also realised a lot of this could also easily be resolved with some better documentation so I'd also make it a goal to add some more detailed installation instructions to the repo, maybe in I've approved this but I'm leaving the merge up to you since I did remove a commit without your approval. |
Add configurable path variables for services
3 participants
Setting PREFIX to a location different to /usr, the install routine
fails to install the systemd and dbus service files. These are
installed, but in the PREFIX directory and not /usr. DBus and systemd
usually only read their files from /usr/ and ignore files in /usr/local.
Now by default, we're asking pkg-config, where to install it. Mostly,
this will be /usr and this conflicts the FHS. But it's the user's intent
to install dunst and (possibly) override the package manager's files
belonging to dunst.
At the current point, even DBus ignores the PREFIX and installs its
systemd service file to the location specified by pkg-config.