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 sway-session.target #5622

Closed
wants to merge 2 commits into from

Conversation

maximbaz
Copy link

In order to keep the discussion focused: this PR is NOT about running sway as a systemd service 🙂

sway can run on many OS, some of which use systemd for service management, like Arch Linux.

On those OS it is needed to organize dependencies and startup order, for example some services should only start in a graphical session, but not in tty, and should stop when graphical session ends.

For this particular task, systemd itself created a standard graphical-session.target, which as per their description acts as a dynamic alias for any concrete graphical user session (so a service that must only run in a graphical session would declare a dependency on this virtual target).

According to this design, graphical-session.target itself should not and cannot be started directly, instead every WM should provide its own .target that "implements" this interface.

sway wiki contains the definition of sway-session.target and instructions for users on where to put it, however given that systemd is popular, I propose to include sway-session.target directly in the repo. This would help consolidating the effort and share best-practices between people who run OS with systemd and who compile sway (for themselves or especially as part of creating sway distro packages), as they would additionally copy this .target (the same one!) to a proper location as part of sway package, instead of users having to do so.

  • Having this .target present in sway repo has nothing to do with how sway itself is being launched.
  • Having this .target installed on an end-user's computer does not change any behavior in itself, users still need to start and stop it themselves.
    • Although this can be further assisted by distro packagers to create a config file that users can just source in their own sway config if they want to and get proper systemd integration that works on their distro, here is an example from Arch package, I will add systemctl --user start sway-session.target to that file.

Looking forward to your feedback 🙂

@WhyNotHugo
Copy link
Contributor

I'd be happy to see this merged in. It's good for sway as an upstream to provide a standard target, so all other tools, tutorials, etc, can point to a single, same, one, rather than implementing their own with slightly-incompatible differences.

@emersion
Copy link
Member

Thanks for the explanation.

What's the end goal with this target?

  • Let external clients ship systemd units started with Sway? But most of these clients aren't Sway-specific, they work in other Wayland compositors too.
  • Let users specify services to start with Sway? But why can't they just use graphical-session.target?
  • Something else?

Do you have examples of other compositors shipping a systemd target file?

@maximbaz
Copy link
Author

The goal is to let users start systemd services after any graphical session is started, sway being just one example.

All other services are expected to depend on graphical-session.target, not on sway-session.target, mako.service is a good example on how to do it.

The reason for this PR is that systemctl --user start graphical-session.target is an invalid command, because it's like a "virtual dependency", by design all compositors must provide their own *-session.target which "implements" this virtual interface.

So, users will run systemctl --user start sway-session.target, this will in turn start graphical-session.target, and all other services that depend on graphical-session.target will then start.

Does this clarify the workflow? 🙂

Do you have examples of other compositors shipping a systemd target file?

I haven't looked in detail, for example GNOME ships a very sophisticated list of targets, we don't really need that complexity.

@craigbarnes
Copy link

craigbarnes commented Aug 21, 2020

There's a good description of graphical-session.target and graphical-session-pre.target in the systemd.special man page. Perhaps it'd be useful to add the following line to the patch:

Documentation=man:systemd.special(7)

@maximbaz
Copy link
Author

maximbaz commented Aug 21, 2020

I don't have a strong preference but I thought if we add Documentation it should refer to sway documentation, not systemd... Also neither the example in systemd.special nor gnome-session targets link to systemd documentation 🤔

By the way graphical-session.target itself does already link to systemd.special manpage 👍

@rpigott
Copy link
Member

rpigott commented Sep 30, 2020

if we add Documentation it should refer to sway documentation, not systemd

The Documentation directive is a list. Many units have several man pages, hyperlinks etc. listed, and in this case we can too. I don't see any reason not to direct users to all the relevant documentation if there are several relevant pages. Especially since more users seem to be confused on the purpose of the target than sway's behavior...

The goal is to let users start systemd services after any graphical session is started, sway being just one example.

That's not the real utility of graphical.target. The systemd.special(7) man page describes the purpose of graphical.target like so:

graphical-session.target
This target is active whenever any graphical session is running. It is used to stop user services which only apply to a graphical (X, Wayland, etc.) session when the session is terminated.

