From 21657b2707e77153c8dfaafc8f47c70e79408f26 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Wed, 27 Sep 2023 21:28:30 -0700 Subject: [PATCH 01/16] [shellenv] fix PATH nesting for devbox-project and devbox-global --- internal/impl/devbox.go | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index b295bcdbbb3..5348aad804a 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -784,16 +784,14 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m currentEnvPath := env["PATH"] debug.Log("current environment PATH is: %s", currentEnvPath) - // Use the original path, if available. If not available, set it for future calls. - // See https://github.com/jetpack-io/devbox/issues/687 - // We add the project dir hash to ensure that we don't have conflicts - // between different projects (including global) - // (moving a project would change the hash and that's fine) - originalPath, ok := env[d.ogPathKey()] + + pathStackEnv, ok := env["PATH_STACK"] if !ok { + // if path stack is empty, then set the first element + pathStackEnv = d.ogPathKey() env[d.ogPathKey()] = currentEnvPath - originalPath = currentEnvPath } + debug.Log("path stack is: %s", pathStackEnv) vaf, err := d.nix.PrintDevEnv(ctx, &nix.PrintDevEnvArgs{ FlakesFilePath: d.nixFlakesFilePath(), @@ -899,7 +897,25 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - env["PATH"] = JoinPathLists(nixEnvPath, originalPath) + // The PathStack enables: + // 1. Tracking which sub-paths in PATH come from which devbox-project + // 2. The priority order of these sub-paths: devbox-projects encountered later get higher priority. + pathStack := strings.Split(pathStackEnv, ":") + nixEnvPathKey := "DEVBOX_NIX_ENV_PATH_" + d.projectDirHash() + if !lo.Contains(pathStack, nixEnvPathKey) { + // if pathStack does not contain this project's nixEnvPath already, + // then we add it + pathStack = append(pathStack, nixEnvPathKey) + } + // Store the nixEnvPath for this devbox-project. This lets us replace this sub-path in the eventual PATH, + // without affecting the sub-paths from other devbox-projects. + env[nixEnvPathKey] = nixEnvPath + env["PATH_STACK"] = strings.Join(pathStack, ":") + + // Look up the path-list for each path-stack element, and join them together to get the final PATH. + pathLists := lo.Map(pathStack, func(part string, idx int) string { return env[part] }) + env["PATH"] = JoinPathLists(lo.Reverse(pathLists)...) // reverse to give priority to the most recent project + debug.Log("computed environment PATH is: %s", env["PATH"]) d.setCommonHelperEnvVars(env) From 346da630993025829caebcb048104f185d591c4d Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Thu, 28 Sep 2023 10:54:42 -0700 Subject: [PATCH 02/16] update tests --- internal/impl/devbox_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/impl/devbox_test.go b/internal/impl/devbox_test.go index c0945d71637..97b9cb063b5 100644 --- a/internal/impl/devbox_test.go +++ b/internal/impl/devbox_test.go @@ -89,6 +89,7 @@ func TestComputeNixPathIsIdempotent(t *testing.T) { "DEVBOX_OG_PATH_"+devbox.projectDirHash(), env["DEVBOX_OG_PATH_"+devbox.projectDirHash()], ) + t.Setenv("PATH_STACK", env["PATH_STACK"]) env, err = devbox.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") @@ -112,6 +113,7 @@ func TestComputeNixPathWhenRemoving(t *testing.T) { "DEVBOX_OG_PATH_"+devbox.projectDirHash(), env["DEVBOX_OG_PATH_"+devbox.projectDirHash()], ) + t.Setenv("PATH_STACK", env["PATH_STACK"]) devbox.nix.(*testNix).path = "" env, err = devbox.computeNixEnv(ctx, false /*use cache*/) From 0cbc1548789ed9feb7cbb2b350af77b5368e3c82 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Thu, 28 Sep 2023 18:16:14 -0700 Subject: [PATCH 03/16] move to envpath.Stack struct --- internal/impl/devbox.go | 48 ++++------------- internal/impl/devbox_test.go | 17 +++--- internal/impl/envpath/envpath.go | 37 +++++++++++++ internal/impl/envpath/pathstack.go | 86 ++++++++++++++++++++++++++++++ internal/impl/envvars.go | 10 +++- internal/impl/shell.go | 31 ----------- internal/impl/shell_test.go | 3 +- 7 files changed, 152 insertions(+), 80 deletions(-) create mode 100644 internal/impl/envpath/envpath.go create mode 100644 internal/impl/envpath/pathstack.go diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 5348aad804a..b046425972a 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -21,6 +21,7 @@ import ( "github.com/pkg/errors" "github.com/samber/lo" "go.jetpack.io/devbox/internal/devpkg" + "go.jetpack.io/devbox/internal/impl/envpath" "go.jetpack.io/devbox/internal/impl/generate" "go.jetpack.io/devbox/internal/searcher" "go.jetpack.io/devbox/internal/shellgen" @@ -782,16 +783,10 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m } } - currentEnvPath := env["PATH"] - debug.Log("current environment PATH is: %s", currentEnvPath) + debug.Log("current environment PATH is: %s", env["PATH"]) - pathStackEnv, ok := env["PATH_STACK"] - if !ok { - // if path stack is empty, then set the first element - pathStackEnv = d.ogPathKey() - env[d.ogPathKey()] = currentEnvPath - } - debug.Log("path stack is: %s", pathStackEnv) + pathStack := envpath.NewStack(env) + debug.Log("path stack is: %s", pathStack) vaf, err := d.nix.PrintDevEnv(ctx, &nix.PrintDevEnvArgs{ FlakesFilePath: d.nixFlakesFilePath(), @@ -851,13 +846,13 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m addEnvIfNotPreviouslySetByDevbox(env, pluginEnv) - env["PATH"] = JoinPathLists( + env["PATH"] = envpath.JoinPathLists( nix.ProfileBinPath(d.projectDir), env["PATH"], ) if !d.OmitBinWrappersFromPath { - env["PATH"] = JoinPathLists( + env["PATH"] = envpath.JoinPathLists( filepath.Join(d.projectDir, plugin.WrapperBinPath), env["PATH"], ) @@ -897,24 +892,7 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - // The PathStack enables: - // 1. Tracking which sub-paths in PATH come from which devbox-project - // 2. The priority order of these sub-paths: devbox-projects encountered later get higher priority. - pathStack := strings.Split(pathStackEnv, ":") - nixEnvPathKey := "DEVBOX_NIX_ENV_PATH_" + d.projectDirHash() - if !lo.Contains(pathStack, nixEnvPathKey) { - // if pathStack does not contain this project's nixEnvPath already, - // then we add it - pathStack = append(pathStack, nixEnvPathKey) - } - // Store the nixEnvPath for this devbox-project. This lets us replace this sub-path in the eventual PATH, - // without affecting the sub-paths from other devbox-projects. - env[nixEnvPathKey] = nixEnvPath - env["PATH_STACK"] = strings.Join(pathStack, ":") - - // Look up the path-list for each path-stack element, and join them together to get the final PATH. - pathLists := lo.Map(pathStack, func(part string, idx int) string { return env[part] }) - env["PATH"] = JoinPathLists(lo.Reverse(pathLists)...) // reverse to give priority to the most recent project + env = pathStack.AddToEnv(env, d.projectDirHash(), nixEnvPath) debug.Log("computed environment PATH is: %s", env["PATH"]) @@ -922,7 +900,7 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m if !d.pure { // preserve the original XDG_DATA_DIRS by prepending to it - env["XDG_DATA_DIRS"] = JoinPathLists(env["XDG_DATA_DIRS"], os.Getenv("XDG_DATA_DIRS")) + env["XDG_DATA_DIRS"] = envpath.JoinPathLists(env["XDG_DATA_DIRS"], os.Getenv("XDG_DATA_DIRS")) } for k, v := range d.env { @@ -957,10 +935,6 @@ func (d *Devbox) nixEnv(ctx context.Context) (map[string]string, error) { return d.computeNixEnv(ctx, usePrintDevEnvCache) } -func (d *Devbox) ogPathKey() string { - return "DEVBOX_OG_PATH_" + d.projectDirHash() -} - func (d *Devbox) nixPrintDevEnvCachePath() string { return filepath.Join(d.projectDir, ".devbox/.nix-print-dev-env-cache") } @@ -1136,8 +1110,8 @@ var ignoreDevEnvVar = map[string]bool{ // common setups (e.g. gradio, rust) func (d *Devbox) setCommonHelperEnvVars(env map[string]string) { profileLibDir := filepath.Join(d.projectDir, nix.ProfilePath, "lib") - env["LD_LIBRARY_PATH"] = JoinPathLists(profileLibDir, env["LD_LIBRARY_PATH"]) - env["LIBRARY_PATH"] = JoinPathLists(profileLibDir, env["LIBRARY_PATH"]) + env["LD_LIBRARY_PATH"] = envpath.JoinPathLists(profileLibDir, env["LD_LIBRARY_PATH"]) + env["LIBRARY_PATH"] = envpath.JoinPathLists(profileLibDir, env["LIBRARY_PATH"]) } // NixBins returns the paths to all the nix binaries that are installed by @@ -1213,7 +1187,7 @@ func (d *Devbox) parseEnvAndExcludeSpecialCases(currentEnv []string) (map[string return nil, err } includedInPath = append(includedInPath, dotdevboxBinPath(d)) - env["PATH"] = JoinPathLists(includedInPath...) + env["PATH"] = envpath.JoinPathLists(includedInPath...) } return env, nil } diff --git a/internal/impl/devbox_test.go b/internal/impl/devbox_test.go index 97b9cb063b5..1467f1ab18a 100644 --- a/internal/impl/devbox_test.go +++ b/internal/impl/devbox_test.go @@ -14,6 +14,7 @@ import ( "github.com/bmatcuk/doublestar/v4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.jetpack.io/devbox/internal/impl/envpath" "go.jetpack.io/devbox/internal/devconfig" "go.jetpack.io/devbox/internal/envir" @@ -85,11 +86,9 @@ func TestComputeNixPathIsIdempotent(t *testing.T) { assert.NotEmpty(t, path, "path should not be nil") t.Setenv("PATH", path) - t.Setenv( - "DEVBOX_OG_PATH_"+devbox.projectDirHash(), - env["DEVBOX_OG_PATH_"+devbox.projectDirHash()], - ) - t.Setenv("PATH_STACK", env["PATH_STACK"]) + t.Setenv(envpath.InitPathEnv, env[envpath.InitPathEnv]) + t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv]) + t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())]) env, err = devbox.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") @@ -109,11 +108,9 @@ func TestComputeNixPathWhenRemoving(t *testing.T) { assert.Contains(t, path, "/tmp/my/path", "path should contain /tmp/my/path") t.Setenv("PATH", path) - t.Setenv( - "DEVBOX_OG_PATH_"+devbox.projectDirHash(), - env["DEVBOX_OG_PATH_"+devbox.projectDirHash()], - ) - t.Setenv("PATH_STACK", env["PATH_STACK"]) + t.Setenv(envpath.InitPathEnv, env[envpath.InitPathEnv]) + t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv]) + t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())]) devbox.nix.(*testNix).path = "" env, err = devbox.computeNixEnv(ctx, false /*use cache*/) diff --git a/internal/impl/envpath/envpath.go b/internal/impl/envpath/envpath.go new file mode 100644 index 00000000000..a432a73807e --- /dev/null +++ b/internal/impl/envpath/envpath.go @@ -0,0 +1,37 @@ +package envpath + +import ( + "path/filepath" + "strings" +) + +// JoinPathLists joins and cleans PATH-style strings of +// [os.ListSeparator] delimited paths. To clean a path list, it splits it and +// does the following for each element: +// +// 1. Applies [filepath.Clean]. +// 2. Removes the path if it's relative (must begin with '/' and not be '.'). +// 3. Removes the path if it's a duplicate. +func JoinPathLists(pathLists ...string) string { + if len(pathLists) == 0 { + return "" + } + + seen := make(map[string]bool) + var cleaned []string + for _, path := range pathLists { + for _, path := range filepath.SplitList(path) { + path = filepath.Clean(path) + if path == "." || path[0] != '/' { + // Remove empty paths and don't allow relative + // paths for security reasons. + continue + } + if !seen[path] { + cleaned = append(cleaned, path) + } + seen[path] = true + } + } + return strings.Join(cleaned, string(filepath.ListSeparator)) +} diff --git a/internal/impl/envpath/pathstack.go b/internal/impl/envpath/pathstack.go new file mode 100644 index 00000000000..5b8b4ea7ada --- /dev/null +++ b/internal/impl/envpath/pathstack.go @@ -0,0 +1,86 @@ +package envpath + +import ( + "strings" + + "github.com/samber/lo" + "golang.org/x/exp/slices" +) + +const ( + PathStackEnv = "DEVBOX_PATH_STACK" + + // InitPathEnv stores the path prior to any devbox shellenv modifying the environment + InitPathEnv = "DEVBOX_INIT_PATH" +) + +// Stack has the following design: +// 1. The PathStack enables tracking which sub-paths in PATH come from which devbox-project +// 2. What it stores: The PathStack is an ordered-list of nixEnvPathKeys +// 3. Each nixEnvPathKey is set as an env-var with the value of the nixEnvPath for that devbox-project. +// 4. The final PATH is reconstructed by concatenating the env-var values of each nixEnvPathKey env-var. +// 5. The Stack is stored in its own env-var shared by all devbox-projects in this shell. +type Stack struct { + + // keys holds the stack elements. + // Earlier (lower index number) keys get higher priority. + // This keeps the string representation of the Stack aligned with the PATH value. + keys []string +} + +func NewStack(env map[string]string) *Stack { + stackEnv, ok := env[PathStackEnv] + if !ok { + // if path stack is empty, then push the current PATH, which is the + // external environment prior to any devbox-shellenv being applied to it. + stackEnv = InitPathEnv + env[InitPathEnv] = env["PATH"] + } + return &Stack{ + keys: strings.Split(stackEnv, ":"), + } +} + +// String is the value of the Stack stored in its env-var. +func (s *Stack) String() string { + return strings.Join(s.keys, ":") +} + +// Key is the element stored in the Stack for a devbox-project. It represents +// a pointer to the nixEnvPath value stored in its own env-var, also using this same +// Key. +func Key(projectHash string) string { + return "DEVBOX_NIX_" + projectHash +} + +// AddToEnv adds the new nixEnvPath for the devbox-project identified by projectHash to the env. +// It does so by modifying the following env-vars: +// 1. nixEnvPath key +// 2. PathStack +// 3. PATH +func (s *Stack) AddToEnv(env map[string]string, projectHash, nixEnvPath string) map[string]string { + key := Key(projectHash) + + // Add this nixEnvPath to env + env[key] = nixEnvPath + + // Add this key to the stack b/c earlier keys get priority + s.keys = lo.Uniq(slices.Insert(s.keys, 0, key)) + env[PathStackEnv] = s.String() + + // Look up the paths-list for each paths-stack element, and join them together to get the final PATH. + pathLists := lo.Map(s.keys, func(part string, idx int) string { return env[part] }) + env["PATH"] = JoinPathLists(pathLists...) + return env +} + +// Has tests if the stack has the specified key. Refer to the Key function for constructing +// the appropriate key for any devbox-project. +func (s *Stack) Has(key string) bool { + for _, k := range s.keys { + if k == key { + return true + } + } + return false +} diff --git a/internal/impl/envvars.go b/internal/impl/envvars.go index 1f0701dcff5..ab258b9ab33 100644 --- a/internal/impl/envvars.go +++ b/internal/impl/envvars.go @@ -7,6 +7,8 @@ import ( "os" "slices" "strings" + + "go.jetpack.io/devbox/internal/impl/envpath" ) const devboxSetPrefix = "__DEVBOX_SET_" @@ -98,5 +100,11 @@ func markEnvsAsSetByDevbox(envs ...map[string]string) { // as a proxy for this. This allows us to differentiate between global and // individual project shells. func (d *Devbox) IsEnvEnabled() bool { - return os.Getenv(d.ogPathKey()) != "" + env := map[string]string{} + for _, keyVal := range os.Environ() { + parts := strings.Split(keyVal, "=") + env[parts[0]] = parts[1] + } + pathStack := envpath.NewStack(env) + return pathStack.Has(envpath.Key(d.projectDirHash())) } diff --git a/internal/impl/shell.go b/internal/impl/shell.go index a8c398c77e0..8385ea3d7a2 100644 --- a/internal/impl/shell.go +++ b/internal/impl/shell.go @@ -387,37 +387,6 @@ func (s *DevboxShell) linkShellStartupFiles(shellSettingsDir string) { } } -// JoinPathLists joins and cleans PATH-style strings of -// [os.ListSeparator] delimited paths. To clean a path list, it splits it and -// does the following for each element: -// -// 1. Applies [filepath.Clean]. -// 2. Removes the path if it's relative (must begin with '/' and not be '.'). -// 3. Removes the path if it's a duplicate. -func JoinPathLists(pathLists ...string) string { - if len(pathLists) == 0 { - return "" - } - - seen := make(map[string]bool) - var cleaned []string - for _, path := range pathLists { - for _, path := range filepath.SplitList(path) { - path = filepath.Clean(path) - if path == "." || path[0] != '/' { - // Remove empty paths and don't allow relative - // paths for security reasons. - continue - } - if !seen[path] { - cleaned = append(cleaned, path) - } - seen[path] = true - } - } - return strings.Join(cleaned, string(filepath.ListSeparator)) -} - func filterPathList(pathList string, keep func(string) bool) string { filtered := []string{} for _, path := range filepath.SplitList(pathList) { diff --git a/internal/impl/shell_test.go b/internal/impl/shell_test.go index aeeae510a77..5bd59aaba1a 100644 --- a/internal/impl/shell_test.go +++ b/internal/impl/shell_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "go.jetpack.io/devbox/internal/impl/envpath" "go.jetpack.io/devbox/internal/shellgen" ) @@ -125,7 +126,7 @@ func TestCleanEnvPath(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := JoinPathLists(test.inPath) + got := envpath.JoinPathLists(test.inPath) if got != test.outPath { t.Errorf("Got incorrect cleaned PATH.\ngot: %s\nwant: %s", got, test.outPath) } From df4820e1abc14bf733f2004b3460bfc404ff1ff2 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Thu, 28 Sep 2023 20:14:04 -0700 Subject: [PATCH 04/16] add flag to bin-wrappers to keep path stack in place --- devbox.go | 4 ++-- internal/boxcli/install.go | 2 +- internal/boxcli/shell.go | 2 +- internal/boxcli/shellenv.go | 18 ++++++++++------ internal/impl/devbox.go | 34 ++++++++++++++++++------------ internal/impl/devbox_test.go | 10 ++++----- internal/impl/envpath/pathstack.go | 19 ++++++++++++++--- internal/impl/packages.go | 2 +- internal/wrapnix/wrapper.sh.tmpl | 2 +- 9 files changed, 59 insertions(+), 34 deletions(-) diff --git a/devbox.go b/devbox.go index cbdc8c1fd27..77dbad7f6af 100644 --- a/devbox.go +++ b/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 IsEnvEnabled() bool ListScripts() []string - NixEnv(ctx context.Context, includeHooks bool) (string, error) + NixEnv(ctx context.Context, includeHooks, pathStackInPlace bool) (string, error) PackageNames() []string ProjectDir() string Pull(ctx context.Context, opts devopt.PullboxOpts) error diff --git a/internal/boxcli/install.go b/internal/boxcli/install.go index fed39224e8d..5063babdf41 100644 --- a/internal/boxcli/install.go +++ b/internal/boxcli/install.go @@ -39,7 +39,7 @@ func installCmdFunc(cmd *cobra.Command, flags runCmdFlags) error { if err != nil { return errors.WithStack(err) } - if err = box.Install(cmd.Context()); err != nil { + if err = box.Install(cmd.Context(), false /*pathStackInPlace*/); err != nil { return errors.WithStack(err) } fmt.Fprintln(cmd.ErrOrStderr(), "Finished installing packages.") diff --git a/internal/boxcli/shell.go b/internal/boxcli/shell.go index 4cb5cff95f5..4a048a9964a 100644 --- a/internal/boxcli/shell.go +++ b/internal/boxcli/shell.go @@ -66,7 +66,7 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error { if flags.printEnv { // false for includeHooks is because init hooks is not compatible with .envrc files generated // by versions older than 0.4.6 - script, err := box.NixEnv(cmd.Context(), false /*includeHooks*/) + script, err := box.NixEnv(cmd.Context(), false /*includeHooks*/, false /*pathStackInPlace*/) if err != nil { return err } diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index 69ea343a1df..b84a74baf42 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -15,10 +15,11 @@ import ( type shellEnvCmdFlags struct { envFlag - config configFlags - runInitHook bool - install bool - pure bool + config configFlags + runInitHook bool + install bool + pure bool + pathStackInPlace bool } func shellEnvCmd() *cobra.Command { @@ -48,6 +49,11 @@ func shellEnvCmd() *cobra.Command { command.Flags().BoolVar( &flags.pure, "pure", false, "If this flag is specified, devbox creates an isolated environment inheriting almost no variables from the current environment. A few variables, in particular HOME, USER and DISPLAY, are retained.") + 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. "+ + "This project's place in the path stack will be unchanged.") + _ = command.Flags().MarkHidden("path-stack-in-place") flags.config.register(command) flags.envFlag.register(command) @@ -73,12 +79,12 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { } if flags.install { - if err := box.Install(cmd.Context()); err != nil { + if err := box.Install(cmd.Context(), flags.pathStackInPlace); err != nil { return "", err } } - envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook) + envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook, flags.pathStackInPlace) if err != nil { return "", err } diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index b046425972a..0566e62b10b 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -175,7 +175,7 @@ func (d *Devbox) Shell(ctx context.Context) error { return err } - envs, err := d.nixEnv(ctx) + envs, err := d.nixEnv(ctx, false /*pathStackInPlace*/) if err != nil { return err } @@ -219,7 +219,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string return err } - env, err := d.nixEnv(ctx) + env, err := d.nixEnv(ctx, false /*pathStackInPlace*/) if err != nil { return err } @@ -261,11 +261,11 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string // Install ensures that all the packages in the config are installed and // creates all wrappers, but does not run init hooks. It is used to power // devbox install cli command. -func (d *Devbox) Install(ctx context.Context) error { +func (d *Devbox) Install(ctx context.Context, pathStackInPlace bool) error { ctx, task := trace.NewTask(ctx, "devboxInstall") defer task.End() - if _, err := d.NixEnv(ctx, false /*includeHooks*/); err != nil { + if _, err := d.NixEnv(ctx, false /*includeHooks*/, pathStackInPlace); err != nil { return err } return wrapnix.CreateWrappers(ctx, d) @@ -282,7 +282,7 @@ func (d *Devbox) ListScripts() []string { return keys } -func (d *Devbox) NixEnv(ctx context.Context, includeHooks bool) (string, error) { +func (d *Devbox) NixEnv(ctx context.Context, includeHooks, pathStackInPlace bool) (string, error) { ctx, task := trace.NewTask(ctx, "devboxNixEnv") defer task.End() @@ -290,7 +290,7 @@ func (d *Devbox) NixEnv(ctx context.Context, includeHooks bool) (string, error) return "", err } - envs, err := d.nixEnv(ctx) + envs, err := d.nixEnv(ctx, pathStackInPlace) if err != nil { return "", err } @@ -314,7 +314,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { return nil, err } - envs, err := d.nixEnv(ctx) + envs, err := d.nixEnv(ctx, false /*pathStackInPlace*/) if err != nil { return nil, err } @@ -322,7 +322,8 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { } func (d *Devbox) ShellEnvHash(ctx context.Context) (string, error) { - envs, err := d.nixEnv(ctx) + // TODO savil: correct? + envs, err := d.nixEnv(ctx, false /*pathStackInPlace*/) if err != nil { return "", err } @@ -765,7 +766,11 @@ func (d *Devbox) StartProcessManager( // Note that the shellrc.tmpl template (which sources this environment) does // some additional processing. The computeNixEnv environment won't necessarily // represent the final "devbox run" or "devbox shell" environments. -func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (map[string]string, error) { +func (d *Devbox) computeNixEnv( + ctx context.Context, + usePrintDevEnvCache bool, + pathStackInPlace bool, +) (map[string]string, error) { defer trace.StartRegion(ctx, "computeNixEnv").End() // Append variables from current env if --pure is not passed @@ -786,7 +791,7 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m debug.Log("current environment PATH is: %s", env["PATH"]) pathStack := envpath.NewStack(env) - debug.Log("path stack is: %s", pathStack) + debug.Log("Original path stack is: %s", pathStack) vaf, err := d.nix.PrintDevEnv(ctx, &nix.PrintDevEnvArgs{ FlakesFilePath: d.nixFlakesFilePath(), @@ -892,7 +897,8 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - env = pathStack.AddToEnv(env, d.projectDirHash(), nixEnvPath) + env = pathStack.AddToEnv(env, d.projectDirHash(), nixEnvPath, pathStackInPlace) + debug.Log("New path stack is: %s", pathStack) debug.Log("computed environment PATH is: %s", env["PATH"]) @@ -915,7 +921,7 @@ var nixEnvCache map[string]string // nixEnv is a wrapper around computeNixEnv that caches the result. // Note that this is in-memory cache of the final environment, and not the same // as the nix print-dev-env cache which is stored in a file. -func (d *Devbox) nixEnv(ctx context.Context) (map[string]string, error) { +func (d *Devbox) nixEnv(ctx context.Context, pathStackInPlace bool) (map[string]string, error) { defer debug.FunctionTimer().End() if nixEnvCache != nil { return nixEnvCache, nil @@ -932,7 +938,7 @@ func (d *Devbox) nixEnv(ctx context.Context) (map[string]string, error) { usePrintDevEnvCache = true } - return d.computeNixEnv(ctx, usePrintDevEnvCache) + return d.computeNixEnv(ctx, usePrintDevEnvCache, pathStackInPlace) } func (d *Devbox) nixPrintDevEnvCachePath() string { @@ -1119,7 +1125,7 @@ func (d *Devbox) setCommonHelperEnvVars(env map[string]string) { // give name. This matches how nix flakes behaves if there are conflicts in // buildInputs func (d *Devbox) NixBins(ctx context.Context) ([]string, error) { - env, err := d.nixEnv(ctx) + env, err := d.nixEnv(ctx, false /*pathStackInPlace*/) if err != nil { return nil, err } diff --git a/internal/impl/devbox_test.go b/internal/impl/devbox_test.go index 1467f1ab18a..65361ba9112 100644 --- a/internal/impl/devbox_test.go +++ b/internal/impl/devbox_test.go @@ -71,7 +71,7 @@ func TestComputeNixEnv(t *testing.T) { d := devboxForTesting(t) d.nix = &testNix{} ctx := context.Background() - env, err := d.computeNixEnv(ctx, false /*use cache*/) + env, err := d.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) require.NoError(t, err, "computeNixEnv should not fail") assert.NotNil(t, env, "computeNixEnv should return a valid env") } @@ -80,7 +80,7 @@ func TestComputeNixPathIsIdempotent(t *testing.T) { devbox := devboxForTesting(t) devbox.nix = &testNix{"/tmp/my/path"} ctx := context.Background() - env, err := devbox.computeNixEnv(ctx, false /*use cache*/) + env, err := devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) require.NoError(t, err, "computeNixEnv should not fail") path := env["PATH"] assert.NotEmpty(t, path, "path should not be nil") @@ -90,7 +90,7 @@ func TestComputeNixPathIsIdempotent(t *testing.T) { t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv]) t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())]) - env, err = devbox.computeNixEnv(ctx, false /*use cache*/) + env, err = devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) require.NoError(t, err, "computeNixEnv should not fail") path2 := env["PATH"] @@ -101,7 +101,7 @@ func TestComputeNixPathWhenRemoving(t *testing.T) { devbox := devboxForTesting(t) devbox.nix = &testNix{"/tmp/my/path"} ctx := context.Background() - env, err := devbox.computeNixEnv(ctx, false /*use cache*/) + env, err := devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) require.NoError(t, err, "computeNixEnv should not fail") path := env["PATH"] assert.NotEmpty(t, path, "path should not be nil") @@ -113,7 +113,7 @@ func TestComputeNixPathWhenRemoving(t *testing.T) { t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())]) devbox.nix.(*testNix).path = "" - env, err = devbox.computeNixEnv(ctx, false /*use cache*/) + env, err = devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) require.NoError(t, err, "computeNixEnv should not fail") path2 := env["PATH"] assert.NotContains(t, path2, "/tmp/my/path", "path should not contain /tmp/my/path") diff --git a/internal/impl/envpath/pathstack.go b/internal/impl/envpath/pathstack.go index 5b8b4ea7ada..75842f47fcf 100644 --- a/internal/impl/envpath/pathstack.go +++ b/internal/impl/envpath/pathstack.go @@ -58,14 +58,27 @@ func Key(projectHash string) string { // 1. nixEnvPath key // 2. PathStack // 3. PATH -func (s *Stack) AddToEnv(env map[string]string, projectHash, nixEnvPath string) map[string]string { +// +// Returns the modified env map +func (s *Stack) AddToEnv( + env map[string]string, + projectHash string, + nixEnvPath string, + pathStackInPlace bool, +) map[string]string { key := Key(projectHash) // Add this nixEnvPath to env env[key] = nixEnvPath - // Add this key to the stack b/c earlier keys get priority - s.keys = lo.Uniq(slices.Insert(s.keys, 0, key)) + // Common case: ensure this key is at the top of the stack + if !pathStackInPlace || + // Case pathStackInPlace == true, usually from bin-wrapper or (in future) shell hook. + // Add this key only if absent from the stack + !lo.Contains(s.keys, key) { + + s.keys = lo.Uniq(slices.Insert(s.keys, 0, key)) + } env[PathStackEnv] = s.String() // Look up the paths-list for each paths-stack element, and join them together to get the final PATH. diff --git a/internal/impl/packages.go b/internal/impl/packages.go index 8025c0c0554..9251f27eb5c 100644 --- a/internal/impl/packages.go +++ b/internal/impl/packages.go @@ -234,7 +234,7 @@ func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMod } // Force print-dev-env cache to be recomputed. - if _, err := d.computeNixEnv(ctx, false /*use cache*/); err != nil { + if _, err := d.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/); err != nil { return err } diff --git a/internal/wrapnix/wrapper.sh.tmpl b/internal/wrapnix/wrapper.sh.tmpl index 4d2b325a0e8..6f5afd7d5ff 100644 --- a/internal/wrapnix/wrapper.sh.tmpl +++ b/internal/wrapnix/wrapper.sh.tmpl @@ -21,7 +21,7 @@ DO_NOT_TRACK=1 can be removed once we optimize segment to queue events. if [[ "${{ .ShellEnvHashKey }}" != "{{ .ShellEnvHash }}" ]] && [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then export {{ .ShellEnvHashKey }}_GUARD=true -eval "$(DO_NOT_TRACK=1 devbox shellenv -c {{ .ProjectDir }})" +eval "$(DO_NOT_TRACK=1 devbox shellenv --path-stack-in-place -c {{ .ProjectDir }})" fi {{/* From 53e6022c1916dd06ce4c9d847451327759d6cdc6 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 12:43:05 -0700 Subject: [PATCH 05/16] rename to preservePathStack --- internal/boxcli/install.go | 2 +- internal/boxcli/shell.go | 2 +- internal/boxcli/shellenv.go | 18 +++++++++--------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/boxcli/install.go b/internal/boxcli/install.go index 5063babdf41..f14a59ffbd6 100644 --- a/internal/boxcli/install.go +++ b/internal/boxcli/install.go @@ -39,7 +39,7 @@ func installCmdFunc(cmd *cobra.Command, flags runCmdFlags) error { if err != nil { return errors.WithStack(err) } - if err = box.Install(cmd.Context(), false /*pathStackInPlace*/); err != nil { + if err = box.Install(cmd.Context(), false /*preservePathStack*/); err != nil { return errors.WithStack(err) } fmt.Fprintln(cmd.ErrOrStderr(), "Finished installing packages.") diff --git a/internal/boxcli/shell.go b/internal/boxcli/shell.go index 4a048a9964a..c3c01811263 100644 --- a/internal/boxcli/shell.go +++ b/internal/boxcli/shell.go @@ -66,7 +66,7 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error { if flags.printEnv { // false for includeHooks is because init hooks is not compatible with .envrc files generated // by versions older than 0.4.6 - script, err := box.NixEnv(cmd.Context(), false /*includeHooks*/, false /*pathStackInPlace*/) + script, err := box.NixEnv(cmd.Context(), false /*includeHooks*/, false /*preservePathStack*/) if err != nil { return err } diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index b84a74baf42..79f19c2c833 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -15,11 +15,11 @@ import ( type shellEnvCmdFlags struct { envFlag - config configFlags - runInitHook bool - install bool - pure bool - pathStackInPlace bool + config configFlags + runInitHook bool + install bool + pure bool + preservePathStack bool } func shellEnvCmd() *cobra.Command { @@ -50,10 +50,10 @@ func shellEnvCmd() *cobra.Command { command.Flags().BoolVar( &flags.pure, "pure", false, "If this flag is specified, devbox creates an isolated environment inheriting almost no variables from the current environment. A few variables, in particular HOME, USER and DISPLAY, are retained.") command.Flags().BoolVar( - &flags.pure, "path-stack-in-place", true, + &flags.preservePathStack, "preserve-path-stack", false, "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.") - _ = command.Flags().MarkHidden("path-stack-in-place") + _ = command.Flags().MarkHidden("preserve-path-stack") flags.config.register(command) flags.envFlag.register(command) @@ -79,12 +79,12 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { } if flags.install { - if err := box.Install(cmd.Context(), flags.pathStackInPlace); err != nil { + if err := box.Install(cmd.Context(), flags.preservePathStack); err != nil { return "", err } } - envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook, flags.pathStackInPlace) + envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook, flags.preservePathStack) if err != nil { return "", err } From 198fba4173ce5b25366b128ac8427a66bad0ef4e Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 12:49:48 -0700 Subject: [PATCH 06/16] remove bool param, and pass as Devbox option --- internal/boxcli/shellenv.go | 9 +++++---- internal/impl/devbox.go | 26 ++++++++++++-------------- internal/impl/devbox_test.go | 10 +++++----- internal/impl/devopt/devboxopts.go | 1 + internal/impl/envpath/pathstack.go | 8 ++++---- internal/impl/packages.go | 2 +- internal/wrapnix/wrapper.sh.tmpl | 2 +- 7 files changed, 29 insertions(+), 29 deletions(-) diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index 79f19c2c833..5f090219bfa 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -69,10 +69,11 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { return "", err } box, err := devbox.Open(&devopt.Opts{ - Dir: flags.config.path, - Stderr: cmd.ErrOrStderr(), - Pure: flags.pure, - Env: env, + Dir: flags.config.path, + Stderr: cmd.ErrOrStderr(), + PreservePathStack: flags.preservePathStack, + Pure: flags.pure, + Env: env, }) if err != nil { return "", err diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 0566e62b10b..a4bcf56a5f3 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -60,6 +60,7 @@ type Devbox struct { nix nix.Nixer projectDir string pluginManager *plugin.Manager + preservePathStack bool pure bool allowInsecureAdds bool customProcessComposeFile string @@ -90,6 +91,7 @@ func Open(opts *devopt.Opts) (*Devbox, error) { projectDir: projectDir, pluginManager: plugin.NewManager(), stderr: opts.Stderr, + preservePathStack: opts.PreservePathStack, pure: opts.Pure, customProcessComposeFile: opts.CustomProcessComposeFile, allowInsecureAdds: opts.AllowInsecureAdds, @@ -175,7 +177,7 @@ func (d *Devbox) Shell(ctx context.Context) error { return err } - envs, err := d.nixEnv(ctx, false /*pathStackInPlace*/) + envs, err := d.nixEnv(ctx) if err != nil { return err } @@ -219,7 +221,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string return err } - env, err := d.nixEnv(ctx, false /*pathStackInPlace*/) + env, err := d.nixEnv(ctx) if err != nil { return err } @@ -290,7 +292,7 @@ func (d *Devbox) NixEnv(ctx context.Context, includeHooks, pathStackInPlace bool return "", err } - envs, err := d.nixEnv(ctx, pathStackInPlace) + envs, err := d.nixEnv(ctx) if err != nil { return "", err } @@ -314,7 +316,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { return nil, err } - envs, err := d.nixEnv(ctx, false /*pathStackInPlace*/) + envs, err := d.nixEnv(ctx) if err != nil { return nil, err } @@ -323,7 +325,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { func (d *Devbox) ShellEnvHash(ctx context.Context) (string, error) { // TODO savil: correct? - envs, err := d.nixEnv(ctx, false /*pathStackInPlace*/) + envs, err := d.nixEnv(ctx) if err != nil { return "", err } @@ -766,11 +768,7 @@ func (d *Devbox) StartProcessManager( // Note that the shellrc.tmpl template (which sources this environment) does // some additional processing. The computeNixEnv environment won't necessarily // represent the final "devbox run" or "devbox shell" environments. -func (d *Devbox) computeNixEnv( - ctx context.Context, - usePrintDevEnvCache bool, - pathStackInPlace bool, -) (map[string]string, error) { +func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (map[string]string, error) { defer trace.StartRegion(ctx, "computeNixEnv").End() // Append variables from current env if --pure is not passed @@ -897,7 +895,7 @@ func (d *Devbox) computeNixEnv( }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - env = pathStack.AddToEnv(env, d.projectDirHash(), nixEnvPath, pathStackInPlace) + env = pathStack.AddToEnv(env, d.projectDirHash(), nixEnvPath, d.preservePathStack) debug.Log("New path stack is: %s", pathStack) debug.Log("computed environment PATH is: %s", env["PATH"]) @@ -921,7 +919,7 @@ var nixEnvCache map[string]string // nixEnv is a wrapper around computeNixEnv that caches the result. // Note that this is in-memory cache of the final environment, and not the same // as the nix print-dev-env cache which is stored in a file. -func (d *Devbox) nixEnv(ctx context.Context, pathStackInPlace bool) (map[string]string, error) { +func (d *Devbox) nixEnv(ctx context.Context) (map[string]string, error) { defer debug.FunctionTimer().End() if nixEnvCache != nil { return nixEnvCache, nil @@ -938,7 +936,7 @@ func (d *Devbox) nixEnv(ctx context.Context, pathStackInPlace bool) (map[string] usePrintDevEnvCache = true } - return d.computeNixEnv(ctx, usePrintDevEnvCache, pathStackInPlace) + return d.computeNixEnv(ctx, usePrintDevEnvCache) } func (d *Devbox) nixPrintDevEnvCachePath() string { @@ -1125,7 +1123,7 @@ func (d *Devbox) setCommonHelperEnvVars(env map[string]string) { // give name. This matches how nix flakes behaves if there are conflicts in // buildInputs func (d *Devbox) NixBins(ctx context.Context) ([]string, error) { - env, err := d.nixEnv(ctx, false /*pathStackInPlace*/) + env, err := d.nixEnv(ctx) if err != nil { return nil, err } diff --git a/internal/impl/devbox_test.go b/internal/impl/devbox_test.go index 65361ba9112..1467f1ab18a 100644 --- a/internal/impl/devbox_test.go +++ b/internal/impl/devbox_test.go @@ -71,7 +71,7 @@ func TestComputeNixEnv(t *testing.T) { d := devboxForTesting(t) d.nix = &testNix{} ctx := context.Background() - env, err := d.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) + env, err := d.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") assert.NotNil(t, env, "computeNixEnv should return a valid env") } @@ -80,7 +80,7 @@ func TestComputeNixPathIsIdempotent(t *testing.T) { devbox := devboxForTesting(t) devbox.nix = &testNix{"/tmp/my/path"} ctx := context.Background() - env, err := devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) + env, err := devbox.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") path := env["PATH"] assert.NotEmpty(t, path, "path should not be nil") @@ -90,7 +90,7 @@ func TestComputeNixPathIsIdempotent(t *testing.T) { t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv]) t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())]) - env, err = devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) + env, err = devbox.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") path2 := env["PATH"] @@ -101,7 +101,7 @@ func TestComputeNixPathWhenRemoving(t *testing.T) { devbox := devboxForTesting(t) devbox.nix = &testNix{"/tmp/my/path"} ctx := context.Background() - env, err := devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) + env, err := devbox.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") path := env["PATH"] assert.NotEmpty(t, path, "path should not be nil") @@ -113,7 +113,7 @@ func TestComputeNixPathWhenRemoving(t *testing.T) { t.Setenv(envpath.Key(devbox.projectDirHash()), env[envpath.Key(devbox.projectDirHash())]) devbox.nix.(*testNix).path = "" - env, err = devbox.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/) + env, err = devbox.computeNixEnv(ctx, false /*use cache*/) require.NoError(t, err, "computeNixEnv should not fail") path2 := env["PATH"] assert.NotContains(t, path2, "/tmp/my/path", "path should not contain /tmp/my/path") diff --git a/internal/impl/devopt/devboxopts.go b/internal/impl/devopt/devboxopts.go index 041b86fb1db..438f2af9e89 100644 --- a/internal/impl/devopt/devboxopts.go +++ b/internal/impl/devopt/devboxopts.go @@ -8,6 +8,7 @@ type Opts struct { AllowInsecureAdds bool Dir string Env map[string]string + PreservePathStack bool Pure bool IgnoreWarnings bool CustomProcessComposeFile string diff --git a/internal/impl/envpath/pathstack.go b/internal/impl/envpath/pathstack.go index 75842f47fcf..1a6b49eeb02 100644 --- a/internal/impl/envpath/pathstack.go +++ b/internal/impl/envpath/pathstack.go @@ -64,7 +64,7 @@ func (s *Stack) AddToEnv( env map[string]string, projectHash string, nixEnvPath string, - pathStackInPlace bool, + preservePathStack bool, ) map[string]string { key := Key(projectHash) @@ -72,8 +72,8 @@ func (s *Stack) AddToEnv( env[key] = nixEnvPath // Common case: ensure this key is at the top of the stack - if !pathStackInPlace || - // Case pathStackInPlace == true, usually from bin-wrapper or (in future) shell hook. + if !preservePathStack || + // Case preservePathStack == true, usually from bin-wrapper or (in future) shell hook. // Add this key only if absent from the stack !lo.Contains(s.keys, key) { @@ -81,7 +81,7 @@ func (s *Stack) AddToEnv( } env[PathStackEnv] = s.String() - // Look up the paths-list for each paths-stack element, and join them together to get the final PATH. + // Look up the paths-list for each Stack element, and join them together to get the final PATH. pathLists := lo.Map(s.keys, func(part string, idx int) string { return env[part] }) env["PATH"] = JoinPathLists(pathLists...) return env diff --git a/internal/impl/packages.go b/internal/impl/packages.go index 9251f27eb5c..8025c0c0554 100644 --- a/internal/impl/packages.go +++ b/internal/impl/packages.go @@ -234,7 +234,7 @@ func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMod } // Force print-dev-env cache to be recomputed. - if _, err := d.computeNixEnv(ctx, false /*use cache*/, false /*pathStackInPlace*/); err != nil { + if _, err := d.computeNixEnv(ctx, false /*use cache*/); err != nil { return err } diff --git a/internal/wrapnix/wrapper.sh.tmpl b/internal/wrapnix/wrapper.sh.tmpl index 6f5afd7d5ff..b4bb9198915 100644 --- a/internal/wrapnix/wrapper.sh.tmpl +++ b/internal/wrapnix/wrapper.sh.tmpl @@ -21,7 +21,7 @@ DO_NOT_TRACK=1 can be removed once we optimize segment to queue events. if [[ "${{ .ShellEnvHashKey }}" != "{{ .ShellEnvHash }}" ]] && [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then export {{ .ShellEnvHashKey }}_GUARD=true -eval "$(DO_NOT_TRACK=1 devbox shellenv --path-stack-in-place -c {{ .ProjectDir }})" +eval "$(DO_NOT_TRACK=1 devbox shellenv --preserve-path-stack -c {{ .ProjectDir }})" fi {{/* From d970e2ae32fbb399b302d93b41d99a3086a2cc79 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 12:59:08 -0700 Subject: [PATCH 07/16] remove moar pathStackInPlace --- devbox.go | 4 ++-- internal/boxcli/install.go | 2 +- internal/boxcli/shell.go | 2 +- internal/boxcli/shellenv.go | 4 ++-- internal/impl/devbox.go | 6 +++--- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/devbox.go b/devbox.go index 77dbad7f6af..cbdc8c1fd27 100644 --- a/devbox.go +++ b/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, pathStackInPlace bool) error + Install(ctx context.Context) error IsEnvEnabled() bool ListScripts() []string - NixEnv(ctx context.Context, includeHooks, pathStackInPlace bool) (string, error) + NixEnv(ctx context.Context, includeHooks bool) (string, error) PackageNames() []string ProjectDir() string Pull(ctx context.Context, opts devopt.PullboxOpts) error diff --git a/internal/boxcli/install.go b/internal/boxcli/install.go index f14a59ffbd6..fed39224e8d 100644 --- a/internal/boxcli/install.go +++ b/internal/boxcli/install.go @@ -39,7 +39,7 @@ func installCmdFunc(cmd *cobra.Command, flags runCmdFlags) error { if err != nil { return errors.WithStack(err) } - if err = box.Install(cmd.Context(), false /*preservePathStack*/); err != nil { + if err = box.Install(cmd.Context()); err != nil { return errors.WithStack(err) } fmt.Fprintln(cmd.ErrOrStderr(), "Finished installing packages.") diff --git a/internal/boxcli/shell.go b/internal/boxcli/shell.go index c3c01811263..4cb5cff95f5 100644 --- a/internal/boxcli/shell.go +++ b/internal/boxcli/shell.go @@ -66,7 +66,7 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error { if flags.printEnv { // false for includeHooks is because init hooks is not compatible with .envrc files generated // by versions older than 0.4.6 - script, err := box.NixEnv(cmd.Context(), false /*includeHooks*/, false /*preservePathStack*/) + script, err := box.NixEnv(cmd.Context(), false /*includeHooks*/) if err != nil { return err } diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index 5f090219bfa..848f9f2371e 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -80,12 +80,12 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { } if flags.install { - if err := box.Install(cmd.Context(), flags.preservePathStack); err != nil { + if err := box.Install(cmd.Context()); err != nil { return "", err } } - envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook, flags.preservePathStack) + envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook) if err != nil { return "", err } diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index a4bcf56a5f3..38ada1fe386 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -263,11 +263,11 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string // Install ensures that all the packages in the config are installed and // creates all wrappers, but does not run init hooks. It is used to power // devbox install cli command. -func (d *Devbox) Install(ctx context.Context, pathStackInPlace bool) error { +func (d *Devbox) Install(ctx context.Context) error { ctx, task := trace.NewTask(ctx, "devboxInstall") defer task.End() - if _, err := d.NixEnv(ctx, false /*includeHooks*/, pathStackInPlace); err != nil { + if _, err := d.NixEnv(ctx, false /*includeHooks*/); err != nil { return err } return wrapnix.CreateWrappers(ctx, d) @@ -284,7 +284,7 @@ func (d *Devbox) ListScripts() []string { return keys } -func (d *Devbox) NixEnv(ctx context.Context, includeHooks, pathStackInPlace bool) (string, error) { +func (d *Devbox) NixEnv(ctx context.Context, includeHooks bool) (string, error) { ctx, task := trace.NewTask(ctx, "devboxNixEnv") defer task.End() From 983f3abf51badb51af18009c5835ba7571499a32 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:05:02 -0700 Subject: [PATCH 08/16] rename file for pathlists --- internal/impl/devbox.go | 1 - .../impl/envpath/{envpath.go => pathlists.go} | 0 internal/impl/envpath/pathlists_test.go | 32 +++++++++++++++++++ internal/impl/shell_test.go | 28 ---------------- 4 files changed, 32 insertions(+), 29 deletions(-) rename internal/impl/envpath/{envpath.go => pathlists.go} (100%) create mode 100644 internal/impl/envpath/pathlists_test.go diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 38ada1fe386..95dcbd258f4 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -324,7 +324,6 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { } func (d *Devbox) ShellEnvHash(ctx context.Context) (string, error) { - // TODO savil: correct? envs, err := d.nixEnv(ctx) if err != nil { return "", err diff --git a/internal/impl/envpath/envpath.go b/internal/impl/envpath/pathlists.go similarity index 100% rename from internal/impl/envpath/envpath.go rename to internal/impl/envpath/pathlists.go diff --git a/internal/impl/envpath/pathlists_test.go b/internal/impl/envpath/pathlists_test.go new file mode 100644 index 00000000000..e3b36a4dad9 --- /dev/null +++ b/internal/impl/envpath/pathlists_test.go @@ -0,0 +1,32 @@ +package envpath + +import ( + "testing" +) + +func TestCleanEnvPath(t *testing.T) { + tests := []struct { + name string + inPath string + outPath string + }{ + { + name: "NoEmptyPaths", + inPath: "/usr/local/bin::", + outPath: "/usr/local/bin", + }, + { + name: "NoRelativePaths", + inPath: "/usr/local/bin:/usr/bin:../test:/bin:/usr/sbin:/sbin:.:..", + outPath: "/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := JoinPathLists(test.inPath) + if got != test.outPath { + t.Errorf("Got incorrect cleaned PATH.\ngot: %s\nwant: %s", got, test.outPath) + } + }) + } +} diff --git a/internal/impl/shell_test.go b/internal/impl/shell_test.go index 5bd59aaba1a..f17337ccd7f 100644 --- a/internal/impl/shell_test.go +++ b/internal/impl/shell_test.go @@ -13,7 +13,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "go.jetpack.io/devbox/internal/impl/envpath" "go.jetpack.io/devbox/internal/shellgen" ) @@ -106,30 +105,3 @@ If the new shellrc is correct, you can update the golden file with: }) } } - -func TestCleanEnvPath(t *testing.T) { - tests := []struct { - name string - inPath string - outPath string - }{ - { - name: "NoEmptyPaths", - inPath: "/usr/local/bin::", - outPath: "/usr/local/bin", - }, - { - name: "NoRelativePaths", - inPath: "/usr/local/bin:/usr/bin:../test:/bin:/usr/sbin:/sbin:.:..", - outPath: "/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin", - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - got := envpath.JoinPathLists(test.inPath) - if got != test.outPath { - t.Errorf("Got incorrect cleaned PATH.\ngot: %s\nwant: %s", got, test.outPath) - } - }) - } -} From 5d4ec61bd7ac6df8714d8cd0e34edf48304fe846 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:23:49 -0700 Subject: [PATCH 09/16] requested changes in envpath --- internal/impl/devbox.go | 4 +-- internal/impl/envpath/pathstack.go | 50 +++++++++++++++--------------- internal/impl/envvars.go | 2 +- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 95dcbd258f4..435ba7f0e17 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -787,7 +787,7 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m debug.Log("current environment PATH is: %s", env["PATH"]) - pathStack := envpath.NewStack(env) + pathStack := envpath.Stack(env) debug.Log("Original path stack is: %s", pathStack) vaf, err := d.nix.PrintDevEnv(ctx, &nix.PrintDevEnvArgs{ @@ -894,7 +894,7 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - env = pathStack.AddToEnv(env, d.projectDirHash(), nixEnvPath, d.preservePathStack) + env = pathStack.PushAndUpdateEnv(env, d.projectDirHash(), nixEnvPath, d.preservePathStack) debug.Log("New path stack is: %s", pathStack) debug.Log("computed environment PATH is: %s", env["PATH"]) diff --git a/internal/impl/envpath/pathstack.go b/internal/impl/envpath/pathstack.go index 1a6b49eeb02..825fb07e8d3 100644 --- a/internal/impl/envpath/pathstack.go +++ b/internal/impl/envpath/pathstack.go @@ -8,27 +8,29 @@ import ( ) const ( + // PathStackEnv stores the string representation of the stack, as a ":" separated list. + // Each element in the list is also the key to the env-var that stores the + // nixEnvPath for that devbox-project. Except for the last element, which is InitPathEnv. PathStackEnv = "DEVBOX_PATH_STACK" // InitPathEnv stores the path prior to any devbox shellenv modifying the environment InitPathEnv = "DEVBOX_INIT_PATH" ) -// Stack has the following design: -// 1. The PathStack enables tracking which sub-paths in PATH come from which devbox-project -// 2. What it stores: The PathStack is an ordered-list of nixEnvPathKeys -// 3. Each nixEnvPathKey is set as an env-var with the value of the nixEnvPath for that devbox-project. -// 4. The final PATH is reconstructed by concatenating the env-var values of each nixEnvPathKey env-var. -// 5. The Stack is stored in its own env-var shared by all devbox-projects in this shell. -type Stack struct { +// stack has the following design: +// 1. The stack enables tracking which sub-paths in PATH come from which devbox-project +// 2. It is an ordered-list of keys to env-vars that store nixEnvPath values of devbox-projects. +// 3. The final PATH is reconstructed by concatenating the env-var values of each nixEnvPathKey. +// 5. The stack is stored in its own env-var PathStackEnv, shared by all devbox-projects in this shell. +type stack struct { // keys holds the stack elements. // Earlier (lower index number) keys get higher priority. - // This keeps the string representation of the Stack aligned with the PATH value. + // This keeps the string representation of the stack aligned with the PATH value. keys []string } -func NewStack(env map[string]string) *Stack { +func Stack(env map[string]string) *stack { stackEnv, ok := env[PathStackEnv] if !ok { // if path stack is empty, then push the current PATH, which is the @@ -36,31 +38,34 @@ func NewStack(env map[string]string) *Stack { stackEnv = InitPathEnv env[InitPathEnv] = env["PATH"] } - return &Stack{ + return &stack{ keys: strings.Split(stackEnv, ":"), } } -// String is the value of the Stack stored in its env-var. -func (s *Stack) String() string { +// String is the value of the stack stored in its env-var. +func (s *stack) String() string { return strings.Join(s.keys, ":") } -// Key is the element stored in the Stack for a devbox-project. It represents +// Key is the element stored in the stack for a devbox-project. It represents // a pointer to the nixEnvPath value stored in its own env-var, also using this same // Key. func Key(projectHash string) string { - return "DEVBOX_NIX_" + projectHash + return "DEVBOX_NIX_ENV_PATH_" + projectHash } -// AddToEnv adds the new nixEnvPath for the devbox-project identified by projectHash to the env. -// It does so by modifying the following env-vars: +// PushAndUpdateEnv adds the new nixEnvPath for the devbox-project identified by projectHash. +// The nixEnvPath is pushed to the top of the stack (given highest priority), unless preservePathStack +// is enabled. +// +// It also updated the env by modifying the following env-vars: // 1. nixEnvPath key // 2. PathStack // 3. PATH // // Returns the modified env map -func (s *Stack) AddToEnv( +func (s *stack) PushAndUpdateEnv( env map[string]string, projectHash string, nixEnvPath string, @@ -81,7 +86,7 @@ func (s *Stack) AddToEnv( } env[PathStackEnv] = s.String() - // Look up the paths-list for each Stack element, and join them together to get the final PATH. + // Look up the paths-list for each stack element, and join them together to get the final PATH. pathLists := lo.Map(s.keys, func(part string, idx int) string { return env[part] }) env["PATH"] = JoinPathLists(pathLists...) return env @@ -89,11 +94,6 @@ func (s *Stack) AddToEnv( // Has tests if the stack has the specified key. Refer to the Key function for constructing // the appropriate key for any devbox-project. -func (s *Stack) Has(key string) bool { - for _, k := range s.keys { - if k == key { - return true - } - } - return false +func (s *stack) Has(key string) bool { + return lo.Contains(s.keys, key) } diff --git a/internal/impl/envvars.go b/internal/impl/envvars.go index ab258b9ab33..4ba1156c4e1 100644 --- a/internal/impl/envvars.go +++ b/internal/impl/envvars.go @@ -105,6 +105,6 @@ func (d *Devbox) IsEnvEnabled() bool { parts := strings.Split(keyVal, "=") env[parts[0]] = parts[1] } - pathStack := envpath.NewStack(env) + pathStack := envpath.Stack(env) return pathStack.Has(envpath.Key(d.projectDirHash())) } From aac1a9356fb16f6f4a61f3a4e9c6954f25530506 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:26:47 -0700 Subject: [PATCH 10/16] update flag description --- internal/boxcli/shellenv.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index 848f9f2371e..2e6fbbd5e47 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -51,8 +51,8 @@ func shellEnvCmd() *cobra.Command { &flags.pure, "pure", false, "If this flag is specified, devbox creates an isolated environment inheriting almost no variables from the current environment. A few variables, in particular HOME, USER and DISPLAY, are retained.") command.Flags().BoolVar( &flags.preservePathStack, "preserve-path-stack", false, - "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.") + "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") _ = command.Flags().MarkHidden("preserve-path-stack") flags.config.register(command) From 4b3683812fad92f11a53a69421954fcba028be49 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 14:51:59 -0700 Subject: [PATCH 11/16] rename file to envpath/stack.go --- internal/impl/envpath/{pathstack.go => stack.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename internal/impl/envpath/{pathstack.go => stack.go} (100%) diff --git a/internal/impl/envpath/pathstack.go b/internal/impl/envpath/stack.go similarity index 100% rename from internal/impl/envpath/pathstack.go rename to internal/impl/envpath/stack.go From bd4a98ae818d125b443947c20256ec169c02c3a2 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 15:35:41 -0700 Subject: [PATCH 12/16] requested changes by Greg and Mike --- internal/envir/util.go | 31 +++++++++++++++++++++++++ internal/impl/devbox.go | 15 ++++++++----- internal/impl/envpath/stack.go | 31 +++++++++++++------------ internal/impl/envvars.go | 41 +++++----------------------------- internal/impl/shell.go | 2 +- internal/impl/shell_test.go | 3 ++- 6 files changed, 64 insertions(+), 59 deletions(-) diff --git a/internal/envir/util.go b/internal/envir/util.go index e7d8d880d13..3ebb0f17a01 100644 --- a/internal/envir/util.go +++ b/internal/envir/util.go @@ -6,6 +6,9 @@ package envir import ( "os" "strconv" + "strings" + + "slices" ) func IsDevboxCloud() bool { @@ -43,3 +46,31 @@ func GetValueOrDefault(key, def string) string { return val } + +// MapToPairs creates a slice of environment variable "key=value" pairs from a +// map. +func MapToPairs(m map[string]string) []string { + pairs := make([]string, len(m)) + i := 0 + for k, v := range m { + pairs[i] = k + "=" + v + i++ + } + slices.Sort(pairs) // for reproducibility + return pairs +} + +// PairsToMap creates a map from a slice of "key=value" environment variable +// pairs. Note that maps are not ordered, which can affect the final variable +// values when pairs contains duplicate keys. +func PairsToMap(pairs []string) map[string]string { + vars := make(map[string]string, len(pairs)) + for _, p := range pairs { + k, v, ok := strings.Cut(p, "=") + if !ok { + continue + } + vars[k] = v + } + return vars +} diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 435ba7f0e17..cbfefffae7c 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -13,11 +13,12 @@ import ( "os/exec" "path/filepath" "runtime/trace" - "slices" "strconv" "strings" "text/tabwriter" + "slices" + "github.com/pkg/errors" "github.com/samber/lo" "go.jetpack.io/devbox/internal/devpkg" @@ -26,6 +27,7 @@ import ( "go.jetpack.io/devbox/internal/searcher" "go.jetpack.io/devbox/internal/shellgen" "go.jetpack.io/devbox/internal/telemetry" + "golang.org/x/exp/maps" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cmdutil" @@ -320,7 +322,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { if err != nil { return nil, err } - return mapToPairs(envs), nil + return envir.MapToPairs(envs), nil } func (d *Devbox) ShellEnvHash(ctx context.Context) (string, error) { @@ -787,8 +789,8 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m debug.Log("current environment PATH is: %s", env["PATH"]) - pathStack := envpath.Stack(env) - debug.Log("Original path stack is: %s", pathStack) + originalEnv := make(map[string]string, len(env)) + maps.Copy(originalEnv, env) vaf, err := d.nix.PrintDevEnv(ctx, &nix.PrintDevEnvArgs{ FlakesFilePath: d.nixFlakesFilePath(), @@ -894,7 +896,10 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - env = pathStack.PushAndUpdateEnv(env, d.projectDirHash(), nixEnvPath, d.preservePathStack) + pathStack := envpath.Stack(originalEnv) + pathStack.Push(env, d.projectDirHash(), nixEnvPath, d.preservePathStack) + env["PATH"] = pathStack.Path(env) + debug.Log("New path stack is: %s", pathStack) debug.Log("computed environment PATH is: %s", env["PATH"]) diff --git a/internal/impl/envpath/stack.go b/internal/impl/envpath/stack.go index 825fb07e8d3..bf315048600 100644 --- a/internal/impl/envpath/stack.go +++ b/internal/impl/envpath/stack.go @@ -32,7 +32,7 @@ type stack struct { func Stack(env map[string]string) *stack { stackEnv, ok := env[PathStackEnv] - if !ok { + if !ok || strings.TrimSpace(stackEnv) == "" { // if path stack is empty, then push the current PATH, which is the // external environment prior to any devbox-shellenv being applied to it. stackEnv = InitPathEnv @@ -48,6 +48,12 @@ func (s *stack) String() string { return strings.Join(s.keys, ":") } +func (s *stack) Path(env map[string]string) string { + // Look up the paths-list for each stack element, and join them together to get the final PATH. + pathLists := lo.Map(s.keys, func(part string, idx int) string { return env[part] }) + return JoinPathLists(pathLists...) +} + // Key is the element stored in the stack for a devbox-project. It represents // a pointer to the nixEnvPath value stored in its own env-var, also using this same // Key. @@ -55,22 +61,20 @@ func Key(projectHash string) string { return "DEVBOX_NIX_ENV_PATH_" + projectHash } -// PushAndUpdateEnv adds the new nixEnvPath for the devbox-project identified by projectHash. -// The nixEnvPath is pushed to the top of the stack (given highest priority), unless preservePathStack -// is enabled. +// Push adds the new nixEnvPath for the devbox-project identified by projectHash. +// The nixEnvPath is pushed to the top of the stack (given highest priority), +// unless preservePathStack is enabled. // -// It also updated the env by modifying the following env-vars: +// It also updates the env by modifying the following env-vars: // 1. nixEnvPath key // 2. PathStack // 3. PATH -// -// Returns the modified env map -func (s *stack) PushAndUpdateEnv( +func (s *stack) Push( env map[string]string, projectHash string, nixEnvPath string, preservePathStack bool, -) map[string]string { +) { key := Key(projectHash) // Add this nixEnvPath to env @@ -85,15 +89,10 @@ func (s *stack) PushAndUpdateEnv( s.keys = lo.Uniq(slices.Insert(s.keys, 0, key)) } env[PathStackEnv] = s.String() - - // Look up the paths-list for each stack element, and join them together to get the final PATH. - pathLists := lo.Map(s.keys, func(part string, idx int) string { return env[part] }) - env["PATH"] = JoinPathLists(pathLists...) - return env } // Has tests if the stack has the specified key. Refer to the Key function for constructing // the appropriate key for any devbox-project. -func (s *stack) Has(key string) bool { - return lo.Contains(s.keys, key) +func (s *stack) Has(projectHash string) bool { + return lo.Contains(s.keys, Key(projectHash)) } diff --git a/internal/impl/envvars.go b/internal/impl/envvars.go index 4ba1156c4e1..aec8380738f 100644 --- a/internal/impl/envvars.go +++ b/internal/impl/envvars.go @@ -5,42 +5,16 @@ package impl import ( "os" - "slices" "strings" + "slices" + + "go.jetpack.io/devbox/internal/envir" "go.jetpack.io/devbox/internal/impl/envpath" ) const devboxSetPrefix = "__DEVBOX_SET_" -// mapToPairs creates a slice of environment variable "key=value" pairs from a -// map. -func mapToPairs(m map[string]string) []string { - pairs := make([]string, len(m)) - i := 0 - for k, v := range m { - pairs[i] = k + "=" + v - i++ - } - slices.Sort(pairs) // for reproducibility - return pairs -} - -// pairsToMap creates a map from a slice of "key=value" environment variable -// pairs. Note that maps are not ordered, which can affect the final variable -// values when pairs contains duplicate keys. -func pairsToMap(pairs []string) map[string]string { - vars := make(map[string]string, len(pairs)) - for _, p := range pairs { - k, v, ok := strings.Cut(p, "=") - if !ok { - continue - } - vars[k] = v - } - return vars -} - // exportify formats vars as a line-separated string of shell export statements. // Each line is of the form `export key="value";` with any special characters in // value escaped. This means that the shell will always interpret values as @@ -100,11 +74,6 @@ func markEnvsAsSetByDevbox(envs ...map[string]string) { // as a proxy for this. This allows us to differentiate between global and // individual project shells. func (d *Devbox) IsEnvEnabled() bool { - env := map[string]string{} - for _, keyVal := range os.Environ() { - parts := strings.Split(keyVal, "=") - env[parts[0]] = parts[1] - } - pathStack := envpath.Stack(env) - return pathStack.Has(envpath.Key(d.projectDirHash())) + pathStack := envpath.Stack(envir.PairsToMap(os.Environ())) + return pathStack.Has(d.projectDirHash()) } diff --git a/internal/impl/shell.go b/internal/impl/shell.go index 8385ea3d7a2..2f69476066e 100644 --- a/internal/impl/shell.go +++ b/internal/impl/shell.go @@ -247,7 +247,7 @@ func (s *DevboxShell) Run() error { env["SHELL"] = s.binPath cmd = exec.Command(s.binPath) - cmd.Env = mapToPairs(env) + cmd.Env = envir.MapToPairs(env) cmd.Args = append(cmd.Args, extraArgs...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout diff --git a/internal/impl/shell_test.go b/internal/impl/shell_test.go index f17337ccd7f..98255ef2b79 100644 --- a/internal/impl/shell_test.go +++ b/internal/impl/shell_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "go.jetpack.io/devbox/internal/envir" "go.jetpack.io/devbox/internal/shellgen" ) @@ -45,7 +46,7 @@ func testWriteDevboxShellrc(t *testing.T, testdirs []string) { test := &tests[i] test.name = filepath.Base(path) if b, err := os.ReadFile(filepath.Join(path, "env")); err == nil { - test.env = pairsToMap(strings.Split(string(b), "\n")) + test.env = envir.PairsToMap(strings.Split(string(b), "\n")) } test.hooksFilePath = shellgen.ScriptPath(projectDir, shellgen.HooksFilename) From ee4975568803ecf7da6f636ad393bfb306428788 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 29 Sep 2023 16:54:43 -0700 Subject: [PATCH 13/16] add stack test --- internal/impl/envpath/stack_test.go | 103 ++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 internal/impl/envpath/stack_test.go diff --git a/internal/impl/envpath/stack_test.go b/internal/impl/envpath/stack_test.go new file mode 100644 index 00000000000..22829d2625d --- /dev/null +++ b/internal/impl/envpath/stack_test.go @@ -0,0 +1,103 @@ +package envpath + +import ( + "strings" + "testing" +) + +func TestNewStack(t *testing.T) { + + // Initialize a new Stack from the existing env + env := map[string]string{ + "PATH": "/init-path", + } + stack := Stack(env) + if len(stack.keys) == 0 { + t.Errorf("Stack has no keys but should have %s", InitPathEnv) + } + if len(stack.keys) != 1 { + t.Errorf("Stack has should have exactly one key (%s) but has %d keys. Keys are: %s", + InitPathEnv, len(stack.keys), strings.Join(stack.keys, ", ")) + } + + // Each testStep below is applied in order, and the resulting env + // is used implicitly as input into the subsequent test step. + // + // These test steps are NOT independent! These are not "test cases" that + // would usually be independent. + testSteps := []struct { + projectHash string + nixEnvPath string + preservePathStack bool + expectedKeysLength int + expectedEnv map[string]string + }{ + { + projectHash: "fooProjectHash", + nixEnvPath: "/foo1:/foo2", + preservePathStack: false, + expectedKeysLength: 2, + expectedEnv: map[string]string{ + "PATH": "/foo1:/foo2:/init-path", + InitPathEnv: "/init-path", + Key("fooProjectHash"): "/foo1:/foo2", + }, + }, + { + projectHash: "barProjectHash", + nixEnvPath: "/bar1:/bar2", + preservePathStack: false, + expectedKeysLength: 3, + expectedEnv: map[string]string{ + "PATH": "/bar1:/bar2:/foo1:/foo2:/init-path", + InitPathEnv: "/init-path", + Key("fooProjectHash"): "/foo1:/foo2", + Key("barProjectHash"): "/bar1:/bar2", + }, + }, + { + projectHash: "fooProjectHash", + nixEnvPath: "/foo3:/foo2", + preservePathStack: false, + expectedKeysLength: 3, + expectedEnv: map[string]string{ + "PATH": "/foo3:/foo2:/bar1:/bar2:/init-path", + InitPathEnv: "/init-path", + Key("fooProjectHash"): "/foo3:/foo2", + Key("barProjectHash"): "/bar1:/bar2", + }, + }, + { + projectHash: "barProjectHash", + nixEnvPath: "/bar3:/bar2", + preservePathStack: true, + expectedKeysLength: 3, + expectedEnv: map[string]string{ + "PATH": "/foo3:/foo2:/bar3:/bar2:/init-path", + InitPathEnv: "/init-path", + Key("fooProjectHash"): "/foo3:/foo2", + Key("barProjectHash"): "/bar3:/bar2", + }, + }, + } + + for _, testStep := range testSteps { + t.Run( + testStep.nixEnvPath, func(t *testing.T) { + + // Push to stack and update PATH env + stack.Push(env, testStep.projectHash, testStep.nixEnvPath, testStep.preservePathStack) + env["PATH"] = stack.Path(env) + + if len(stack.keys) != testStep.expectedKeysLength { + t.Errorf("Stack should have exactly %d keys but has %d keys. Keys are: %s", + testStep.expectedKeysLength, len(stack.keys), strings.Join(stack.keys, ", ")) + } + for k, v := range testStep.expectedEnv { + if env[k] != v { + t.Errorf("env[%s] should be %s but is %s", k, v, env[k]) + } + } + }) + } +} From 67b941d9b4a7e3db317a625370669dea9737e4b9 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Mon, 2 Oct 2023 11:42:35 -0700 Subject: [PATCH 14/16] update dependency --- internal/impl/devbox.go | 2 +- internal/impl/envpath/stack.go | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index cbfefffae7c..8a7266210a7 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -17,6 +17,7 @@ import ( "strings" "text/tabwriter" + "maps" "slices" "github.com/pkg/errors" @@ -27,7 +28,6 @@ import ( "go.jetpack.io/devbox/internal/searcher" "go.jetpack.io/devbox/internal/shellgen" "go.jetpack.io/devbox/internal/telemetry" - "golang.org/x/exp/maps" "go.jetpack.io/devbox/internal/boxcli/usererr" "go.jetpack.io/devbox/internal/cmdutil" diff --git a/internal/impl/envpath/stack.go b/internal/impl/envpath/stack.go index bf315048600..3f975b7c816 100644 --- a/internal/impl/envpath/stack.go +++ b/internal/impl/envpath/stack.go @@ -65,10 +65,8 @@ func Key(projectHash string) string { // The nixEnvPath is pushed to the top of the stack (given highest priority), // unless preservePathStack is enabled. // -// It also updates the env by modifying the following env-vars: -// 1. nixEnvPath key -// 2. PathStack -// 3. PATH +// It also updates the env by modifying the PathStack env-var, and the env-var +// for storing the nixEnvPath. func (s *stack) Push( env map[string]string, projectHash string, From 92c0585d9dd25c294987ccbdbe029eebcf1d52c7 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Mon, 2 Oct 2023 12:26:56 -0700 Subject: [PATCH 15/16] fix bug with stack updating wrong env --- internal/impl/devbox.go | 8 +++----- internal/impl/envpath/stack.go | 8 +++++--- internal/impl/envpath/stack_test.go | 10 ++++++---- internal/impl/envvars.go | 7 ++++--- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 8a7266210a7..6809aca6f40 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -9,17 +9,16 @@ import ( "fmt" "io" "io/fs" + "maps" "os" "os/exec" "path/filepath" "runtime/trace" + "slices" "strconv" "strings" "text/tabwriter" - "maps" - "slices" - "github.com/pkg/errors" "github.com/samber/lo" "go.jetpack.io/devbox/internal/devpkg" @@ -896,10 +895,9 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m }) debug.Log("PATH after filtering with buildInputs (%v) is: %s", buildInputs, nixEnvPath) - pathStack := envpath.Stack(originalEnv) + pathStack := envpath.Stack(env, originalEnv) pathStack.Push(env, d.projectDirHash(), nixEnvPath, d.preservePathStack) env["PATH"] = pathStack.Path(env) - debug.Log("New path stack is: %s", pathStack) debug.Log("computed environment PATH is: %s", env["PATH"]) diff --git a/internal/impl/envpath/stack.go b/internal/impl/envpath/stack.go index 3f975b7c816..30e449521c7 100644 --- a/internal/impl/envpath/stack.go +++ b/internal/impl/envpath/stack.go @@ -30,13 +30,15 @@ type stack struct { keys []string } -func Stack(env map[string]string) *stack { - stackEnv, ok := env[PathStackEnv] +// Stack initializes the path stack in the `env` environment. +// It relies on old state stored in the `originalEnv` environment. +func Stack(env, originalEnv map[string]string) *stack { + stackEnv, ok := originalEnv[PathStackEnv] if !ok || strings.TrimSpace(stackEnv) == "" { // if path stack is empty, then push the current PATH, which is the // external environment prior to any devbox-shellenv being applied to it. stackEnv = InitPathEnv - env[InitPathEnv] = env["PATH"] + env[InitPathEnv] = originalEnv["PATH"] } return &stack{ keys: strings.Split(stackEnv, ":"), diff --git a/internal/impl/envpath/stack_test.go b/internal/impl/envpath/stack_test.go index 22829d2625d..0bb2ae6f31d 100644 --- a/internal/impl/envpath/stack_test.go +++ b/internal/impl/envpath/stack_test.go @@ -1,6 +1,7 @@ package envpath import ( + "fmt" "strings" "testing" ) @@ -8,10 +9,11 @@ import ( func TestNewStack(t *testing.T) { // Initialize a new Stack from the existing env - env := map[string]string{ + originalEnv := map[string]string{ "PATH": "/init-path", } - stack := Stack(env) + env := make(map[string]string) + stack := Stack(env, originalEnv) if len(stack.keys) == 0 { t.Errorf("Stack has no keys but should have %s", InitPathEnv) } @@ -81,9 +83,9 @@ func TestNewStack(t *testing.T) { }, } - for _, testStep := range testSteps { + for idx, testStep := range testSteps { t.Run( - testStep.nixEnvPath, func(t *testing.T) { + fmt.Sprintf("step_%d", idx), func(t *testing.T) { // Push to stack and update PATH env stack.Push(env, testStep.projectHash, testStep.nixEnvPath, testStep.preservePathStack) diff --git a/internal/impl/envvars.go b/internal/impl/envvars.go index aec8380738f..488e9ad0508 100644 --- a/internal/impl/envvars.go +++ b/internal/impl/envvars.go @@ -5,9 +5,8 @@ package impl import ( "os" - "strings" - "slices" + "strings" "go.jetpack.io/devbox/internal/envir" "go.jetpack.io/devbox/internal/impl/envpath" @@ -74,6 +73,8 @@ func markEnvsAsSetByDevbox(envs ...map[string]string) { // as a proxy for this. This allows us to differentiate between global and // individual project shells. func (d *Devbox) IsEnvEnabled() bool { - pathStack := envpath.Stack(envir.PairsToMap(os.Environ())) + fakeEnv := map[string]string{} + // the Stack is initialized in the fakeEnv, from the state in the real os.Environ + pathStack := envpath.Stack(fakeEnv, envir.PairsToMap(os.Environ())) return pathStack.Has(d.projectDirHash()) } From 038b7604889f7bec3e5338fe6fcb162753106eeb Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Mon, 2 Oct 2023 12:39:22 -0700 Subject: [PATCH 16/16] lint fix --- internal/envir/util.go | 3 +-- internal/impl/envpath/stack.go | 1 - internal/impl/envpath/stack_test.go | 2 -- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/envir/util.go b/internal/envir/util.go index 3ebb0f17a01..8ebefb87847 100644 --- a/internal/envir/util.go +++ b/internal/envir/util.go @@ -5,10 +5,9 @@ package envir import ( "os" + "slices" "strconv" "strings" - - "slices" ) func IsDevboxCloud() bool { diff --git a/internal/impl/envpath/stack.go b/internal/impl/envpath/stack.go index 30e449521c7..88824e0e7cd 100644 --- a/internal/impl/envpath/stack.go +++ b/internal/impl/envpath/stack.go @@ -23,7 +23,6 @@ const ( // 3. The final PATH is reconstructed by concatenating the env-var values of each nixEnvPathKey. // 5. The stack is stored in its own env-var PathStackEnv, shared by all devbox-projects in this shell. type stack struct { - // keys holds the stack elements. // Earlier (lower index number) keys get higher priority. // This keeps the string representation of the stack aligned with the PATH value. diff --git a/internal/impl/envpath/stack_test.go b/internal/impl/envpath/stack_test.go index 0bb2ae6f31d..74846dc1463 100644 --- a/internal/impl/envpath/stack_test.go +++ b/internal/impl/envpath/stack_test.go @@ -7,7 +7,6 @@ import ( ) func TestNewStack(t *testing.T) { - // Initialize a new Stack from the existing env originalEnv := map[string]string{ "PATH": "/init-path", @@ -86,7 +85,6 @@ func TestNewStack(t *testing.T) { for idx, testStep := range testSteps { t.Run( fmt.Sprintf("step_%d", idx), func(t *testing.T) { - // Push to stack and update PATH env stack.Push(env, testStep.projectHash, testStep.nixEnvPath, testStep.preservePathStack) env["PATH"] = stack.Path(env)