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

nixos/hypridle: only run the service on Hyprland #355416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JohnRTitor
Copy link
Contributor

@JohnRTitor JohnRTitor commented Nov 12, 2024

Since #355959 have been merged we can finally get this going.

This PR makes the service only start with "[email protected]". This needs a UWSM configured setup.

Finally closes #347651

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • Tested, as applicable:
    • With my own configuration

@JohnRTitor JohnRTitor requested a review from Atemu November 12, 2024 12:58
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 12, 2024
@JohnRTitor JohnRTitor requested a review from fufexan November 12, 2024 12:58
Copy link
Contributor

@fufexan fufexan left a comment

Choose a reason for hiding this comment

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

I don't think this is a good way to check for Hyprland's existence.
Take the scenario where a user logs into Hyprland, closes it, and then logs into another compositor/DE/WM. The directory will still exist until the next boot (or whenever it is removed), and hypridle will start every time, even on sessions where it may be unwanted.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 12, 2024

Well, this kind of safeguards it instead of just letting it start once graphical-session.target has reached.

Another approach would be checking for a variable, like XDG_SESSION_DESKTOP.

  systemd.user.services.hypridle.unitConfig.ConditionEnvironment = "XDG_SESSION_DESKTOP=Hyprland";

@JohnRTitor
Copy link
Contributor Author

CC @Vladimir-csp for inputs, if they have a better idea :D

@Vladimir-csp
Copy link

My idea is to reuse systemd-xdg-autostart-condition which is a predictable mechanism from systemd's implementation of XDG Autostart. I would avoid custom solutions like probing for compositor-specific files or content of XDG_SESSION_DESKTOP.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 12, 2024
@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 13, 2024

Not familiar with the implementation. AFAIK you need a desktop file for XDG autostart?
Could you provide an example? A a simple service file is fine, if you are unfamiliar with Nix.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 13, 2024

This would work only if XDG_SESSION_DESKTOP is set and only if it is set to Hyprland. Uwsm does that (it takes the first item of XDG_CURRENT_DESKTOP), but other solutions may not.

Originally posted by @Vladimir-csp in 0021223

I suppose so, but why Hyprland does not set the variable by default (as in force it)? @fufexan

@fufexan
Copy link
Contributor

fufexan commented Nov 13, 2024

The 2nd option in this comment has the code for XDG autostart condition #347651 (comment).

@Vladimir-csp
Copy link

Vladimir-csp commented Nov 13, 2024

Pointing WantedBy to a specific unit (or units) is also perfectly valid, you can add as many as needed.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 14, 2024

I'm all for it. Though I'm unsure of the route we should take to make hypridle depend on the Hyprland session:

  1. We simply add .wantedBy = "wayland-session@hyprland\x2duwsm.desktop.target";
  2. We add .serviceConfig.ExecCondition = ''${pkgs.systemd}/lib/systemd/systemd-xdg-activation-condition "Hyprland" ""'';

Option 1 would allow users to add other targets as well (to run hypridle with other compositors), while option 2 would limit hypridle's use to Hyprland only.

Option 1 is not feasible without UWSM, we want to have it working without relying on UWSM if possible. A question also arises if Hypridle even works on other compositors.

Option 2 is good, but I suppose this will also work when UWSM is not enabled? (Does Hyprland implement XDG-activation-condition by itself?)

@Vladimir-csp
Copy link

Option 1 is not feasible without UWSM, we want to have it working without relying on UWSM if possible. A question also arises if Hypridle even works on other compositors.

It might be feasible with other implementations that provide a compositor-specific units. What I'm saying is, there would be no conflict if WantedBy would point to multiple different units of different implementations.

Option 2 is good, but I suppose this will also work when UWSM is enabled? (Does Hyprland implement XDG-activation-condition by itself?)

It should work with any implementation that invokes systemd's xdg-desktop-autostart.target. Hyprland AFAIK has no XDG Autostart implementation of its own.

@JohnRTitor
Copy link
Contributor Author

I'd lean towards the WantedBy solution then. But that just that alone won't solve our issue.

If we have two wantedBys for example (without the conditions set on this PR), "graphical-session.target" and "[email protected]" it will still launch on other DEs not solving our issue. Again, we have to leave graphical-session be, as it should work without UWSM.