Which is a reasonable function, I think. Tray icon applets and notifications daemons are probably the intended users of this. The man page gives gnome-settings-daemon as its motivating example. Basically any program related to the graphical session which might not be prompted to close by losing connection to a Wayland display. This behavior is why the man page recommends units specify PartOf=graphical-session.target.

Autostarting apps is simple enough without a target. systemd even provides systemd-xdg-autostart-generator for a method based on XDG autostart. But I think the intended purpose described by the man page is the more novel and useful behavior.

@markstos
Copy link
Contributor

Merging this PR would help other packagers to rely on sway-session.target existing with a standard name if sway is installed.

It would be a bit of a mess now if multiple sway-specific background services wanted to depend on a sway target, as they would all end up shipping sway-session.target in their own packages, causing conflicts.

Copy link
Contributor

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

Really looking forward to seeing this merged.

It'll make it easier for applications to include a systemd.service unit file that depends on a standardised target.

@emersion
Copy link
Member

emersion commented Dec 8, 2020

TBH I'd rather let systemd distributions add the target themselves. If they're going to add one line to their distro-specific config file, having to add a 3-line file isn't a big deal.

@markstos
Copy link
Contributor

markstos commented Dec 8, 2020

@emersion the challenge is there's a whole ecosystem of related projects.

In the world where all systemd distributions who wanted this added exactly the same file with exactly the name sway-session.target, this would be OK. Putting this on distros opens the door that some distributions ship sway.target and some ship sway-session.target. That doesn't really help related projects who are looking for a stable target name to depend on.

The option proposed here is to ship the systemd file in the core project, providing a canonical target name for every distro to use, eliminating the risk of differences and the need to coordinate between distros for this detail.

@emersion
Copy link
Member

emersion commented Dec 8, 2020

Services should be started on graphical-session.target, not sway-session.target. In any case, contrib/ is just community-contributed content, distros can do their own thing anyways and that would be perfectly fine.

@markstos
Copy link
Contributor

markstos commented Dec 8, 2020

@emersion Thanks for the reply.

Are there specific questions left to discuss before a merge/no-merge decision is made?

@WhyNotHugo
Copy link
Contributor

In the world where all systemd distributions who wanted this added exactly the same file with exactly the name sway-session.target, this would be OK. Putting this on distros opens the door that some distributions ship sway.target and some ship sway-session.target. That doesn't really help related projects who are looking for a stable target name to depend on.

It's undeniable that it'll require downstream changes by some. Right now, but sway.target and sway-session.target seem to be used, both by applications, and by published / shared dotfiles and helper packages.

sway-session.target matches the naming convention of other compositors' target, and of systemd, so I'd prefer that

Services should be started on graphical-session.target, not sway-session.target. In any case, contrib/ is just community-contributed content, distros can do their own thing anyways and that would be perfectly fine.

Some services are sway-specific and this target is for those.

Co-authored-by: Hugo Barrera <hugo@barrera.io>
@alebastr
Copy link
Contributor

For Fedora we needed a bit more than just a session target, so I extracted all my relevant configs and scripts into alebastr/sway-systemd and currently working on adding that as a recommended dependency for our sway package.
The scope is very narrow, just to solve a common set of issues with systemd user session and other components (oomd). Nothing as fancy as sway-services.

If you think your distribution can leverage that, feel free to take the configs, provide feedback or contribute. I certainly wouldn't mind making that a cross-distribution effort.

@markstos
Copy link
Contributor

@alebastr I'm bookmarking the project and may try it on Arch Linux. I'll provide feedback through the sway-systemd project once I test it out.

I reviewed the sway-services project and found it a more complex and under-documented then what I was hoping for. The smaller scope of your take on "sway + systemd" is appealing.

@WhyNotHugo
Copy link
Contributor

Is there anything blocking this from being merged?

I realise it's not critical, but it's definitely a nice-to-have, and I can't see if having any negative side-effects.

@emersion emersion added the enhancement New feature or incremental improvement label Dec 21, 2021
@LaserEyess
Copy link

LaserEyess commented Apr 25, 2023

This is now 2.5 years old, and I see there has been very little movement on it. From my perspective, this is a fix that will benefit many people using sway while giving almost 0 burden to sway maintainers. Any arguments about "distros should handle this" are, quite frankly, delusional wishful thinking at best. (No offense meant to sway developers here, but distros coming together is lol). Distros should not handle this, this should be standardized upstream with the specific purpose of 3rd party libraries being able to rely on the target existing.

