fish: source hm-session-vars.sh only for interactive shells

[stasjok]

Description

Unlike bash fish executes all commands from config.fish. Bash executes .bash_profile or .profile for login shells and .bashrc for interactive shells. In my opinion environment variables should be set only for login shells. Simple fish command shouldn't change global variables like PATH. Right now in bash hm-session-vars.sh is sourced in .profile (login shells). Also it is sourced in .bashrc, but only when targets.genericLinux.enable is enabled:

home-manager/modules/targets/generic-linux.nix

Lines 66 to 71 in cbacdab

	# We need to source both nix.sh and hm-session-vars.sh as noted in
	# https://github.com/nix-community/home-manager/pull/797#issuecomment-544783247
	programs.bash.initExtra = ''
	. "${nixPkg}/etc/profile.d/nix.sh"
	. "${profileDirectory}/etc/profile.d/hm-session-vars.sh"
	'';

The linked comment mentions a tmux issue, but it's not the case at least for modern tmux: in tmux by default shell is a login shell.

If you are wary that it won't be sourced in some cases, we can change condition to status is-interactive instead.

I'm pretty sure that one of the conditions (is-login or is-interactive) are desirable, because commands like fish -c '<something>' should be as quick as possible, because many tools may need to run some shell commands and they are using $SHELL variable for it. Right now in home-manager fish -c exit is pretty slow (30 ms vs 8 ms for me) because fenv is slow (it runs external commands like bash/sed/tr multiple times).

Checklist

• Change is backwards compatible.

• Code formatted with ./format.

• Code tested through nix-shell --pure tests -A run.all.

• Test cases updated/added. See example.

• Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

• If this PR adds a new module

  • Added myself as module maintainer. See example.

  • Added myself and the module files to .github/CODEOWNERS.

[stasjok]

Even though it works perfectly well for me only for login shells, I changed it to interactive shells just in case. I think running shell in graphical environment often will not be a login shell. But It should be a responsibility of the graphical environment to load env vars.

With is-interactive it should be backwards compatible with previous behavior in practical scenarios.

[jian-lin]

IIUC, the issue you deal with is the slowdown of setting env vars at shell initialization caused by fenv.

Your approach is to only set those env vars for interactive or login shells, which may cause those env vars not being set when needed, e.g. non-login and interactive shells started by gnome-terminal and non-login and non-interactive shells for running commands through ssh as described in my related pr for zsh.

I propose a better approach: replace fenv with babelfish. babelfish can translate shell scripts to native fish scripts, which can then be sourced. See its pr in nixpkgs for more info. In this way, the slowdown issue is solved and commands setting env var need not be guarded by is-login or is-interactive because those commands will immediately return if not needed.

[stale]

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

• GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
• If it is unfinished but you plan to finish it, please mark it as a draft.
• If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
• To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

• If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

[jian-lin]

Since #4012, fenv is replaced by babelfish, which reduces the runtime of fish -c exit from 40ms to 10ms on my machine.

I think this can be closed.

[stasjok]

OK, then.