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

modules/nixos: implement session variables #16

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

Conversation

nezia1
Copy link

@nezia1 nezia1 commented Feb 2, 2025

This pull request adds handling of NixOS session variables through environment.d. However, I had questions about a few design decisions that I made:

  1. One of the shortcomings of NixOS when it comes to managing environment variables with environment.d is that if said variable is already defined in environment.variables, the user variables defined will not take precedence over the NixOS option. One of the solutions I've found was to explicitely unset it in environment.extraInit, but the issue with that is that it unsets it for the entire system, which is a no-go for a home management solution. I did add a forceOverride option in environment.nix as draft, but I'd like to get input on that.

  2. Is systemd ultimately even what we want to manage session variables? I found it extremely convenient, but it makes for a. a solution tied to systemd in general, which is probably fine for NixOS since it heavily depends on systemd and b. not portable to other systems.

I'd also like to gather input on the code, ie. if I misplaced some options that should go into common.nix.

@nezia1 nezia1 force-pushed the implement-environment-variables branch 3 times, most recently from 419a4b9 to f9a2fc1 Compare February 2, 2025 21:23
@nezia1 nezia1 changed the title modules/nixos: implement user environment variables modules/nixos: implement session variables Feb 3, 2025
@nezia1 nezia1 force-pushed the implement-environment-variables branch 2 times, most recently from 3186453 to 800cf2a Compare February 3, 2025 01:24
@nezia1
Copy link
Author

nezia1 commented Feb 3, 2025

I have additionally added a no systemd version, under this branch. This is a simpler implementation that just creates ~/.profile, and seemed to work just fine. Keep in mind that I am using greetd, which does source ~/.profile, so this might not work on other display managers.

I have also tested this with zsh and fish, and it seemed to set the variables just fine.

@NotAShelf
Copy link
Contributor

Given this is an addition to the NixOS module, and that NixOS does in fact use systemd. It feels like a safer bet to expect ~/.config/environment.d rather than ~/.profile. Shame we cannot do this with PAM directly, but @eclairevoyant and I have agreed that non-systemd support within the NixOS module is not quite a priority.

@eclairevoyant
Copy link
Member

The envvar shadowing is unfortunate, but I can't think of a better solution at this time for that.

@nezia1
Copy link
Author

nezia1 commented Feb 3, 2025

The envvar shadowing is unfortunate, but I can't think of a better solution at this time for that.

It definitely is, I'm not sure how else we could do it. So far this seems to be the only solution if we do want to use environment.d. I think putting it under a mkEnableOption is probably the best we could do. Perhaps I should document it better, and say that this will in fact override everything globally even though it might be only one user that has a conflicting variable?

@nezia1 nezia1 marked this pull request as ready for review February 4, 2025 10:59
@nezia1 nezia1 force-pushed the implement-environment-variables branch from 800cf2a to 2894e0d Compare February 4, 2025 13:19
@nezia1
Copy link
Author

nezia1 commented Feb 4, 2025

The concatStringsSep was previously not turning values inside of the list into strings. Fixed

@nezia1 nezia1 force-pushed the implement-environment-variables branch from 2894e0d to 2045bcf Compare February 4, 2025 13:38
@nezia1
Copy link
Author

nezia1 commented Feb 4, 2025

@eclairevoyant This should be done. What do you think? Not too sure about the type handling in toEnv, but as we only have int path str we should be okay.

modules/nixos/default.nix Outdated Show resolved Hide resolved
modules/nixos/environment.nix Outdated Show resolved Hide resolved
modules/nixos/environment.nix Outdated Show resolved Hide resolved
modules/nixos/environment.nix Outdated Show resolved Hide resolved
modules/nixos/environment.nix Outdated Show resolved Hide resolved
modules/nixos/environment.nix Outdated Show resolved Hide resolved
@nezia1 nezia1 force-pushed the implement-environment-variables branch from 2045bcf to f8b3fb8 Compare February 5, 2025 18:32
nezia1 added a commit to nezia1/flocon that referenced this pull request Feb 9, 2025
nezia1 added a commit to nezia1/flocon that referenced this pull request Feb 9, 2025
nezia1 added a commit to nezia1/flocon that referenced this pull request Feb 9, 2025
This commit adds the module I made for hjem (feel-co/hjem#16). Needs to be removed after it gets merged.
nezia1 added a commit to nezia1/flocon that referenced this pull request Feb 13, 2025
This commit adds the module I made for hjem (feel-co/hjem#16). Needs to be removed after it gets merged.
@nezia1 nezia1 force-pushed the implement-environment-variables branch from f8b3fb8 to 5dbca2d Compare February 15, 2025 00:36
modules/nixos/environment.nix Outdated Show resolved Hide resolved
modules/nixos/environment.nix Outdated Show resolved Hide resolved
@nezia1 nezia1 force-pushed the implement-environment-variables branch from 5dbca2d to ee96255 Compare February 15, 2025 00:51
@nezia1 nezia1 requested a review from NotAShelf February 15, 2025 00:54
@nezia1 nezia1 force-pushed the implement-environment-variables branch from ee96255 to 9704dad Compare February 15, 2025 00:55
@nezia1 nezia1 force-pushed the implement-environment-variables branch from 9704dad to d943655 Compare February 17, 2025 11:22
@nezia1
Copy link
Author

nezia1 commented Feb 17, 2025

The suggestion @NotAShelf mentioned (making sure the file is sourced only once per interactive non-login child shells) has been implemented. Let me know if there is anything that doesn't look right

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.

3 participants