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

fix(niri-session)!: no longer import all environment variables into dbus/systemd #255

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jokeyrhyme
Copy link

fixes #254

I did some testing and can confirm that the contents of my environment variables and those in my user service manager were as expected on my machine

But note that I'm not yet using niri as my primary desktop, so it's possible/probable that my testing doesn't cover something someone else would notice and miss dearly

@YaLTeR
Copy link
Owner

YaLTeR commented Mar 12, 2024

I tried this on my GDM setup and it panics upon starting; presumably, it doesn't know what TTY it's supposed to take over because some environment variable is missing.

Actually, this makes me remember another reason for importing all variables: so that variables from the shell .bashrc and all make it into the spawned processes (via niri or systemd itself). I don't think there's any other way of doing this? What does systemd recommend?

@jokeyrhyme
Copy link
Author

Yeah, I wasn't specific, but the .bashrc variables were what I was thinking of when I mentioned this over in the issue:

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

When I tested this, I started from greetd as my greeter, I'll make sure to also test with GDM when I revisit this

I don't know if it's recommended, but what I've been doing with my own shell setup (for probably years now) is to e.g. set PATH and then explicitly systemctl --user import-environment PATH (all inside my shell setup)

@jokeyrhyme jokeyrhyme marked this pull request as draft March 12, 2024 19:11
@YaLTeR
Copy link
Owner

YaLTeR commented Mar 13, 2024

I have a concrete use-case: right now on Fedora they set $DEBUGINFOD_URLS in the distro-provided bash init scripts, and with the current blanket import-all, it ends up in niri and in its children fully automatically without having to manually do anything.

@jokeyrhyme
Copy link
Author

Hmmm, anything in the user's default SHELL (or bash as a fallback) should end up in the environment variables for niri, and I believe that passes them along to any child processes

So, it's only processes activated by D-Bus or systemd that would miss out (I imagine there is a smaller number of these compared to those launched as niri's child processes, but that's a guess)

To prevent regressions for those processes, we can come up with a list of variables, or we can decline this PR :)

@YaLTeR
Copy link
Owner

YaLTeR commented Mar 13, 2024

Hmmm, anything in the user's default SHELL (or bash as a fallback) should end up in the environment variables for niri

How? Right now the way it ends up there, is by the blanket import-environment, as far as I can tell. Because niri is started as a systemd service also, so it just uses that global environment.

Perhaps in your case something else is doing a blanket import-environment earlier, so you have it filled in?

@jokeyrhyme
Copy link
Author

Bah, I totally forgot about systemctl --user --wait start niri.service sorry!

Yep, I'm onboard, it's a problem :)

@YaLTeR
Copy link
Owner

YaLTeR commented Mar 13, 2024

FWIW I imagine the gnome session has the same issue to deal with? Have they come up with anything to this end yet?

I'm pretty sure a few years ago they tried avoiding the shell in the session startup, then ran into the same problem that users expect shell variables to be set.

@jokeyrhyme
Copy link
Author

jokeyrhyme commented Mar 13, 2024

Some other compositors do harvest environment from SHELL but don't seem to run as a systemd unit:

Hyprland does offer a systemd unit ( https://github.com/hyprwm/Hyprland/blob/893c55217b15cf0e5fe7eb3eb2d88183df410b9a/example/hyprland.service ) but doesn't seem to try to harvest values from the user's default SHELL ( https://github.com/hyprwm/Hyprland/blob/893c55217b15cf0e5fe7eb3eb2d88183df410b9a/src/main.cpp )

I think KDE actually works either as a systemd unit or not, but I can't find if they do or don't harvest values from SHELL: https://invent.kde.org/plasma/plasma-workspace/-/blob/1c67160e780a981b95d16f578284974ee0d6be8a/startkde/startplasma.cpp#L687-730

dwl, river, and sway leave all of this as an exercise for the reader (no systemd unit, and no SHELL variables) :)

@jokeyrhyme
Copy link
Author

Perhaps in your case something else is doing a blanket import-environment earlier, so you have it filled in?

Ah, yes, I went all-in on systemd a while back and have a bunch of user environment generators: https://gitlab.com/jokeyrhyme/dotfiles/-/tree/main/etc/systemd/user-environment-generators

Which explains why the SHELL setup on my machine is so inconsequential, as intended and then later forgotten by me :)

@jokeyrhyme
Copy link
Author

Okay, thinking about this a bit more, abandoning this is still an option (we don't know when upstream's deprecation will become a hard-removal, if ever): won't break compatibility with existing setups

Or we can change the approach to not launch niri as a systemd service, just run it directly in niri-session, similar to how COSMIC does it, and we can have a new niri-session.target to trigger graphical-session.target (started by niri), but there's lots of value in the systemd approach we currently have (and it's not clear that can be retained): users setting up other units to depend on niri.service will have to change that to work with one of the targets instead

Or we can change niri-session to drop all the variables in a file, and change niri.service to use EnvironmentFile= , although the systemd man pages discourage use of environment variables for secrets, but storing this as a file seems inelegant to me, and I wonder if we'd be storing some access token or other confidential value accidentally: won't break compatibility with existing setups

Or we can hardcode a list of environment variables that we pass to systemctl --user import-environment, focusing on the most popular/necessary ones like PATH, XDG_..., DEBUGINFOD_URLS, LC_..., etc: will probably break somebody's existing setup

There are probably other solutions, I'm just trying to put together the list while I mull it over :)

@YaLTeR
Copy link
Owner

YaLTeR commented Mar 18, 2024

Or we can change the approach to not launch niri as a systemd service, just run it directly in niri-session, similar to how COSMIC does it, and we can have a new niri-session.target to trigger graphical-session.target (started by niri), but there's lots of value in the systemd approach we currently have (and it's not clear that can be retained): users setting up other units to depend on niri.service will have to change that to work with one of the targets instead

Yeah, I think having a systemd service is good and has a lot of benefits. Another example is you can do systemctl --user status niri.service to see the status / logs, and also having this service unit lets niri live in its own cgroup in the session slice with potentially higher resource allowance compared to regular processes.

Or we can change niri-session to drop all the variables in a file, and change niri.service to use EnvironmentFile= , although the systemd man pages discourage use of environment variables for secrets, but storing this as a file seems inelegant to me, and I wonder if we'd be storing some access token or other confidential value accidentally: won't break compatibility with existing setups

That kinda sounds just like reinventing a blanket import-environment but more complex and less brittle, plus systemd-activated programs won't get the environment variables, which is undesirable.

Or we can hardcode a list of environment variables that we pass to systemctl --user import-environment, focusing on the most popular/necessary ones like PATH, XDG_..., DEBUGINFOD_URLS, LC_..., etc: will probably break somebody's existing setup

Yeah that's just asking for trouble or endless maintenance.

abandoning this is still an option (we don't know when upstream's deprecation will become a hard-removal, if ever)

Yeah I guess let's just not worry about it for now, then see what others do like GNOME. Maybe some solution will emerge.

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.

systemctl import-environment without specifying desired variables is deprecated
2 participants