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

Include a systemd.service file #240

Merged
merged 11 commits into from
Mar 14, 2020
Merged

Include a systemd.service file #240

merged 11 commits into from
Mar 14, 2020

Conversation

WhyNotHugo
Copy link
Contributor

Including (and installing) these changes triggers calls to org.freedesktop.Notifications to start a systemd user service with mako.

This is pretty much how similar daemons (eg: dunst) behave. It allows the service to be auto-started on demand, and also ties it to the graphical session.

This also makes is trivial to restart mako like any other user service, by just running systemd --user restart mako.

Users not running systemd (if that's supported) shouldn't be affected.

Including (and installing) these changes triggers calls to
`org.freedesktop.Notifications` to start a systemd user service with
mako.

This is pretty much how similar daemons (eg: dunst) behave. It allows
the service to be auto-started on demand, and also ties it to the
graphical session.

This also makes is trivial to restart `mako` like any other user service,
by just running `systemd --user restart mako`.

Users not running systemd (if that's supported) shouldn't be affected.
mako.systemd.service.in Outdated Show resolved Hide resolved
mako.systemd.service.in Outdated Show resolved Hide resolved
mako.systemd.service.in Outdated Show resolved Hide resolved
@emersion
Copy link
Owner

Have you seen #188?

@WhyNotHugo
Copy link
Contributor Author

@emersion Nope 🤦‍♂️

@WhyNotHugo
Copy link
Contributor Author

This PR does cover your comment regarding extra steps when setting up.
Though the build system on the other one is better. I think mixing both's content is probably best.

@emersion
Copy link
Owner

Maybe we could use ConditionPathExists=${XDG_RUNTIME_DIR}/${WAYLAND_DISPLAY}? Does this correctly fail when WAYLAND_DISPLAY is unset?

@WhyNotHugo
Copy link
Contributor Author

WhyNotHugo commented Mar 10, 2020 via email

@emersion
Copy link
Owner

So systemd doesn't check for Condition* when starting a service by dbus activation?

@WhyNotHugo
Copy link
Contributor Author

It doesn't look like variable expansion works on Condition*. Some preliminary testing seems to indicate that systemd doesn't support this.

@emersion
Copy link
Owner

systemd/systemd#15067

@WhyNotHugo
Copy link
Contributor Author

In any case, this Condition* doesn't really block this PR.

  • Without it, the service is started upon DBus activation and fails.
  • With it, the service is skipped upon DBus activation and does not start.

The end result is still the same, and the difference is just under the hood (a logged service startup failure).

Hugo Osvaldo Barrera added 2 commits March 10, 2020 17:46
mako.systemd.service.in Outdated Show resolved Hide resolved
@emersion
Copy link
Owner

@xdbob Thoughts on ExecCondition?

mako.systemd.service.in Outdated Show resolved Hide resolved
@emersion
Copy link
Owner

LGTM otherwise.

@emersion
Copy link
Owner

One last thing: can we move this service file to contrib/, since I don't want to support any service manager in particular?

I'll merge once @xdbob gives positive feedback. :)

@xdbob
Copy link

xdbob commented Mar 10, 2020

I'll merge once @xdbob gives positive feedback. :)

I'll do some tests and think this through, give me a few days :)
You may also want @gdamjan opinion about these changes

@emersion
Copy link
Owner

No problem, we aren't in a hurry.

Co-Authored-By: Дамјан Георгиевски <gdamjan@gmail.com>
Copy link

@xdbob xdbob left a comment

Choose a reason for hiding this comment

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

I still feel like using ExecCondition is a hack (at best a failsafe) but I don't have anything better, yet.

LGTM (but please squash the commits :) )

Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Alright, let's merge this. Thanks everyone!

@emersion emersion merged commit 16fbced into emersion:master Mar 14, 2020
@emersion
Copy link
Owner

I still feel like using ExecCondition is a hack (at best a failsafe) but I don't have anything better, yet.

You could open a systemd issue to improve this (ConditionEnv? or ConditionSessionType? or something else?).

@WhyNotHugo WhyNotHugo deleted the systemd-dbus-activation branch March 14, 2020 09:30
@WhyNotHugo
Copy link
Contributor Author

Thanks for all your patience! :)

@gdamjan
Copy link
Contributor

gdamjan commented Mar 14, 2020

You could open a systemd issue to improve this (ConditionEnv? or ConditionSessionType? or something else?).

ConditionSessionType seems too specific. At this point introducing wayland-session.target in upstream systemd might be the easiest thing. OTOH, that could be added to {lib}wayland or some similar package too.

@xdbob
Copy link

xdbob commented Mar 14, 2020

ConditionSessionType seems too specific. At this point introducing wayland-session.target in upstream systemd might be the easiest thing. OTOH, that could be added to {lib}wayland or some similar package too.

Yes, I will ask to {lib,}wayland + Xorg if systemd don't want to include the targets

@@ -6,7 +6,7 @@ PartOf=graphical-session.target
[Service]
Type=dbus
BusName=org.freedesktop.Notifications
ExecCondition=/bin/sh -c '[ "$XDG_SESSION_TYPE" == "wayland" ]'
ExecCondition=/bin/sh -c '[ -z $WAYLAND_DISPLAY ] && exit 1 || exit 0'
Copy link
Contributor

Choose a reason for hiding this comment

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

A cleaner way do this in systemd v246 and later is to use ConditionEnvironment=WAYLAND_DISPLAY instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Care to make a PR with this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. #319

ammgws added a commit to ammgws/dotfiles that referenced this pull request Apr 22, 2021
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.

5 participants