[shellenv] fix PATH nesting for devbox-project and devbox-global #1508

savil · 2023-09-28T04:29:22Z

Summary

Problem:
High level problem description:

A devbox-global binary is used when executing a devbox script.
- For example, devbox global add [email protected] adds bin-wrappers for sh and bash. This sh wrapper is used to execute the devbox script.
The devbox-global binary (example, sh) needs to re-evaluate its own environment
As a result, the devbox-global binary amends the PATH, but in doing so it may drop the devbox script's PATH elements, leading to errors because the devbox-project's binaries cannot be found.

Cause
In our implementation of shellenv, we save an "OG Path" in an env-var. This is the PATH prior to entering devbox shell. We then calculate the print-dev-env PATH. Finally, we construct the PATH for the shellenv as <print-dev-env paths>:<og-paths>.

However, if another devbox-project has introduced additional path-elements in PATH, those will get dropped. Because when this shellenv is re-evaluated: it will do PATH=<new nixEnvPath>:<og-path>.

Solution:
Develop a PATH_STACK that can track which path parts come from which devbox-projects.

So, for a devbox-project nested within a shell that is using devbox-global, the PATH_STACK looks like {<devbox-project-nix-env-path>, <devbox-global-nix-env-path>, <devbox-init-path>}

earlier parts get priority, just like PATH elements
devbox-init-path, is analogous to og-path, but not identical: this PATH_STACK applies to all devbox projects within this shell. The og-path was specific to a devbox-project.

Now, if the global shellenv is re-evaluated, then the <devbox-global-nix-env-path> value is overidden, but the PATH_STACK structure is maintained.

Finally, we construct PATH for shellenv as PATH=<devbox-project-nix-env-path>:<devbox-global-nix-env-path>:<devbox-init-path>.

How was it tested?

add unit test for envpath.Stack

TODO: fill in.

devbox global add bash doesn't lead to errors with devbox run build.
Inside a devbox add vim && devbox shell, then doing devbox rm vim doesn't give the nix store path i.e. [Bug]: devbox shellenv shouldn't prepend to existing path #687 is still fixed.
Adhoc test:

mkdir -p foo-hello/bar-cowsay
cd foo-hello && devbox init && devbox add hello
cd bar-cowsay && devbox init && devbox add cowsay

cd ../..
eval $(devbox -c foo-hello shellenv)
hello
eval $(devbox -c foo-hello/bar-cowsay shellenv)
cowsay wow

# before, this would drop the bar-cowsay bin-wrappers and nix-profile from PATH
eval $(devbox -c foo-hello shellenv)
cowsay works

Previously, the last "cowsay works" would fail
https://gist.github.com/savil/2c6e5f86208ff95ae54e717c17c794d6

savil · 2023-09-28T04:29:33Z

Current dependencies on/for this PR:

main
- PR [fish] do not print hash -r in shellenv #1516
  - PR [shellenv] fix PATH nesting for devbox-project and devbox-global #1508 👈

This comment was auto-generated by Graphite.

savil · 2023-09-29T04:43:44Z

@mikeland73 have a look please. I think the main approach is in place. I needed to move to a struct to keep the logic encapsulated in both nixEnv and envvars.go's IsEnvEnabled.

Not 100% confident about all the places I added false /*pathStackInPlace*/. Need to audit them again.
I also need to make another pass over the comments in the pathstack.go file. Feel free to point out nits, or suggested changes.

internal/impl/envpath/pathstack.go

mikeland73

Doing RC for flag name and changing the piping logic so we don't have to pass a boolean everwhere.

I think the flag should the exception behavior. So no flag means environment gets stacked on, yes flag means path stack is kept in tact.

I'll review new envpath package in parallel

In parallel

mikeland73 · 2023-09-29T19:08:03Z

internal/boxcli/shellenv.go

