[pure-shell] making sure nix is in path in pure mode #1143
Conversation
There was a problem hiding this comment.
@mohsenari see nix XDG code: NixOS/nix@2384d36
(I have not tested it, but I assume the goal was to put the nix binaries in the custom XDG dir)
internal/impl/devbox.go
Outdated
| if d.pure { // make nix available inside pure shell - necessary for devbox commands to work | ||
| singleUserNixBin := fmt.Sprintf("%s/.nix-profile/bin", env["HOME"]) | ||
| multiUserNixBin := "/nix/var/nix/profiles/default/bin" | ||
| // ~/.nix-profile/bin takes precedent because of this from Nix reference manual: | ||
| // "You generally wouldn’t have /nix/var/nix/profiles/some-profile/bin in your PATH. | ||
| // Rather, there is a symlink ~/.nix-profile that points to your current profile. | ||
| // This means that you should put ~/.nix-profile/bin in your PATH | ||
| // (and indeed, that’s what the initialisation script /nix/etc/profile.d/nix.sh does)." | ||
| currentEnvPath = fmt.Sprintf("%s:%s", singleUserNixBin, multiUserNixBin) |
There was a problem hiding this comment.
From offline chat: IMO we should only keep stuff in path if it's already there and not add it if it's missing to avoid polluting the path.
There was a problem hiding this comment.
Fair enough. I updated the PR after reading more about xdg stuff and tested the PR in a fresh ubuntu install with XDG_STATE_HOME set to an arbitrary dir, and then installing nix.
| // handling special cases to for pure shell | ||
| // Passing HOME USER and DISTPLAY for pure shell to leak through | ||
| // otherwise devbox binary won't work - this matches nix | ||
| if !d.pure || key == "HOME" || key == "USER" || key == "DISPLAY" || key == "PATH" { |
There was a problem hiding this comment.
What's the motivation for including PATH even in --pure mode?
PATH seems to be one of the critical env-vars that we definitely want to omit from the external environment for pure mode, because then all sorts of external programs become callable inside the pure shell.
There was a problem hiding this comment.
Oh I see. Could you add a small comment like:
// We include PATH to find the nix installation. It is cleaned for pure mode below.
?
internal/impl/devbox.go
Outdated
| continue | ||
| } | ||
| // handling special cases to for pure shell | ||
| // Passing HOME USER and DISTPLAY for pure shell to leak through |
| return err | ||
| } | ||
|
|
||
| func (d *Devbox) convertEnvToMap(currentEnv []string) (map[string]string, error) { |
There was a problem hiding this comment.
This function is doing much more than converting env to map. We should name it accordingly. It sounds like a generic transformation. Always err on the side of over-specifying.
| defaultSingleUserNixBin := fmt.Sprintf("%s/.nix-profile/bin", env["HOME"]) | ||
| defaultMultiUserNixBin := "/nix/var/nix/profiles/default/bin" | ||
| xdgNixBin := xdg.StateSubpath("/nix/profile/bin") | ||
| pathElements := strings.Split(env["PATH"], ":") |
| } | ||
|
|
||
| func findNixInPATH(env map[string]string) (string, error) { | ||
| defaultSingleUserNixBin := fmt.Sprintf("%s/.nix-profile/bin", env["HOME"]) |
There was a problem hiding this comment.
This should use os.UserHomeDir
| for _, el := range pathElements { | ||
| if el == xdgNixBin || | ||
| el == defaultSingleUserNixBin || | ||
| el == defaultMultiUserNixBin { | ||
| return el, nil | ||
| } |
There was a problem hiding this comment.
I sort of think we should include all that are specified instead of just the first. It's not a big deal, but it would be more defensive code.
There was a problem hiding this comment.
@mikeland73 isn't your suggestion contradicting your comment here?
## Summary This is a follow up to [this conversation](#1143 (comment)) and [this conversation](#1170 (review)). 1. Renamed `ConvertEnvToMap` function to a more precise name. 2. Added pure_shell.go file under `impl` package and moved related functions to pure shell there. 3. Moved call to function that creates devbox symlink outside of CreateWrappers. ## How was it tested? - compile - devbox shell --pure - inside pure shell run devbox commands (e.g., `devbox version`) to confirm devbox is available inside pure shell.
Summary
This is a follow up PR from as a result of this discussion.
Note, xdg dirs don't keep any nix executables as far as I know. Single user and multi user nix installation put the nix executable in
~/.nix-profile/bin/and/nix/var/nix/profiles/default/binrespectively.Also added tests for devbox run pure mode. For
devbox shellenv, pure mode doesn't really make sense to write tests for, and fordevbox shellit's not possible to write testscript tests.How was it tested?
./devbox shell --puredevbox add whichwhich nixshould point to~/.nix-profile/bin/nix/nix/var/nix/profiles/default/bin/nix