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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnome3.gnome-session: always run /etc/set-environment on startup #70501

Merged
merged 1 commit into from Oct 7, 2019

Conversation

hedning
Copy link
Contributor

@hedning hedning commented Oct 6, 2019

Motivation for this change

gnomes-session sources its environment from a login shell. But if the systemd
user environment already contains __NIXOS_SET_ENVIRONMENT_DONE the environment
setup won't happen correctly. Simply unset this variable to ensure a fresh
environment.

This was previously handled by a GDM patch, but it no longer works reliably with
gnome-sessions systemd session.

This means we can drop the not so reliable patch in gdm. We still need the patch to handle plasma et. al.

See #48255 for more info on the
underlying issue.

With the newly added pam environment (and #70430) this shouldn't be strictly necessary, but it does clean up a now broken fix. No idea why I didn't just fix it like this in the first place 馃槅 (did it in GDM to handle other sessions obviously).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@ofborg ofborg bot added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Oct 6, 2019
@ofborg ofborg bot requested review from worldofpeace and jtojnar October 6, 2019 05:29
gnomes-session sources its environment from a login shell. But if the systemd
user environment already contains `__NIXOS_SET_ENVIRONMENT_DONE` the environment
setup won't happen correctly. Simply unset this variable to ensure a fresh
environment.

This was previously handled by a GDM patch, but it no longer works reliably with
gnome-sessions systemd session.

We still need the GDM patch to handle running other sessions (eg. plasma).

See NixOS#48255 for more info on the
underlying issue.
@worldofpeace
Copy link
Contributor

It seems there's a patch upstream where they copy the same spawn_session code in GDM to gnome-session
https://github.com/NixOS/nixpkgs/blob/master/pkgs/desktops/gnome-3/core/gdm/reset-environment.patch
https://gitlab.gnome.org/GNOME/gnome-session/merge_requests/24/diffs

@hedning
Copy link
Contributor Author

hedning commented Oct 6, 2019

It seems there's a patch upstream where they copy the same spawn_session code in GDM to gnome-session

Yeah, I tried to patch it like gdm without success (tried with variable_blacklist and variable_unsetlist, though without the unsetenv on logout patch). I find this solution a lot cleaner tbh, and possibly more robust as we don't have to rely on gnome-session cleaning up after itself correctly.

I mean we want gnome-session to always source the environment on startup, so unsetting __NIXOS_SET_ENVIRONMENT_DONE just before running the logging shell is straight forward and makes sense to me.

@worldofpeace worldofpeace merged commit 837cc07 into NixOS:master Oct 7, 2019
@worldofpeace
Copy link
Contributor

@hedning I agree. It's very simple 馃憤

Are there patches we could consolidate in gdm that PAM env makes unneeded?

@hedning hedning deleted the fix-gnome-session-env-setup branch October 7, 2019 05:40
@hedning
Copy link
Contributor Author

hedning commented Oct 12, 2019

So this didn't work completely. While crashing my session a bunch of times testing nixos-rebuild switch I noticed sudo not working again (ie. /run/wrappers/bin not being in the path`).

So I'd say we should work towards getting the pam environment to work correctly and rely on that. Getting the environment sourced correctly from the shell just doesn't work it seems :/

@worldofpeace
Copy link
Contributor

@hedning Are you sure this isn't a side effect of not having #70430 as well?

@hedning
Copy link
Contributor Author

hedning commented Oct 12, 2019

Yeah, the failure I saw was with a436602, but without #70430. I'm fairly certain that the wrapperDir fix actually resolves the issue, just strange that a436602 doesn't work by itself. Perhaps the pam environment somehow takes precedent (though that wouldn't explain why eg. the first login works) 馃槙

I guess with a436602 we can at least be more confident that users will get stuff they've set using environment.variables. gnome-session probably won't clobber random variables, hopefully 馃槅

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

Successfully merging this pull request may close these issues.

None yet

2 participants