requested changes by Greg and Mike

Author: savil

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
+}

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"])

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,29 +48,33 @@ 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.
 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))
 }

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())
 }

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

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)