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

systemctl import-environment without specifying desired variables is deprecated #254

Open
jokeyrhyme opened this issue Mar 12, 2024 · 18 comments · May be fixed by #255
Open

systemctl import-environment without specifying desired variables is deprecated #254

jokeyrhyme opened this issue Mar 12, 2024 · 18 comments · May be fixed by #255
Labels
bug Something isn't working

Comments

@jokeyrhyme
Copy link

Howdie, thanks so much for sharing this project! <3

I noticed a deprecation message pop up briefly during niri's launch, and I tracked it down to this: systemd/systemd#18137

And we call systemctl --user import-environment without specific variable names here:

systemctl --user import-environment

The thing is, I'm not sure when this will stop working on the systemctl side, seeing as it's been deprecated for years

And I'm not even sure what I'd recommend going forward

  • do we drop the line completely? this might break existing user scripts, however, these values will still end up in the environment variables for the niri process itself (and thus every child process it launches), just not in the user service manager
  • do we provide a list of variables that users are likely to want (e.g. PATH)? that seems like a game of whack-a-mole

Another aspect of the current behaviour is that variables from the virtual console environment end up in my user service manager environment, like TERM=linux and PWD which are often unhelpful/confusing once I'm in a GUI desktop environment (but I can probably find workarounds for my own stuff)

Anyhow, this has probably not been super helpful, but I thought I'd log it, just in case anyone else notices the deprecation message :)

@jokeyrhyme jokeyrhyme added the bug Something isn't working label Mar 12, 2024
@YaLTeR
Copy link
Owner

YaLTeR commented Mar 12, 2024

Hey! I cargo culted that line from other projects. I'm not sure if it's actually needed. Could try removing it (and the dbus one below) and see if everything still works?

The actually important variables like WAYLAND_DISPLAY are imported by the niri binary itself.

@jokeyrhyme
Copy link
Author

jokeyrhyme commented Mar 12, 2024

I believe we'd still want the WAYLAND_DISPLAY and XDG_... variables to end up in D-Bus and systemd after niri is ready and before too many (or any at all) other new processes have started

I had a related conversation with the COSMIC folks about this, but that solution doesn't map cleanly to the infrastructure we have here: pop-os/cosmic-session#31

@YaLTeR
Copy link
Owner

YaLTeR commented Mar 12, 2024

I believe we'd still want the WAYLAND_DISPLAY and XDG_... variables to end up in D-Bus and systemd after niri is ready and before too many (or any at all) other new processes have started

Yeah, the niri binary takes care of that.

@Vladimir-csp
Copy link

Is there a way to disable activation environment management and readiness notification at runtime? For a case of external service manager? Like via a variable? Or to configure which variables are exported to activation environments?

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 10, 2024

Uhh, not really? Not sure why you'd want to disable that.

@Vladimir-csp
Copy link

Also, a hint: dbus-update-activation-environment is not needed if dbus is provided by dbus-broker (it reuses systemd environment). And I did not find any environment cleanup handling (which classic dbus has troubles with).

@Vladimir-csp
Copy link

Uhh, not really? Not sure why you'd want to disable that.

So it would be handled properly by a session manager, and dependent services would not be started prematurely.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 10, 2024

Also, a hint: dbus-update-activation-environment is not needed if dbus is provided by dbus-broker

And if it's not provided by dbus-broker? :) Some distros that run niri don't even use systemd. It's easier to have a common code path.

And I did not find any environment cleanup handling

It's in the resources/niri-session script.

So it would be handled properly by a session manager, and dependent services would not be started prematurely.

But it is already properly handled and dependent services are not started prematurely.. I guess I don't really understand why uwsm is needed for niri which already provides a working systemd integration?

@Vladimir-csp
Copy link

It has its features, like login shell integration, idempotent environment management (including cleanup and dbus/dbus-broker issues workarounds), clean unit shutdown, various helpers, desktop entry launch wrapper, etc...

Anyway, people use it and I got a bug report which appeared like a premature compositor unit readiness, but most likely it is not. If compositor exports essential variables to activation environments and signals unit readiness by itself, it is great there is no incompatibility here. The only remaining issue would be additional variables: if user tries to add more variables via uwsm finalize ... in compositor's startup, those vars will most likely come too late because compositor already notified startup at this point.

Similar issue with Hyprland was resolved by a couple of environment switches (one, two) which are used on uwsm's side via a plugin. Labwc also has LABWC_UPDATE_ACTIVATION_ENV switch, and it does not hardcode unit readiness notification.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 10, 2024

I guess it's not too big a deal to gate sd notify behind an env var, but is that really correct here? Like, my niri.service has Type=notify, which means that niri must notify its readiness. And if you're planning to use a custom service instead of niri.service, then you should already be able to set up the dependencies in such a way that niri by itself will not activate graphical-session.target, and instead for example activate a uwsm target that will add any extra variables and then activate graphical-session.target?

@Vladimir-csp
Copy link

I guess it's not too big a deal to gate sd notify behind an env var, but is that really correct here?

There is always a possibility that a compositor can be integrated into some overarching structure like a DE or a session manager. In this case a way to hand off session/unit/environment operations would be useful.

niri.service has Type=notify, which means that niri must notify its readiness.

wayland-wm@.service has:

Type=notify
NotifyAccess=all

Which allows receiving notification from child processes, that is uwsm finalize command, which has to be run by compositor, to also get vars from it. It exports WAYLAND_DISPLAY (always) and DISPLAY (if set), and any other variables that were requested by user or plugin, also maintaining a cleanup list.

And if you're planning to use a custom service instead of niri.service, then you should already be able to set up the dependencies in such a way that niri by itself will not activate graphical-session.target, and instead for example activate a uwsm target that will add any extra variables and then activate graphical-session.target?

Triggering graphical-session.target via a different unit is an idea worth investigating, thanks! I guess it can be a process that watches activation environment for WAYLAND_DISPLAY to appear. But it is not obvious how to maintain a cleanup list if compositor puts more variables into activation environment by itself. And how to access any var that a compositor might set, but not export to activation environment (I'm thinking general cases here).

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 13, 2024

I see you're experimenting with this idea; let me know if it works out and what changes, if any, would be needed if it does.

@Vladimir-csp
Copy link

My experiments will at least yield a delayer for graphical-session.target that waits for WAYLAND_DISPLAY to appear. Probably, also a workaround for adding more vars from compositor's environment to activation environment while delayer specifically waits for those vars, even after compositor already dealt with WAYLAND_DISPLAY and unit readiness by itself. Unfortunately I do not yet see a generic automagical solution that accounts for all possible compositor's behaviors without compositor-specific preconfiguration.

As for changes, as I said above:

There is always a possibility that a compositor can be integrated into some overarching structure like a DE or a session manager. In this case a way to hand off session/unit/environment operations would be useful.

So adding some kind of a switch would be nice in any case. No other changes required.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 13, 2024

I added NIRI_DISABLE_SYSTEM_MANAGER_NOTIFY env var, setting it 1 should suppress both systemd and NOTIFY_FD notification. Could you try if that's sufficient for you?

I don't think suppressing env import should be that necessary?

@Vladimir-csp
Copy link

Thank you!

@Vladimir-csp
Copy link

It went better than I expected.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 15, 2024

Interesting, so disabling sd notify isn't necessary after all?

@Vladimir-csp
Copy link

In case of uwsm now it isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants