search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
16.49k stars 12.99k forks source link

NixOS modules should not set environment variables to executable names available in $PATH #311207

Open amozeo opened 1 month ago

amozeo commented 1 month ago

Describe the bug

Executable names should not be used in environent variables, expected to be always available in PATH. Using nix-shell --pure --keep EDITOR where EDITOR environment variable is nvim, may make some software (like ranger file manager) that rely on thiese variables fail to run executable.

for these executables, which are already present on the system, /run/current-system/sw/bin directory should be used as an alternative.

I have found that these NixOS modules use executable names in environment variables

This issue was originally merged with https://github.com/NixOS/nixpkgs/issues/312224 however I believe a separating it from this issue is better.

Steps To Reproduce

Steps to reproduce

#### Steps to reproduce the behaviour of dependency being in PATH 1. set an environment variable `SOMETHING` using `export` command to an executable that you have installed in system - `export SOMETHING=nvim` 2. verify that you can run it using environment variable - `$SOMETHING` 3. `nix-shell -p hello --pure --keep SOMETHING` 4. notice that you cannot run the executable through environment variable - `$SOMETHING` *(bash shell is used here, but I am sure it will be reproducible with other shells)*

Add a :+1: reaction to issues you find important.

Atemu commented 1 month ago

When updating system using nixos-rebuild switch or nixos-rebuild test, environment variables usually are not being updated in running processess and user sessions, and in order for them to be updated, a session relog or system restart would need to happen.

Thus, environement variables could contain outdated store path (or even non existing store paths after nix-collect-garbage --delete-old), and that's the reason why nix store paths should not be used in environment variables and be actively discouraged.

This I agree with. We should never put Nix store paths into session env vars.

Executable names should not be used in environemt variables too, expected to be always available in PATH. For example, using nix-shell --pure --keep EDITOR where EDITOR environment variable is nvim, may make some software (like ranger file manager) that rely on this variable fail to run executable.

This I do not agree with. Discovering executables via $PATH is a standard unix mechanism and we should not break it.

nix-shell --pure's explicit purpose is to unset env vars such as $PATH. If the env var was set to an absolute path like you suggest, commands ran inside nix-shell --p nvim -I nixpkgs=... --keep EDITOR would still use editor from the system which I think breaks the principle of least surprise as I'd expect it to use nvim from the shell here. IMHO it is also expected behaviour for software dependant on $PATH to break if $PATH is mangled; that's how it should work.

The difference between the absolute path and exe name is forcing the usage of the system instance of the editor vs. using the one from the current context. Unless we have a very good reason to force the system instance, I think we should do the normal thing and use $PATH.

I'd like to bracket out removing nix store paths from session variables from this issue as it should be uncontroversial. Direct references have no place in a context that may outlive the reference's target; everyone who has used a systems programming language knows that.

There is some nuance on what to replace the store path reference with though: Whether you should use $PATH or an absolute path to the sw bin instead of the store path depends on whether the referenced program should sensibly be available globally or only concerns certain isolated components. In the latter case, an absolute path should be preferred, in the former the exe name.

For instance, fcitx should probably not be available in the gloabl $PATH and not be changed depending on $PATH context as it's more or less a component of the desktop session and should probably not change when entering a nix shell.
As a counter example, the disnix client should be in the $PATH as it's supposed to be invoked by the user and also taken from $PATH as the user may want to be able to override it.
(Disclaimer: I am making assumptions about these two components here.)

jian-lin commented 1 month ago

environement variables could contain outdated store path (or even non existing store paths after nix-collect-garbage --delete-old)

Are you sure? My experiment shows that nix-collect-garbage keeps all store paths referenced by environment variables.

Atemu commented 1 month ago

Are you sure? My experiment shows that nix-collect-garbage keeps all store paths referenced by environment variables.

Indeed you can check sudo nix-store --gc --print-roots to see which paths have an implicit GC root on them and where it comes from (i.e. a process' env, exe or maps).

Still, such stale references can cause other issues and annoyances like requiring logging out for some changes to actually apply.

amozeo commented 1 month ago

environement variables could contain outdated store path (or even non existing store paths after nix-collect-garbage --delete-old)

Are you sure? My experiment shows that nix-collect-garbage keeps all store paths referenced by environment variables.

my bad, this was my bad assumption on how nix behaves.

amozeo commented 1 month ago

For instance, fcitx should probably not be available in the gloabl $PATH and not be changed depending on $PATH context as it's more or less a component of the desktop session and should probably not change when entering a nix shell.

for non executable references to the nix store and for executable refrences to the nix store that are not in /run/current_system/sw/bin I had idea to make a new directory under system build, in similar way how etc is, to store symlinks to the nix store, which would stay the same even if those references to nix store after nixos-rebuild test would be updated.

Atemu commented 1 month ago

for non executable references to the nix store and for executable refrences to the nix store that are not in /run/current_system/sw/bin

That's actually a good point; those can exist. Executables that should be uniform globally but should not be in the global $PATH.

I believe that's what the libexec convention is for, so /run/current-system/sw/libexec/ would be the logical place to put these.

I had idea to make a new directory under system build, in similar way how etc is, to store symlinks to the nix store, which would stay the same even if those references to nix store after nixos-rebuild test would be updated.

This part I don't understand. How would they "stay the same"? Bound by what scope?

The difficulty here is that the first set must get there in some way but a boot is indistinguishable from a subsequent activation (I believe).