+	command.Flags().BoolVar(
+		&flags.pure, "path-stack-in-place", true,
+		"If true, Devbox will not give top priority to the PATH of this project's shellenv. "+


I thought the name we agreed on was something like preserve-path-stack

Why is it defaulting to true? Should be false. I'm confused by the name maybe

oh yeah, I was struggling to recall the name we'd agreed on. I can change it.

ugh, it should default to false.

The PR's state is a bit subpar b/c my fish shell has gotten into a bad state and I've been having problems running programs. So, sent this for prelim feedback.

mikeland73 · 2023-09-29T19:09:12Z

devbox.go

@@ -20,10 +20,10 @@ type Devbox interface {
 	Config() *devconfig.Config
 	EnvVars(ctx context.Context) ([]string, error)
 	Info(ctx context.Context, pkg string, markdown bool) (string, error)
-	Install(ctx context.Context) error
+	Install(ctx context.Context, pathStackInPlace bool) error


To really reduce the size and file touches of this PR, you can add this to devopt.Opts instead. And also avoid adding boolean params.

I can do that but I feel kinda mixed about it.

We're not eliminating the boolean param, but sending it via a somewhat global state.

We're not eliminating the boolean param, but sending it via a somewhat global state.

Yeah, but I see the utility in not complicating the core funciton interfaces for this edge-case handling.

Yeah agree devbox options are semi-global (not quite global as this PR can attest)

But it really makes everything else easier to read and the risk is minimal (specifically a single devbox object being used multiple times in a single command which is a pattern we don't use at all)

internal/impl/envpath/pathstack.go

mikeland73 · 2023-09-29T20:05:42Z

internal/boxcli/shellenv.go

+		"If true, Devbox will not give top priority to the PATH of this project's shellenv. "+
+			"This project's place in the path stack will be unchanged.")


Seggested description:

"Preserves existing PATH order if this project's environment is already in PATH. Useful if you want to avoid overshadowing another devbox project that is already active"

mikeland73 · 2023-09-29T20:15:47Z

internal/impl/envpath/pathstack.go

+		// Add this key only if absent from the stack
+		!lo.Contains(s.keys, key) {
+
+		s.keys = lo.Uniq(slices.Insert(s.keys, 0, key))


totally get why you chose Insert over append (so that we keep same order as needed by JoinPathLists) but I always think of golang slices as natural stacks where append can be used as a natural push

yes! I totally get you.

I first implemented stack.keys with later values having higher priority. Because append is a natural push, as you say.

But then changed to have the highest-priority be the zeroth index, because it aligns the path-stack's golang and string representations with the expanded form $PATH variable. This makes debugging much easier, when we dump out these values in fmt.Printf or echo $DEVBOX_PATH_STACK or echo $PATH.

savil

@mikeland73 made updates as requested. Also, renamed envpath.go to envpath/pathlists.go and moved its unit test to this package.

I still need to write a unit test for envpath.stack.

I'm also thinking it may make sense to wait on landing this until Remove Nixpkgs and the Script Exit on Error are released. To minimize risk.

savil · 2023-09-29T20:22:03Z

internal/impl/envpath/pathstack.go

+		// Add this key only if absent from the stack
+		!lo.Contains(s.keys, key) {
+
+		s.keys = lo.Uniq(slices.Insert(s.keys, 0, key))


yes! I totally get you.

I first implemented stack.keys with later values having higher priority. Because append is a natural push, as you say.

But then changed to have the highest-priority be the zeroth index, because it aligns the path-stack's golang and string representations with the expanded form $PATH variable. This makes debugging much easier, when we dump out these values in fmt.Printf or echo $DEVBOX_PATH_STACK or echo $PATH.

mikeland73 · 2023-09-29T20:53:14Z

internal/impl/devbox.go

+	env = pathStack.PushAndUpdateEnv(env, d.projectDirHash(), nixEnvPath, d.preservePathStack)
+	debug.Log("New path stack is: %s", pathStack)


I thought I had written this comment previously, but github seems to have swallowed it. Here's a suggestion for an interface I find a bit easier to follow:

pathStack := envpath.Stack(env) if !d.preservePathStack || !pathStack.Has(d.projectDirHash()) { pathStack.PushAndDedup(d.projectDirHash(), nixEnvPath) } env["PATH"] = pathStack.Path()

Benefits:

Remove preserve logic from stack

All code is concentrated together.

Split out push from computing the path

Change Has to take the hash instead of key. key can now be private

I like this.

Another benefit:

we move out handling env["PATH"]

we can later have an envpath.Path struct that encapsulates further a lot of the complexity of this function, that then calls the envpath.Stack as shown here.

There is a problem with this. We need to update the env-vars for the nixEnvPathKey regardless of the if-condition. And I think that should be inside the stack, since its internal logic.

So, a fix could be:

pathStack := envpath.Stack(env) if !d.preservePathStack || !pathStack.Has(d.projectDirHash()) { pathStack.PushAndDedup(d.projectDirHash(), nixEnvPath) } env[envpath.Key(d.projectDirHash())] = nixEnvPath // exposes internal logic of pathstack. env["PATH"] = pathStack.Path()

but my concern would be that it breaks encapsulation and exposes internal logic.

Good catch. Yeah, agree that it's not great to expose the internals.

In that case maybe just put the stack creation and use next to each other?

yup, did that.

Actually we could still do:

pathStack := envpath.Stack(env) pathStack.PushAndUpdateEnv(d.projectDirHash(), nixEnvPath, d.preservePathStack) env := pathStack.Env()

I'm going to land as-is, so can focus on other work.

Changing to .Env() I have some concerns with:

We'll be making a copy of env internally in the constructor. Then applying that over any changes between the constructor and doing pathStack.Env() could potentially lead to issues, since it can lose information.

I prefer the existing design so that:

Internal state is restricted to the path-stack keys. Other state is directly stored in env that is passed in.

The PATH is updated externally. That enables us adding a separate data structure to abstract the PATH handling in this function (future work).

Either way, don't feel strongly and we can iterate on it.

gcurtis

I know you guys will probably disagree, but the Go-ish way would be to make the zero-value do what the constructor is doing:

type Stack struct {
    keys []string
}

// Remove func Stack()

func (s *Stack) Push(env map[string]string, ...) {
    if s.keys == nil {
        // do what Stack() was doing
    }

    // continue with PushAndUpdateEnv
}

Usage:

var s envpath.Stack
s.Push(...)

gcurtis · 2023-09-29T21:05:02Z

internal/impl/envpath/pathstack.go

+		env[InitPathEnv] = env["PATH"]
+	}
+	return &stack{
+		keys: strings.Split(stackEnv, ":"),


Not sure if this matters, but keys will be a slice with a single empty string if stackEnv is an empty string.

good point! I can check for stackEnv being an empty string

gcurtis · 2023-09-29T21:11:08Z

internal/impl/envvars.go

-	return os.Getenv(d.ogPathKey()) != ""
+	env := map[string]string{}
+	for _, keyVal := range os.Environ() {
+		parts := strings.Split(keyVal, "=")


Use strings.Cut, otherwise you'll lose the ends of variables that have an = in them. I think you could also export envir.mapToPairs and reuse it here.

oh i like that. Done!

internal/impl/envpath/pathstack.go

mikeland73 · 2023-09-29T22:45:17Z

@gcurtis I don't think dislike this pattern at all! I think it's cool. But it doesn't quite work because the stack needs to be initialized with env before calling Push

In that case the struct would contain an Env field.

One downside to not initializing is that now you need to have the initialization code in every public method.

gcurtis · 2023-09-29T22:55:34Z

@mikeland73 it seems like the Push method could do that, no? I was thinking of it as:

Push the initial environment
Push the devbox environment
Push the nested devbox environment
Etc.

I don't think initializing the slice is too bad if the number of methods is small. It matches how types like strings.Builder, bytes.Buffer, list.List, etc. behave.

Edit: @savil sorry for adding noise to your PR! Just wanted to be clear that I fully support merging this as-is.

mikeland73 · 2023-09-29T23:56:48Z

@gcurtis I think we're mixing pushing environment and pushing the path (we call the path nixEnvPath which is confusing).

What gets pushed on the stack is PATH fragments. But we also need to pass in the full environment because the stack itself is stored in env variables, as well as another env variable that stores the stack keys.

I guess we can push the environment AND the path in each push. But I think initializing with env makes it nicer (see #1508 (comment))

savil force-pushed the savil/shellenv-path branch 4 times, most recently from ab2109f to e4411e5 Compare September 29, 2023 04:28

savil requested a review from mikeland73 September 29, 2023 04:41

savil marked this pull request as ready for review September 29, 2023 04:43

savil force-pushed the savil/shellenv-path branch from e4411e5 to af9beed Compare September 29, 2023 18:19

savil commented Sep 29, 2023

View reviewed changes

internal/impl/envpath/pathstack.go Outdated Show resolved Hide resolved

mikeland73 requested changes Sep 29, 2023

View reviewed changes

savil changed the base branch from main to savil/fish-no-hash September 29, 2023 19:55

savil force-pushed the savil/shellenv-path branch from af9beed to fca3dab Compare September 29, 2023 19:55

savil mentioned this pull request Sep 29, 2023

[fish] do not print hash -r in shellenv #1516

Merged

mikeland73 reviewed Sep 29, 2023

View reviewed changes

internal/impl/envpath/pathstack.go Outdated Show resolved Hide resolved

internal/impl/envpath/pathstack.go Outdated Show resolved Hide resolved

internal/impl/envpath/pathstack.go Outdated Show resolved Hide resolved

Base automatically changed from savil/fish-no-hash to main September 29, 2023 20:01

mikeland73 approved these changes Sep 29, 2023

View reviewed changes

savil commented Sep 29, 2023

View reviewed changes

mikeland73 reviewed Sep 29, 2023

View reviewed changes

gcurtis approved these changes Sep 29, 2023

View reviewed changes

savil added 7 commits September 29, 2023 17:04

[shellenv] fix PATH nesting for devbox-project and devbox-global

21657b2

update tests

346da63

move to envpath.Stack struct

0cbc154

add flag to bin-wrappers to keep path stack in place

df4820e

rename to preservePathStack

53e6022

remove bool param, and pass as Devbox option

198fba4

remove moar pathStackInPlace

d970e2a

savil added 6 commits September 29, 2023 17:04

rename file for pathlists

983f3ab

requested changes in envpath

5d4ec61

update flag description

aac1a93

rename file to envpath/stack.go

4b36838

requested changes by Greg and Mike

bd4a98a

add stack test

ee49755

savil force-pushed the savil/shellenv-path branch from de13cf7 to ee49755 Compare September 30, 2023 00:11

savil added 3 commits October 2, 2023 11:42

update dependency

67b941d

fix bug with stack updating wrong env

92c0585

lint fix

038b760

savil merged commit c11031a into main Oct 2, 2023

savil deleted the savil/shellenv-path branch October 2, 2023 20:23

		"If true, Devbox will not give top priority to the PATH of this project's shellenv. "+
		"This project's place in the path stack will be unchanged.")

		env = pathStack.PushAndUpdateEnv(env, d.projectDirHash(), nixEnvPath, d.preservePathStack)
		debug.Log("New path stack is: %s", pathStack)

[shellenv] fix PATH nesting for devbox-project and devbox-global #1508

[shellenv] fix PATH nesting for devbox-project and devbox-global #1508

Uh oh!

Conversation

savil commented Sep 28, 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 commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

savil commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gcurtis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikeland73 commented Sep 29, 2023

savil commented Sep 28, 2023 •

edited

Loading

savil commented Sep 28, 2023 •

edited

Loading

savil commented Sep 29, 2023 •

edited

Loading

gcurtis commented Sep 29, 2023 •

edited

Loading

mikeland73 commented Sep 29, 2023 •

edited

Loading