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

Jump to bottom

Introduce environment.profileRelativeSessionVariables #68492

Merged
merged 11 commits into from Sep 18, 2019
Merged

Introduce environment.profileRelativeSessionVariables #68492

merged 11 commits into from Sep 18, 2019

Conversation

worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Sep 11, 2019 •
edited

Motivation for this change

Fixes the general issue found in #68383. This has been useful for every display-manager.

Thanks @rycee for 5736489 it was magically just the commit I needed.
Originates from #67942.

This will allow me to fix elementary/greeter#318 and hopefully update it to its latest release.

Things to test for display-managers

  • lightdm greeters have proper theming

  • correct cursor theme is used in gdm

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 @

@worldofpeace worldofpeace added this to the 20.03 milestone Sep 11, 2019
@rycee
Copy link
Member

rycee commented Sep 11, 2019

I'm glad the changes in #67942 are more generally useful 👍

Originally I didn't have a profileRelativeSessionVariables option and instead simply put profileRelativeEnvVars in the pam configuration with $HOME and $USER replaced by @{HOME} and @{PAM_USER}. Not sure it if it perhaps would be good to return to that idea. I do kind of like separating the PAM and the shell variables, though since it would allow people to use the pam variable syntax.

@worldofpeace
Copy link
Contributor Author

Originally I didn't have a profileRelativeSessionVariables option and instead simply put profileRelativeEnvVars in the pam configuration with $HOME and $USER replaced by @{HOME} and @{PAM_USER}. Not sure it if it perhaps would be good to return to that idea. I do kind of like separating the PAM and the shell variables, though since it would allow people to use the pam variable syntax.

I actually like it this way, I think I'd expect profileRelativeEnvVars to just be in the shell.
A further idea here is to use sessionVariables everywhere 😄

@worldofpeace
Copy link
Contributor Author

We currently have it setup for environment.variables to always have whatever is set in config.environment.sessionVariables.

nixpkgs/nixos/modules/config/shells-environment.nix

Line 158 in f595f28

environment.variables = config.environment.sessionVariables;

Should we do the same for environment.profileRelativeEnvVars and only set environment.profileRelativeSessionVariables everywhere?

@worldofpeace worldofpeace force-pushed the pam-sessionvariables branch 2 times, most recently from 9a50b4a to 276d62f Compare September 13, 2019 16:13
This was referenced Sep 14, 2019
Merged
Merged
michaelpj
michaelpj approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

This looks great!

Does this activate early enough that systemd system services will get it? I'm sure I've had problems before due to them not having XDG_DATA_DIRS set properly.

nixos/modules/config/shells-environment.nix
@@ -157,6 +157,8 @@ in
# terminal instead of logging out of X11).
environment.variables = config.environment.sessionVariables;

environment.profileRelativeEnvVars = config.environment.profileRelativeSessionVariables;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this seemed unnecesssary, then I read the comment on the assignment above. Maybe move this up a line so it obviously belongs with the previous one.

nixos/modules/programs/environment.nix
@@ -30,7 +32,7 @@ in
];

# TODO: move most of these elsewhere
environment.profileRelativeEnvVars =
environment.profileRelativeSessionVariables =
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be using environment.sessionVariables for the assignments to environment.variables above as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to migrate all uses of environment.variables to sessionVariables where appropriate (in another PR).
Also, I think that would infinitely recurse.

nixos/modules/config/system-environment.nix
environment.profileRelativeSessionVariables = mkOption {
type = types.attrsOf (types.listOf types.str);
example = { PATH = [ "/bin" ]; MANPATH = [ "/man" "/share/man" ]; };
description = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have some guidance about when to use this versus environment.profileRelativeEnvVars (likewise sessionVariables vs environment.variables). I'm not sure where that should live, though.

nixos/modules/services/x11/display-managers/gdm.nix Show resolved Hide resolved
nixos/modules/services/x11/display-managers/sddm.nix Show resolved Hide resolved
@worldofpeace
Copy link
Contributor Author

Does this activate early enough that systemd system services will get it? I'm sure I've had problems before due to them not having XDG_DATA_DIRS set properly.

I did systemctl --user show-environment and it has the profile relative environment variables.

@michaelpj
Copy link
Contributor

I did systemctl --user show-environment and it has the profile relative environment variables.

I guess it's too much to hope that it'll get into systemctl show-environment for the system services too. I guess those inherit from systemd itself which probably starts earlier.

@worldofpeace
lightdm-mini-greeter: put xgreeters in passthru
5669b06
@worldofpeace
nixos/environment: set GTK_DATA_PREFIX
cc12581 
Many desktop environment modules are already setting
this so it already makes sense to just do this globally.
@worldofpeace
nixos/environment: set GTK_EXE_PREFIX
6663a79
@worldofpeace worldofpeace merged commit ee29fbd into NixOS:master Sep 18, 2019
@worldofpeace worldofpeace deleted the pam-sessionvariables branch September 18, 2019 15:15
@michaelpj michaelpj mentioned this pull request Sep 26, 2019
Closed
@worldofpeace worldofpeace mentioned this pull request Oct 12, 2019
Closed
@hedning hedning mentioned this pull request Oct 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ttuegel ttuegel ttuegel left review comments

@michaelpj michaelpj michaelpj approved these changes

@hedning hedning Awaiting requested review from hedning

@jtojnar jtojnar Awaiting requested review from jtojnar

@mnacamura mnacamura Awaiting requested review from mnacamura

@ocharles ocharles Awaiting requested review from ocharles

@eadwu eadwu Awaiting requested review from eadwu

Assignees
No one assigned
Labels
6.topic: emacs 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

wingpanel isn't visible in greeter on NixOS
4 participants
@worldofpeace @rycee @michaelpj @ttuegel