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

treewide: standardize Wayland graphical services #6239

Conversation

thiagokokada
Copy link
Contributor

@thiagokokada thiagokokada commented Dec 28, 2024

Description

Graphical services that are meant for Wayland environment are all over the place:

  • Some set After = [ "graphical-session-pre.target" ], that is almost always a mistake (see), others set After = [ "graphical-session.target" ]
  • Some have systemd.target (or systemdTarget) to allow changing the default target, some does not
  • Some set ConditionEnvironment = "WAYLAND_DISPLAY", some does not

This PR tries to standardize this by:

  • Adding systemd.target to services missing it, and defaulting it to graphical-session.target
  • Set After, PartOf and Wanted to cfg.systemd.target
  • Adding ConditionEnvironment = "WAYLAND_DISPLAY" for services missing it

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@@ -37,6 +37,19 @@ in {

systemd = mkEnableOption "systemd service and socket for wob"
// mkOption { default = true; };

systemdTarget = mkOption {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

systemd is sadly already used to enable the service above, so using systemdTarget instead to avoid breaking changes.

@thiagokokada thiagokokada changed the title Standardize graphical services treewide: standardize Wayland graphical services Dec 28, 2024
@thiagokokada
Copy link
Contributor Author

CC @rycee.

@Lillecarl
Copy link
Contributor

Lillecarl commented Dec 29, 2024

This is good. If I had a say in it I'd create an option like "wayland.systemdTarget" and make all the options default to this one. What do you think? Then we end up with the hardest problem in programming... Naming things.

@rycee
Copy link
Member

rycee commented Dec 30, 2024

Thanks @thiagokokada for taking on this. It has indeed become quite the mess. I'll have a deeper look at this PR as soon as possible.

I think @Lillecarl's idea to have a common wayland.systemdTarget seems quite good. I suspect it would be very rare that anybody would like different targets.

That said, there is something fundamentally wrong somewhere if we cannot use graphical-session.target and graphical-session-pre.target as-is. The whole point of these targets is to handle the graphical session services! I haven't had time and motivation to look into it since graphical-session.target works fine for me even on Wayland (in my case Hyprland).

@thiagokokada
Copy link
Contributor Author

I think @Lillecarl's idea to have a common wayland.systemdTarget seems quite good. I suspect it would be very rare that anybody would like different targets.

I initially had the same idea, but was against it because it would be more work (e.g.: deprecating old options). But if this is the way we want to do things I can redo this work this way instead.

That said, there is something fundamentally wrong somewhere if we cannot use graphical-session.target and graphical-session-pre.target as-is. The whole point of these targets is to handle the graphical session services! I haven't had time and motivation to look into it since graphical-session.target works fine for me even on Wayland (in my case Hyprland).

To be clear, graphical-session.target works fine to me, but graphical-sesion-pre.target doesn't since in most cases things that services depends are not ready yet (e.g.: WAYLAND_DISPLAY). The only reason I added systemd.target for other services is for consistency, but my main reason for this PR is replacing After = [ "graphical-session-pre.target" ]; with After = [ "graphical-session.target" ];.

@thiagokokada
Copy link
Contributor Author

BTW, graphical-session-pre.target only makes sense for the compositor (e.g.: Hyprland, Sway, etc.), everything else should use graphical-session.target: Vladimir-csp/uwsm#40 (comment).

@Lillecarl
Copy link
Contributor

That said, there is something fundamentally wrong somewhere if we cannot use graphical-session.target and graphical-session-pre.target as-is. The whole point of these targets is to handle the graphical session services! I haven't had time and motivation to look into it since graphical-session.target works fine for me even on Wayland (in my case Hyprland).

Everything should work just fine on graphical-session.target, but considering the target audience (people who run "not in [ kde gnome ] I think having the option available is a good thing.

I initially had the same idea, but was against it because it would be more work (e.g.: deprecating old options). But if this is the way we want to do things I can redo this work this way instead.

You can just set "default = config.wayland.systemdTarget" in all options that already exist, you don't need to deprecate them. Then again, they're just one mkForce away if they're deprecated and removed.

@thiagokokada
Copy link
Contributor Author

You can just set "default = config.wayland.systemdTarget" in all options that already exist, you don't need to deprecate them. Then again, they're just one mkForce away if they're deprecated and removed.

I know, but unless we deprecate the old options the inconsistency will exist and folks will find new and creative ways to abuse the old target (like this one #6241, that I find it would be better done by disabling the systemd service instead).

@thiagokokada
Copy link
Contributor Author

Ok, I created a wayland.systemd.target and add a few more services. Opened a new PR: #6253.

@thiagokokada thiagokokada deleted the standardize-graphical-services branch December 31, 2024 13:57
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.

3 participants