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

systemd-desktop - replace contents to rely on uwsm to start a systemd session. Remove hyprland-session.service #8376

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

izmyname
Copy link
Contributor

@izmyname izmyname commented Nov 7, 2024

Describe your PR, what does it fix/add?

As proposed and discussed here #8339 offload Hyprland systemd management to uwsm. This entry, in particular, is meant to be started with a display manager

Is there anything you want to mention?

See below

Is it ready for merging, or does it need work?

Yes

@izmyname izmyname changed the title systemd-desktop - replace contents to rely on uwsm to start a systemd session systemd-desktop - replace contents to rely on uwsm to start a systemd session. Remove hyprland-session.service Nov 7, 2024
@izmyname izmyname requested a review from fufexan November 7, 2024 16:01
@izmyname
Copy link
Contributor Author

izmyname commented Nov 7, 2024

Also, I didn't add uwsm dependency as we've yet to decide - whether it should be a mandatory systemd dependency or optional.

@fufexan
Copy link
Member

fufexan commented Nov 7, 2024

Shouldn't be required, as users can also write their own services instead of using uwsm. It's just easier and probably more correct to use uwsm.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 7, 2024

Fair. Though I guess I'd avoid adding uwsm opt dep to this PR myself, to not mess things up, for now.

UPD: Optional dependency thing can be done disto-side, as I understand. In case of Arch, just adding it into PKGBUILD should be enough.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 10, 2024

Some extras.

1. Technically, a desktop entry should work as I used @Vladimir-csp example for Hyprland for it. However, if someone can test it with a DM (especially, exiting and re-entering the session with user services enabled) - it would be great. Make sure to use loginctl terminate-user "" command, instead of hyprctl dispatch exit .

It works.

2. No idea, what's wrong with the clang_format, since merging 0.45, but as I see - many PRs now failing this check.

  1. Not related to PR - just a notice. Manual building with meson doesn't autodetect systemd. Probably, Systemd fixes #8320

@fufexan
Copy link
Member

fufexan commented Nov 10, 2024

@izmyname I just tested with GDM, seems to work fine.

@izmyname
Copy link
Contributor Author

@izmyname I just tested with GDM, seems to work fine.

In this case, I think the PR is ready.

pld-gitsync pushed a commit to pld-linux/hyprland that referenced this pull request Nov 10, 2024
- don't package systemd user unit / systed wayland session, installation
  path is wrong but it's not worth fixing as there's PR already to drop
  them anyway (hyprwm/Hyprland#8376)
Copy link
Contributor

@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.

Nice, so since this is essentially replacing Hyprland.desktop file.

In order to get this actually picked up by display-manager.service, we need to put this under /usr/share/wayland-sessions or NixOS equivalent. I suppose we need to update the Nix packaging.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 10, 2024

Nice, so since this is essentially replacing Hyprland.desktop file.

Non uwsm Hyprland.desktop is still available and will be installed alongside hyprland-uwsm.desktop

we need to put this under /usr/share/wayland-sessions

The PR already does it.

@fufexan
Copy link
Member

fufexan commented Nov 10, 2024

I guess this should also be in the PR.

diff --git a/nix/default.nix b/nix/default.nix
index 29d31485..4fef0f97 100644
--- a/nix/default.nix
+++ b/nix/default.nix
@@ -171,7 +171,7 @@ in
         ''}
       '';
 