This will just bring us back to where we started.

@Vladimir-csp
Copy link

But which unit binds to graphical-session.target in alternative solutions?

@fufexan
Copy link
Contributor

fufexan commented Nov 14, 2024

WantedBy = "graphical-session.target" has to go. If there's any systemd user not willing to transition to uwsm, they will need to use exec-once = systemctl --user start hypridle. This is a breaking change and has been/will be marked as such.

@JohnRTitor
Copy link
Contributor Author

WantedBy = "graphical-session.target" has to go

If using UWSM is the only recommended solution for systemd support in Hyprland by upstream, we can remove graphical-session.target in its entirety from here.

I think we should start by adding a withUWSM option in Hyprland module, to make it easier for users and enable it by default later. I'll open a PR soon.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 14, 2024
This needs uwsm enabled. You can do so by setting `programs.hyprland.withUWSM = true`
@JohnRTitor JohnRTitor requested a review from fufexan November 17, 2024 18:19
Copy link
Contributor Author

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

image

nixos manual built

Copy link
Contributor

@fufexan fufexan left a comment

Choose a reason for hiding this comment

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

I think we should provide some information about setting systemd.user.services.hypridle.wantedBy to other values to allow using hypridle with other compositors.

I'd prefer this over duplicating the wantedBy option.

@fufexan
Copy link
Contributor

fufexan commented Nov 17, 2024

Also, the target will be different when using UWSM. In my case it is wayland-session@hyprland\x2duwsm.desktop.target. I assume it will be the same for everyone using the withUWSM option, due to how systemd escapes the desktop entry's path (hyprland-uwsm.desktop).

@JohnRTitor
Copy link
Contributor Author

Sure, but atleast one wantedBy here is needed, else NixOS module system won't recognise the service AFAIK.

@JohnRTitor
Copy link
Contributor Author

Also, the target will be different when using UWSM.

Is that supposed to work like that?

image

I am using the standard configuration of Hyprland with UWSM.

https://github.com/JohnRTitor/nix-conf/blob/6b71ccfb2a1cc273a8c37bb747f736b9f3c1690c/system/hyprland/session.nix#L21-L27

@fufexan
Copy link
Contributor

fufexan commented Nov 17, 2024

Ok, this might be GDM at play here. I'm using greetd's initial_session (autologin) with uwsm start hyprland-uwsm.desktop. I thought it would process the file and get the required info from it, instead of passing the name verbatim.

@Vladimir-csp could this be implemented? Or should I modify my call?

@JohnRTitor
Copy link
Contributor Author

uwsm start hyprland-uwsm.desktop

I mean... isn't that double wrapping with uwsm? hyprland-uwsm.desktop should wrap Hyprland with uwsm already. GDM can execute the hyprland-uwsm.desktop directly, not sure about Greetd.

@Vladimir-csp
Copy link

Unit specifier comes from whatever was given to uwsm start as the main argument, that is how uwsm in the units knows what to search for and do stuff with. In case of uwsm start hyprland-uwsm.desktop the resulting specifier will indeed be hyprland\x2duwsm.desktop. No double wrapping will happen, since uwsm is capable of parsing Exec=uwsm start ... from such entries. But launching non-uwsm entries by uwsm is preferable. As is launching uwsm entries via DMs.

@JohnRTitor
Copy link
Contributor Author

resulting specifier will indeed be hyprland\x2duwsm.desktop

perhaps, in case of Hyprland or Sway, autodetection could be done and the service will always be: [email protected] or [email protected], regardless of how it is launched?

@fufexan
Copy link
Contributor

fufexan commented Nov 17, 2024

Hardcoding that would be ugly. Treating the Exec line inside the desktop file (if it has uwsm start) verbatim would be a way to go. Though this could be abused to produce an infinite loop.

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 19, 2024
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

This does not work. At the minimum, you need what I worked out here:

#347651 (comment)

Comment on lines +32 to +34
# Service should be only be started on Hyprland
# this target is started by UWSM
wantedBy = [ "[email protected]" ];
Copy link
Member

Choose a reason for hiding this comment

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

This will only start the service. It will not wait for to have started and never stop it either.

