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

Add sessionVariablesOnce to home-env, bash and zsh #6594

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stevenroose
Copy link

Description

There are some issues about users that expect their environment variables to change when they change their sessionVariables in their config files. I am one of them.

I learned twice the hard way that they are only sourced once per session and for me this is quite unintuitive. I thought sessions always start fresh and all variables are set at the beginning always.

F.e. when opening a new terminal with zsh, the sessionVariables are not set after they are changed, but only after logging out and in entirely.

In fact, bash did not have the same behavior, making me think that maybe zsh shouldn't even have this in the first place. Happy to amend this change to drop the "only once" behavior from zsh program entirely.

As for home session vars, I think it might make sense, and I added a field explicitly for this purpose, sessionVariablesOnce. This solution was suggested here by user @Bujiraso.

Checklist

  • Change is backwards compatible.

Unfortunately, people expecting the Once behavior for their regular variables might end up with very long variables in the end, but since this is mostly used for PATH-like variables, this will not break any functionality. So IMO this is backwards compatible enough.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all
    or nix build --reference-lock-file flake.lock ./tests#test-all using Flakes.

They both seem broken. They error on stuff totally unrelated to my changes.

Maintainer CC

@rycee (no maintainers for programs.zsh)

@Bujiraso
Copy link

Bujiraso commented Mar 9, 2025

Cool!

@ambroisie
Copy link
Contributor

I personally don't really like this change, as I think it should be a user expectation/documentation issue rather than introducing more complexity to the module's API. On the other hand I can understand that restarting one's session can be an annoying friction.


One note: this should move all variables that currently use sessionVariables to sessionVariablesOnce for backwards compatibility (and/or individually review each usage to check whether it should be set once or would be harmless if set on each shell startup).

As an example, PATH and other sessionSearchVariables should only be set once, or you could quickly accumulate duplicate entires.

@khaneliman
Copy link
Collaborator

khaneliman commented Mar 11, 2025

I personally don't really like this change, as I think it should be a user expectation/documentation issue rather than introducing more complexity to the module's API. On the other hand I can understand that restarting one's session can be an annoying friction.

One note: this should move all variables that currently use sessionVariables to sessionVariablesOnce for backwards compatibility (and/or individually review each usage to check whether it should be set once or would be harmless if set on each shell startup).

As an example, PATH and other sessionSearchVariables should only be set once, or you could quickly accumulate duplicate entires.

Yeah, I kind of agree at this point of maturity of the ecosystem... it'd make more sense to introduce a different option that fulfills the new role instead of randomly changing the behavior of the existing option.

That said, I originally was also caught by surprise of the behavior of the variables not updating and got frustrated by it many times.... I just worry about the effect of the change, as a maintainer.

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

Successfully merging this pull request may close these issues.

4 participants