If there is no standardization then 3rd party sway services have absolutely no way of knowing how to start after sway has started. Each distro can be free to choose sway-session.target, sway.target, sway-graphical-session.target or any manner of incompatible targets that downstream users must figure out per distro to add services to. That is either a) a lot of patching and configuration for individual users, or b) a lot of patching at the distro level to get this to work. If a target exists upstream in sway that means 3rd party libraries can use it, and distros and users don't need to do any work.

The only downsides I've heard from sway developers fall into two arguments:

  1. "We don't want to maintain this"
  2. "We don't support any particular init system"

I would argue that a 6 line INI file that is not connected to any particular API is not a maintenance burden. I would also argue that explicitly not supporting "an init system" is the same thing as choosing a side.

Please consider merging this.

@kennylevinsen
Copy link
Member

Any arguments about "distros should handle this" are, quite frankly, delusional at best.

Calling maintainer arguments delusional is not a particularly productive way to lead a discussion. :)

sway-session.target acts solely as a trigger for graphical-session.target, which is a well-defined systemd entity. The ability to target sway-session.target from a service file is not the goal of this change, and it would be an error for a service definition to rely on a sway-specific target. There is therefore no reason for concern related to compatibility or ecosystem health, even if every user had their own variation of the target and name.

The only question is who owns the "6 line INI file": a service-manager agnostic project which has historically avoided this sort of thing, or distribution packages that normally contain whatever integrations are relevant to their distribution-specific design decisions.

@LaserEyess
Copy link

LaserEyess commented Apr 26, 2023

The ability to target sway-session.target from a service file is not the goal of this change, and it would be an error for a service definition to rely on a sway-specific target.

Incorrect, sway provides an IPC socket that services cannot know is ready solely based off of graphical-session.target. Again, this is not about the wayland session, you can use ConditionEnvironment=WAYLAND_DISPLAY for that.

a service-manager agnostic project which has historically avoided this sort of thing, or distribution packages that normally contain whatever integrations are relevant to their distribution-specific design decisions.

Service-manager agnostic is not the same thing as service manager antagonistic. Sway does not support login mangers but ships a sway.desktop file at the top level of this repository despite its only purpose being to enable usage by login managers. This is also a 6 line INI file that has effectively 0 maintenance burden for developers. What is the difference here?

@markstos
Copy link
Contributor

markstos commented Apr 26, 2023 via email

@kennylevinsen
Copy link
Member

Incorrect, sway provides an IPC socket that services cannot know is ready solely based off of graphical-session.target. Again, this is not about the wayland session, you can use ConditionEnvironment=WAYLAND_DISPLAY for that.

This is not the goal of this PR. The goal of this PR as per its description is to trigger graphical-session.target that other packages might be relying on, which for technical reasons require an intermediate target.

This original intent does have some merit, but I am not a fan of the alternate uses that have cropped up in the discussion.

Service-manager agnostic is not the same thing as service manager antagonistic. Sway does not support login mangers but ships a sway.desktop file at the top level of this repository despite its only purpose being to enable usage by login managers.

The .desktop file is a freedesktop specification supported by numerous login managers and platforms. While not pretty, it is not against any of our policies (not supporting login managers just mean that we will not debug GDM problems for you). The equivalent to a systemd service file would instead be if we carried a GDM-specific login manager configuration.

Regardless, trying to nitpick on our values and policies is also not a particularly productive way to discuss things. You are not trying to defeat a machine with errors in its logic, but are trying to convince human meatbags to change their ideologies and opinions. :)

I have services that I want to launch that are Sway-specific [..] They don't make sense in Gnome

This is a perfect counter-example for direct usage of a sway-specific target: Turning "not for gnome" into "strictly for sway" in a packaged service definition is unfriendly to users and the Wayland ecosystem as a whole, which may wish to use the service on a different, but otherwise compatible and sensible Wayland server.

Even sway IPC is not sway specific, as it is i3's IPC system with a few sway extensions. For the limited number of applications that rely on sway IPC, the recommended way to start these are with an exec in your sway configuration. There is no good way to have a target for things depending on sway/i3 style sockets specifically, but this is largely a non-issue as exec works just fine for these.