-      passthru.providedSessions = ["hyprland"];
+      passthru.providedSessions = ["hyprland" "hyprland-uwsm"];
 
       meta = {
         homepage = "https://github.com/hyprwm/Hyprland";

@github-actions github-actions bot added the Nix NixOS issue label Nov 10, 2024
@izmyname
Copy link
Contributor Author

@fufexan hope I didn't miss anything

@fufexan
Copy link
Member

fufexan commented Nov 10, 2024

Looks fine.

@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

@izmyname I think this patch fixes meson autodetecting systemd:

diff --git a/meson.build b/meson.build
index 76765645..e4abad36 100644
--- a/meson.build
+++ b/meson.build
@@ -52,8 +52,12 @@ backtrace_dep = cpp_compiler.find_library('execinfo', required: false)
 epoll_dep = dependency('epoll-shim', required: false) # timerfd on BSDs
 
 # Handle options
-if get_option('systemd').enabled()
-  systemd = dependency('systemd')
+systemd_option = get_option('systemd')
+systemd = dependency('systemd', required: systemd_option)
+systemd_option.enable_auto_if(systemd.found())
+
+if (systemd_option.enabled())
+  message('Enabling systemd integration')
   add_project_arguments('-DUSES_SYSTEMD', language: 'cpp')
   subdir('systemd')
 endif
diff --git a/nix/default.nix b/nix/default.nix
index 4fef0f97..4756b1c8 100644
--- a/nix/default.nix
+++ b/nix/default.nix
@@ -152,7 +152,6 @@ in
         (mapAttrsToList mesonEnable {
           "xwayland" = enableXWayland;
           "legacy_renderer" = legacyRenderer;
-          "systemd" = withSystemd;
         })
         (mapAttrsToList mesonBool {
           "b_pch" = false;

Also tested with Nix by removing explicit option passing.

@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

After some tests I've concluded that installing the uwsm desktop file on NixOS has no benefit or is even detrimental. This is why I've added a feature flag to both Meson and CMake which is enabled by default, but can be easily disabled by users.

Also, the Type used in the desktop file was incorrect, as reported by uwsm.

Type is not "Application", "Link" or "Directory"

@izmyname
Copy link
Contributor Author

@izmyname I think this patch fixes meson autodetecting systemd:

It doesn't.

Also, we should probably remove gdb make dep from hyprland-wiki, as Hyprland builds perfectly without it. Additionally, Arch maintainers removed it from Hyprland PKGBUILD in extra repo.

izmyname and others added 5 commits November 11, 2024 11:02
The file in the repo cannot be used in NixOS due to missing full paths,
and the fact that `uwsm` does not have access to `PATH` to find the
listed binaries. Might be useful in other situations as well.
Will be enabled in the NixOS module.
@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

It doesn't.

If there's no option overriding from the CLI, and the systemd dep is found, then I have no clue what's going on.

@izmyname
Copy link
Contributor Author

If there's no option overriding from the CLI, and the systemd dep is found, then I have no clue what's going on.

Not the goal of this PR, so I think it can be fixed later. It doesn't break any existing workflow, anyway.

@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

Ok, I'll merge this and the wiki PR in a few hours when I'm free again.

Copy link
Member

@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.

LGTM.

@fufexan fufexan merged commit b88e4a1 into hyprwm:main Nov 11, 2024
12 checks passed
@izmyname
Copy link
Contributor Author

IMHO, makes sense to make an announcement for the people, who use old hyprland-session.service.

@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

Sure, I'll do that.

@Vladimir-csp
Copy link
Contributor

Thanks, everyone!

@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

Thank you for all the help!

@b4shful
Copy link

b4shful commented Nov 11, 2024

just spotted all this work you folks have been doing on this - looks awesome :)

(before using uwsm) the whole thing about launching portals and invocation of dbus-update-activation-environment at the right place, along with the various weird shell scripts killing processes and doing odd workarounds.. all that stuff was becoming a bit of a nightmare, I was always curious about how to improve things to do it "the right way", it seems this is the answer.

@b4shful
Copy link

b4shful commented Nov 11, 2024

Also, is there a place to discuss this further? I have found myself with a few questions after reading the Wiki entry, for example, does this mean I should prepend anything using the exec dispatcher with uwsm app --? And when it says "Running applications as child processes inside compositor’s unit is discouraged." is this what would occur if you just used the exec dispatcher on its own?

@fufexan
Copy link
Member

fufexan commented Nov 11, 2024

Also, is there a place to discuss this further?

Not yet but we can create a discussion.

does this mean I should prepend anything using the exec dispatcher with uwsm app --?

Yes. That and if you use an app launcher that supports prepending a launch prefix (like fuzzel or anyrun), you should also use that for the added benefit of also having those apps be bound to systemd units.

And when it says "Running applications as child processes inside compositor’s unit is discouraged." is this what would occur if you just used the exec dispatcher on its own?

Yes.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 11, 2024

does this mean I should prepend anything using the exec dispatcher with uwsm app --?

except flatpaks, hyprctl commands (like for hyprpaper) and clients for already started systemd processes (like footclient to foot-server), I suppose.

@fufexan btw, do you start a launcher, itself, with uwsm app -- prefix? I tried with fuzzel, but it gets slightly slower. to start

@fufexan
Copy link
Member

fufexan commented Nov 12, 2024

Yeah I start the launcher with uwsm app -- /nix/store/.../bin/anyrun, and when it starts an app, it does it like this: uwsm app -- /path/to/program.desktop.

I don't notice slowdowns. Maybe because I always give it the full desktop entry path and it doesn't have to search again.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 12, 2024

Did the same for fuzzel launch-prefix='uwsm app -- '

Although, probably, launching fuzzel, itself, without prefix isn't that critical, in my case - as I rarely keep it open for a long time and it isn't launched when I shutdown.

@Vladimir-csp
Copy link
Contributor

If a launcher doesn't live long, there is little point of wrapping it itself other than pedantic reasons.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 12, 2024

If a launcher doesn't live long, there is little point of wrapping it itself other than pedantic reasons.

Thanks for the clarification!

@izmyname
Copy link
Contributor Author

Also, some closing thoughts, regarding a proper systemd/xdg support.

@fufexan I think the next and final step for Hyprland to be fully on par with existing desktop environments would be implementing org.freedesktop.Background for xdg-desktop-portal-hyprland, in order, to properly autostart flatpaks without hacky workarounds, that look more like exploits. (see hyprwm/hyprland-wiki#850 (comment) Vladimir-csp/uwsm#51 (comment) and hyprwm/xdg-desktop-portal-hyprland#177 ).

@b4shful
Copy link

b4shful commented Nov 12, 2024

@fufexan

Not yet but we can create a discussion.

That would be cool - you guys are doing amazing work. @izmyname mentioned making Hyprland "fully on par with existing desktop environments" which definitely describes most of the stuff I've been hoping/wishing for surrounding this stuff.

It sounds kindof like a "project" in itself, or some changes in the general direction of "make it somewhat more like a desktop environment", so I'm sure someone could come up with a good title for a discussion thread.

There's a lot of things autostarted when using uwsm, is hyprpolkitagent part of this? Just thinking my HL config might have some exec-onces for hyprland desktop portal and hyprpolkitagent which may no longer be necessary.

@izmyname
Copy link
Contributor Author

izmyname commented Nov 12, 2024

hyprpolkitagent part of this?

hyprpolkit, hypridle and hyprpaper (git version) have their own systemd services you can enable on startup

portal

You don't need to explicitly autostart xdph. It's automatically started via D-Bus.

@fufexan
Copy link
Member

fufexan commented Nov 12, 2024

I think the next and final step for Hyprland to be fully on par with existing desktop environments would be implementing org.freedesktop.Background for xdg-desktop-portal-hyprland

Honestly I've never worked on xdph directly, but I will try to take a look when I have time. I don't use flatpaks myself so I'll have to ask you to aid with testing when that time comes.

@izmyname
Copy link
Contributor Author

I'll have to ask you to aid with testing when that time comes.

That goes without a question

@Ghosthree3
Copy link

I notice the wiki suggests using loginctl terminate-user "" instead of exit to exit Hyprland now, but the uwsm github page mentions using loginctl terminate-session "$XDG_SESSION_ID" to close only the current session. Are there any issues with using the latter? (If there are I imagine they're portal related).
terminate-user works as advertised, it kills every active login session and running units of the user, which means if I were to say run Hyprland in tty2 to bisect, I'd kill the tty1 session on exit. Also all background services/units are killed? This is not very desirable.

@fufexan
Copy link
Member

fufexan commented Nov 13, 2024

If terminate-session works fine, we can update the wiki.

@fufexan
Copy link
Member

fufexan commented Nov 13, 2024

I've made a discussion #8459.

@izmyname
Copy link
Contributor Author

izmyname commented Dec 26, 2024

Apologies for bumping the old discussion, but since there isn't a suitable xdph-related discussion thread, I post it here.

@fufexan you might be interested to take a look at how KDE implements the background. https://github.com/KDE/xdg-desktop-portal-kde/blob/master/src/background.cpp https://github.com/KDE/xdg-desktop-portal-kde/blob/master/src/background.h

Surprisingly, there seem to be just two relatively small files. I hope it doesn't lead to the need of changes within the compositor, itself, to avoid possible complexity of implementing the interface.

@fufexan
Copy link
Member

fufexan commented Dec 26, 2024

They look small but there are a lot of includes in there as well :)

@izmyname
Copy link
Contributor Author

izmyname commented Dec 26, 2024

@izmyname
Copy link
Contributor Author

izmyname commented Jan 6, 2025

Also, here's an issue on xdp-wlr tracker, regarding org.freedesktop.Background. The first post contains quite an interesting discussion with links and explanations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Nix NixOS issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants