[CLI] devbox shell --pure #1084

mohsenari · 2023-06-01T22:15:34Z

Summary

I added a --pure flag for devbox shell to create an isolated env without variables leaking from the parent shell.

Q: Should I add a --pure flag for devbox run and devbox shellenv?
A: Yes
Q2: Because the created shell is "pure" the devbox command is also not available inside the pure shell. Should that be an exception?
A: Yes

Note: in pure mode, the $HOME env variable has to leak in because the devbox binary relies on it.

How was it tested?

compile
export FOO=bar
./devbox shell --pure
echo $FOO should show no value
Also ./devbox run --pure -- echo $FOO should show no value

savil

Yes, I do think we need the flag for shellenv and run
And yes, I do think we need to ensure devbox binary is included.

Could we add some testscript unit-tests for pure versus impure?

Some prelim comments. Need to go to a meeting....

internal/boxcli/shell.go

internal/impl/devbox.go

savil

nice!

Yes, I do think we need the flag for shellenv and run
And yes, I do think we need to ensure devbox binary is included.

Could we add some testscript unit-tests for pure versus impure?

Some prelim comments. Need to go to a meeting....

internal/impl/shell.go

savil

Nice, this is looking more promising. I had a few thoughts inline, and below:

1.CreateWrappers sets up the wrapper.sh.tmpl which calls shellenv: https://github.com/jetpack-io/devbox/blob/main/internal/wrapnix/wrapper.sh.tmpl#L29.

I tried reasoning thru whether we need to invoke --pure here as well, or not. But this is pretty confusing when I think of various scenarios (devbox add within a pure shell etc.), but my head is hurting now.

@mikeland86 thoughts?

I think we need the .envrc file that is generated for direnv to also incorporate --pure? Maybe in a follow up PR...
When I start devbox shell --pure, I get:

 (mohsen--pure-shell)> devbox shell --pure
Starting a devbox shell...

Error: We found a /nix directory but nix binary is not in your PATH and we were not able to find it in the usual locations. Your nix installation might be broken. If restarting your terminal or reinstalling nix doesn't work, please create an issue at https://github.com/jetpack-io/devbox/issues

My nix is in /nix/var/nix/profiles/default/bin (IIRC its from the default devbox install). Do we need to ensure that this is in PATH? One solution is to make the following edit inside computeNixEnv function:

        currentEnvPath := env["PATH"]
+       if currentEnvPath == "" {
+               currentEnvPath = "/nix/var/nix/profiles/default/bin"
+       }

devbox add doesn't seem to work for me inside a pure shell:

(devbox) Savil-Srivastavas-MacBook-Pro:devbox savil$ devbox add ripgrep

Error: There was an error installing nix packages
source: exec: "git": executable file not found in $PATH

(devbox) Savil-Srivastavas-MacBook-Pro:devbox savil$

internal/impl/devbox.go

internal/impl/packages.go

internal/impl/shell.go

mikeland73

This PR as is makes me a bit nervous. Like @savil said, it's difficult to reason about what needs pure vs impure environments. Additionally adding boolean params everywhere makes code more difficult to read (and 1 boolean leads to 2, 3.....)

Open to ideas, but I think we can make 2 changes that will reduce the size of this PR a bunch.

Remove pure from CreateWrappers and just create the symlink unconditionally. I'm not sure this is the right place, but it's simple.
Change devbox.Open(dir string, writer io.Writer) to be devbox.Open(dir string, writer io.Writer, devbox.Opts{}) and add pure as a field in the devbox struct. This way you don't need to pass pure everywhere. You have it when you need it. The only downside to this is that you can't have a single devbox object produce pure and impure outcomes, but I think thats OK.

devbox.go

internal/impl/devbox.go

mikeland73 · 2023-06-08T19:25:01Z

internal/impl/devbox.go

 	env["PATH"] = JoinPathLists(nixEnvPath, originalPath)
 	debug.Log("computed environment PATH is: %s", env["PATH"])

 	d.setCommonHelperEnvVars(env)


I'm unsure if these should go in pure or not. cc: @gcurtis

I think we still want LD_LIBRARY_PATH and LIBRARY_PATH, but without the current environment's values appended. Since it looks like env["LIBRARY_PATH"] and env["LD_LIBRARY_PATH"] will always be empty, I think this is okay as-is.

internal/impl/shell.go

internal/wrapnix/wrapper.go

mohsenari · 2023-06-08T21:50:37Z

@mikeland86 @savil
I made an overhaul to the PR and here is the list of changes to make it easier to review:

I removed pure being passed as a function argument. I added it to the Devbox struct and set it when devbox.Open() is called.
But that means I had to update all devbox.Open() calls, most of them get an empty devbox.Opts{}. So I hope that's ok.
In NewDevboxShell() I had to pass it as a function argument because of the way ShellOptions is implemented.
I also removed --pure-eval from nix eval since I couldn't find justification to have it added.
I added $USER and $DISPLAY along with $HOME as env variables being passed through to match what Nix does.
Finally, added Nix's bin path (/nix/var/nix/profiles/default/bin) to the path in pure shell because it was needed for commands like devbox add to work inside a pure shell. It is hardcoded but I think it's ok because it's the same path in all unix operating systems.