@LaserEyess
Copy link

Regardless, trying to nitpick on our values and policies is also not a particularly productive way to discuss things. You are not trying to defeat a machine with errors in its logic, but are trying to convince human meatbags to change their ideologies and opinions. :)

This is not about your values or opinions, this is about what currently exists in the repository. A desktop file was added and in ~8 years it has changed exactly once: to update a comment. It has not been an explicit endorsement of login managers nor explicit support. It does not require to sacrifice or betray any ideologies, and I'm sure the dev team does have some opinions about freedesktop.org. A sway-session.target file would be similar, it is simply a file that should exist upstream in order to facilitate downstream functionality in the vast majority of desktop-centric distros.

Can it be abused by 3rd party projects to erroneously depend on it for wayland sessions when they should be using graphical-session.target? Absolutely, but that's not sway's problem, it should be fixed in those 3rd party projects. But sway has its own IPC with extensions that do not exist in i3's IPC, not to mention things that rely on the combination of wlr-* protocols and sway IPC. I won't belabor the point since I think you do know that there are valid reasons for this beyond "I only want this to run when sway does just because".

@kennylevinsen
Copy link
Member

This is not about your values or opinions, this is about what currently exists in the repository.

Whether we merge this PR is purely a matter of our opinion of it, with the primary blocker being our opinion of hosting service manager specific - to some extend especially systemd-specific - files and features.

This kind of change has a default NACK policy due to our ideologies around favoritism and the resulting implicit lock-in that we do not think belongs in good FOSS. I am tempted to ACK it, but the recent arguments have only been around usages that I am not at all interested in providing new mechanisms for.

I am only interested in helping sway fit into a generic ecosystem of pre-existing graphical-session.target dependent services. If you have a service that needs to use sway IPC, just put it as an exec in your config.

@ony
Copy link

ony commented Apr 26, 2023

I think discussion is went beyond just justifying some standing point. I don't think stakes are so high here 😛.

If this PR is to reduce patches by distros which ideally should be zero, then it should also include target activation script.
Without that it is weird if target installed by Sway and activation of it comes from distro or by user.

Sway wiki for SystemD is nice. But in case if many distros and LFS users have to do it anyway. Why not just make it easier to use for majority cases?
If concern is "system" effect that keeps systemd at top. Why not add support for more init systems? It is not that far from providing completions for multiple shells.

P.S. Not a heavy systemd user.

Incorrect, sway provides an IPC socket that services cannot know is ready solely based off of graphical-session.target. Again, this is not about the wayland session, you can use ConditionEnvironment=WAYLAND_DISPLAY for that.

Can't we use ConditionEnvironment=SWAYSOCK same way?

@rpigott
Copy link
Member

rpigott commented Apr 27, 2023

A sway-session.target has utility in a systemd environment because graphical-session.target has utility, and that is how the systemd documentation directs graphical-session.target to be used (rather than started directly).

Really it's just a suggestion for how to use sway within systemd. The wiki is an acceptable place for this file but so is the repo imo. Ideally the distro would carry this as necessary but I don't think its too much trouble to help them out here, especially since Arch at least has got it wrong atm. I personally use a similar target in place of Arch's 50-systemd-user.conf just as suggested in the OP and I don't start sway as a user service.

Incorrect, sway provides an IPC socket that services cannot know is ready solely based off of graphical-session.target. Again, this is not about the wayland session, you can use ConditionEnvironment=WAYLAND_DISPLAY for that.

Can't we use ConditionEnvironment=SWAYSOCK same way?

The ConditionEnvironment directive isn't very useful in either case. One generally just wants a dependency ordering here, not conditional execution. Sway does ensure both vars are set before "exec" commands are run though.

The trouble on systemd on some distros is that the vars may not yet be available in the user service manager's environment block until dbus-udpate-activation-environment --systemd returns successfully. So some applications which are actually user services have trouble because that's where they inherit their environment. Arch tries to help users manage this by providing that command in /etc/sway/50-systemd-user.conf, but because exec is asynchronous in sway and there is no target for systemd user services to require, it is useless.

The file is small and distros can ignore it if they want. I don't think it really contributes to lock-in in the same way that say linking libsystemd would, and we also provide that as an option.

@WhyNotHugo
Copy link
Contributor

Can't we use ConditionEnvironment=SWAYSOCK same way?

ConditionEnvironment= evaluates whether an environment variable is set for the systemd process. It does not evaluate whether a variable is set in the environment for forked processes (e.g.: those shown in show-environment).

ConditionEnvironment=SWAYSOCK will always be false unless you set the variable before systemd --user itself starts.

See: systemd/systemd#20366

@WhyNotHugo
Copy link
Contributor

WhyNotHugo commented Apr 27, 2023

Even with this target, users still need "glue" in their sway config to actually trigger this target once sway has initialised. The target file could contain a comment that instructs to user how to do this. E.g., something like:

# This target should be used as a dependency for services that must be started
# after sway has finished initalising. It must be started manually, for example,
# by adding the following at some point in sway's config:
# 
#  # Run all other session services that depend on sway.
#  exec systemctl --user start sway-session.target

Edit: actually, users still need to import things like WAYLAND_DISPLAY and SWAYSOCK into their systemd session for services that depend on sway to work. The target by itself requires quite a bit of additional setup to really work.

I'll also point out that systemd supports only one concurrent user session at a time (this is a known limitation). If you log in on a second tty, the existing systemd instance is shared, so the sway instance on that second tty wouldn't have any services run for it. It would also mess up systemd's service environment and when services (re)start, they would run on the second sway instance, or fail to start after the second sway instance exits (even if the first is still running).

@kennylevinsen
Copy link
Member

The wiki is an acceptable place for this file but so is the repo imo.

Hmm, the "systemd integration" section of the wiki seems like a fine location for this indeed, and in fact it already seems to be present there (https://github.com/swaywm/sway/wiki/Systemd-integration#managing-user-applications-with-systemd). We already have content in that section that we disagree even more with anyway (running sway itself as a user service).

With that in mind, NACK on a PR that puts it in the repo.

@LaserEyess
Copy link

Having this is the wiki is not a substitute for having it in the repository and available for installation by downstream. The point of this file is to provide this functionality for 3rd party projects, and to standardize it so downstream (distros) do not have to carry patches for this. There is absolutely no chance of standardization happening at the distro level, none.

If this is such an affront to the ideologies of the maintainers then perhaps it could be gated behind systemd support:

if sdbus.name() == 'libsystemd'
   install_data('sway-session.target', install_dir: systemd_install_dir)
endif

Now BSDs and devuan won't be poisoned with the file.

@markstos
Copy link
Contributor

I'm in favor of merging this, but I'm also in favor of OSS maintainer's right to choose.

In this case, the problem is solved by the sway-systemd package, which is already available for Arch Linux and derivatives, Fedora and derivatives and OpenMandriva:

https://repology.org/project/sway-systemd/versions

The package is easy to find if you search for sway and systemd and other distros are welcome to package it. Besides providing a target file, this package takes care of a few other details of sway/systemd integration that people are likely to also want-- features that are also unlikely to be merged in the Sway repo. Considering this context, I'm also supportive of rejecting this PR in favor of that, because even closing the issue provides some clarity on the issue and can be seen as an endorsement of that alternative.

@flancian
Copy link

flancian commented Jul 27, 2023

You are my hero @maximbaz \o/

For maintainers: please merge this. It is the lighter weight option; I prefer to manage my own user units using dotfile management (chezmoi). Adding this service and the right incantation in the sway config file is just what I wanted.

@WhyNotHugo
Copy link
Contributor

Contrib has been moved to an external repository: https://github.com/OctopusET/sway-contrib

Maybe this file can find a home there?

markstos added a commit to markstos/sway-contrib that referenced this pull request Aug 17, 2023
@markstos
Copy link
Contributor

Submitted to third-party repo: OctopusET/sway-contrib#6

It still seems like it would be better if that sway-contrib repo were under the umbrella of the Sway github organization. The contrib project could be granted different managers so it could still be independent.

OctopusET pushed a commit to OctopusET/sway-contrib that referenced this pull request Aug 18, 2023
@bl4ckb0ne
Copy link
Contributor

Closing it as it has been merged in sway-contrib

@bl4ckb0ne bl4ckb0ne closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or incremental improvement
Development

Successfully merging this pull request may close these issues.

None yet