systemd.user.services.hypridle = {
# Service should be only be started on Hyprland
# this target is started by UWSM
wantedBy = [ "[email protected]" ];
Copy link

@Vladimir-csp Vladimir-csp Nov 21, 2024

Choose a reason for hiding this comment

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

Suggested change
wantedBy = [ "[email protected]" ];
wantedBy = [ "[email protected]" ];
# order start after compositor (and WAYLAND_DISPLAY) is available
# order stop before compositor
after = [ "[email protected]" ];
# squeeze before graphical session.target
before = [ "graphical-session.target" ];
# stop with the session
partOf = [ "graphical-session.target" ];

Copy link
Member

@Atemu Atemu Nov 21, 2024

Choose a reason for hiding this comment

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

These two targets do not exist on NixOS. We've explicitly limited this module to only work with the programs.hyprland.withUWSM option which only creates the [email protected]. (I'd prefer a stronger API than the Hyprland package's binary name here but that's something our uwsm module would need to offer.)

We also want to order hypridle before the wayland session so that it's only considered up when hypridle has actually started. (Wants= does not imply any order.)

Choose a reason for hiding this comment

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

edited

Copy link
Member

Choose a reason for hiding this comment

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

In my config I made it before and partOf [email protected] because I think of hypridle as belonging to the "Hyprland session" rather than the graphical-session directly. This would also be okay IMV.

I do find it a bit weird to depend on the session target rather than the wm service though as that's our actual dependency.

Choose a reason for hiding this comment

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

Units of service and target are bound, so there is no difference for dependencies. Targets are more frequently used as dependencies.

$ grep -r ^WantedBy= /lib/systemd/system | grep -cF .target
124
$ grep -r ^WantedBy= /lib/systemd/system | grep -cF .service
90
$ grep -r ^WantedBy= /lib/systemd/user | grep -cF .target
24
$ grep -r ^WantedBy= /lib/systemd/user | grep -cF .service
17

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't follow. Could you reword?

Choose a reason for hiding this comment

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

[email protected] is bound to [email protected], so they are interchangeable as dependencies (but can differ when ordering).

Targets are more frequently used for dependencies than services.

Copy link
Contributor Author

@JohnRTitor JohnRTitor Nov 21, 2024

Choose a reason for hiding this comment

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

Depending on graphical-session.target, is fine in my opinion.

When a WM managed by UWSM is terminated, graphical-session also stops. So I understand the partOf bit.

What I don't understand is why are we supposed to start it "before" graphical-session? Starting it after isn't correct?

I'd prefer a stronger API than the Hyprland package's binary name here

Agreed.

EDIT: I did read the above discussion about before, but still confused

Copy link
Member

Choose a reason for hiding this comment

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

What I don't understand is why are we supposed to start it "before" graphical-session? Starting it after isn't correct?

Because we only want the session to be considered up and running once hypridle is up and running.

Comment on lines +18 to +19
For this service to work properly on Hyprland, you
need to have `programs.hyprland.withUWSM` enabled.
Copy link
Member

Choose a reason for hiding this comment

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

We should assert that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because @fufexan wants it to be compatible with other compositors.

Copy link
Member

Choose a reason for hiding this comment

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

As it stands right now, it cannot be compatible with other compositors because it depends on the hyprland-uwsm-specific units.

If you wanted to use it with another compositor, you should bring your own hypridle unit.

Choose a reason for hiding this comment

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

It could be compatible if you just add more targets in WantedBy and After.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or simply add systemd.user.services.swayidle.wantedBy = ["your-compositors-session.target"];. This should be an admonition in the enable option's description.

Copy link
Member

Choose a reason for hiding this comment

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

We could turn the targets that hypridle is wantedBy and before into an option. withUWSM would set it to the UWSM-generated session unit. We could then simply assert that this option's value isn't an empty list.

@JohnRTitor
Copy link
Contributor Author

JohnRTitor commented Nov 21, 2024

@fufexan just so we are on the clear here, at the moment, is hypridle supported on WMs like sway, wayfire, mir?

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 22, 2024
manuelbb-upb added a commit to manuelbb-upb/nixos that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixos/hypridle: hypridle incorrectly starts on other DEs as it is wantedBy graphical-session.target
6 participants