devbox.go

mikeland73 · 2023-06-08T21:53:34Z

internal/impl/devbox.go


 	currentEnvPath := env["PATH"]
+	if d.pure { // make nix available inside pure shell - necessary for devbox add to work
+		currentEnvPath = "/nix/var/nix/profiles/default/bin"


Is this needed? devbox should not need nix in path to work

@mikeland86 It is! you can test it. In a pure shell, devbox add throws this error:

Error: We found a /nix directory but nix binary is not in your PATH and we were not able to find it in the usual locations. Your nix installation might be broken. If restarting your terminal or reinstalling nix doesn't work, please create an issue at https://github.com/jetpack-io/devbox/issues

hm, OK. I'm not 100% sure this works in all cases (e.g. single user installs).

I just tested and single user on ubuntu installed:

$ which nix /home/mike/.nix-profile/bin/nix

See https://github.com/jetpack-io/devbox/blob/main/internal/nix/source.go#L32-L32

For some reason, we are not sourcing nix correctly. In the pure shell, is __ETC_PROFILE_NIX_SOURCED=1 set?

If that is set, then we may need a different approach.

One possible approach is to look in $PATH and try to find:

/nix/var/nix/profiles/default/bin
OR
$HOME/.nix-profile/bin/nix

and keep them around.

There's also :

xdg.StateSubpath("nix/profile/etc/profile.d/nix.sh"), xdg.StateSubpath("nix/profiles/profile/etc/profile.d/nix.sh"),

I'm not sure if nix also stores bins there.

So in short, I think you need to look in path for any of these:

/nix/var/nix/profiles/default/bin $HOME/.nix-profile/bin xdg.StateSubpath(/nix/profile/bin) xdg.StateSubpath(nix/profiles/profile/bin)

If any of those are in PATH, then they need to be retained. In practice it will only be one of them.

I'm not 100% sure there are no other nix paths, but this should cover all the cases we currently consider.

devbox.go

mikeland73 · 2023-06-08T21:55:48Z

internal/impl/shell.go

+	if !pure {
+		// First, check the SHELL environment variable.
+		path = os.Getenv(envir.Shell)
+		if path != "" {
+			debug.Log("Using SHELL env var for shell binary path: %s\n", path)
+			return path, nil
+		}


I'm not 100% sure we want to ignore the SHELL variable. Which shell we use is does not modify the environment. If a user wants to do SHELL=zsh devbox run something in CICD it won't work.

Thoughts @gcurtis @Lagoja @loreto ?

I think here we should ignore it and always use Nix's bash shell. IMO the shell can have a pretty big impact on the environment.

I agree with the use-case of letting the user override the shell, but it might make more sense to do that through a new devbox.json field. That way it can be a Nix-installed shell that's guaranteed to be the same for everyone.

internal/impl/shell.go

internal/wrapnix/wrapper.go

internal/impl/shell.go

mohsenari · 2023-06-08T22:28:22Z

@mikeland86 Addressed the nit feedback. Not sure about the SHELL variable and about d.setCommonHelperEnvVars(env)

mikeland73

I think all my last round of comments can be done in a follow up. The only semi-urgent one is to search PATH for a few places nix binary can be and not just a single hard coded one. This gives us a better chance of finding it in all types of installations.

Since this PR is already really large, you should merge and do a fast follow with a fix for that. Other nits can be addressed after that.

devbox.go

internal/boxcli/devopt/devboxopts.go

mikeland73 · 2023-06-09T20:38:54Z

internal/impl/devbox.go


 	currentEnvPath := env["PATH"]
+	if d.pure { // make nix available inside pure shell - necessary for devbox add to work
+		currentEnvPath = "/nix/var/nix/profiles/default/bin"


hm, OK. I'm not 100% sure this works in all cases (e.g. single user installs).

I just tested and single user on ubuntu installed:

$ which nix /home/mike/.nix-profile/bin/nix

See https://github.com/jetpack-io/devbox/blob/main/internal/nix/source.go#L32-L32

For some reason, we are not sourcing nix correctly. In the pure shell, is __ETC_PROFILE_NIX_SOURCED=1 set?

If that is set, then we may need a different approach.

One possible approach is to look in $PATH and try to find:

/nix/var/nix/profiles/default/bin
OR
$HOME/.nix-profile/bin/nix

and keep them around.

There's also :

xdg.StateSubpath("nix/profile/etc/profile.d/nix.sh"), xdg.StateSubpath("nix/profiles/profile/etc/profile.d/nix.sh"),

I'm not sure if nix also stores bins there.

So in short, I think you need to look in path for any of these:

/nix/var/nix/profiles/default/bin $HOME/.nix-profile/bin xdg.StateSubpath(/nix/profile/bin) xdg.StateSubpath(nix/profiles/profile/bin)

If any of those are in PATH, then they need to be retained. In practice it will only be one of them.

I'm not 100% sure there are no other nix paths, but this should cover all the cases we currently consider.

internal/wrapnix/wrapper.go

savil

Much nicer!

As some followups, I think we will want to enable a direnv user to opt-into pure mode

internal/impl/devbox.go

internal/boxcli/shellenv.go

savil · 2023-06-09T21:13:25Z

It would also be great to add some testscript unit-tests under testscripts/ for the --pure case. Not blocking landing this PR, can be done in a followup.

mohsenari · 2023-06-09T22:19:40Z

Merging this now, will followup with another PR addressing nix in PATH issue.

## Summary This is a follow up PR from as a result of [this discussion](#1084 (comment)). 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/bin` respectively. Also added tests for devbox run pure mode. For `devbox shellenv`, pure mode doesn't really make sense to write tests for, and for `devbox shell` it's not possible to write testscript tests. ## How was it tested? - in a single user nix installed env, run `./devbox shell --pure` - `devbox add which` - `which nix` should point to `~/.nix-profile/bin/nix` - in a multi-user nix, same test should point to `/nix/var/nix/profiles/default/bin/nix`

mohsenari requested review from gcurtis and savil June 6, 2023 01:31

mohsenari marked this pull request as ready for review June 6, 2023 01:32

savil reviewed Jun 6, 2023

View reviewed changes

internal/boxcli/shell.go Outdated Show resolved Hide resolved

internal/impl/devbox.go Outdated Show resolved Hide resolved

savil reviewed Jun 6, 2023

View reviewed changes

internal/impl/shell.go Outdated Show resolved Hide resolved

mohsenari force-pushed the mohsen--pure-shell branch from 79aa6a9 to f2d3b84 Compare June 7, 2023 19:52

mohsenari requested a review from savil June 7, 2023 20:01

savil reviewed Jun 8, 2023

View reviewed changes

internal/impl/devbox.go Outdated Show resolved Hide resolved

internal/impl/devbox.go Outdated Show resolved Hide resolved

internal/impl/packages.go Outdated Show resolved Hide resolved

internal/impl/shell.go Outdated Show resolved Hide resolved

savil requested a review from mikeland73 June 8, 2023 04:34

mikeland73 requested changes Jun 8, 2023

View reviewed changes

mohsenari requested review from savil and mikeland73 June 8, 2023 21:47

mikeland73 reviewed Jun 8, 2023

View reviewed changes

mohsenari requested a review from mikeland73 June 8, 2023 22:28

mikeland73 approved these changes Jun 9, 2023

View reviewed changes

savil approved these changes Jun 9, 2023

View reviewed changes

internal/impl/devbox.go Outdated Show resolved Hide resolved

internal/boxcli/shellenv.go Show resolved Hide resolved

mohsenari added 10 commits June 9, 2023 18:02

First draft for pure shell

3e36386

Added --pure-eval to pure shell

d1faa3a

Fixed small bug

f7afa40

Fixed passing pure to ensurepackagesareinstalled

0c3d563

Added symlink to have devbox in pure shell

debef7c

Added pure to devbox run

fee752a

Added pure to shellenv

2af74f1

Allowed HOME to leak to pure shell

5d1c3e7

fixed missing arg for update

5d83b0f

Change pure flag to be devbox param

32261da

mohsenari added 8 commits June 9, 2023 18:04

Addressed nit feedback

b1bf767

renamed short param

d606f37

Consolidated devbox.Open params into devopt package

f615980

Updated pure flag description

71e3831

Moved boxcli/devopt to impl/devopt

b3141b5

Removed commented code

5f30cc6

Fixed lint error

e739154

Addressing lint error again

5cd0413

mohsenari force-pushed the mohsen--pure-shell branch from d13545a to 5cd0413 Compare June 9, 2023 22:04

Removed unused struct

f85b7b3

mohsenari merged commit 3764425 into main Jun 9, 2023

mohsenari deleted the mohsen--pure-shell branch June 9, 2023 22:19

mohsenari mentioned this pull request Jun 12, 2023

[pure-shell] making sure nix is in path in pure mode #1143

Merged

[CLI] devbox shell --pure #1084

[CLI] devbox shell --pure #1084

Uh oh!

Conversation

mohsenari commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

savil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikeland73 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

gcurtis Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mohsenari commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mikeland73 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

mohsenari Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeland73 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikeland73 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

gcurtis Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mohsenari commented Jun 8, 2023

Uh oh!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikeland73 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mohsenari commented Jun 1, 2023 •

edited

Loading

savil left a comment •

edited

Loading

mohsenari commented Jun 8, 2023 •

edited

Loading

mohsenari Jun 8, 2023 •

edited

Loading

savil commented Jun 9, 2023 •

edited

Loading