[services] Fix issue where env var is ignored (#1570)

## Summary

Fixes #1565

The bugs:
* env vars specified via `--env-file` (and possibly via other methods)
are ignored in nginx config.
* When inside a devbox shell or a direnv context, starting services
would ignore env vars.

Intermediate causes:
* `envsubst` binary was a bin wrapped binary. This was resetting the env
vars. Fix: Never use bin wrappers when inside a `devbox run` (This was
previously only scoped to using the CLI command explicitly, but now we
do it any time we use RunScript function)
* Previously we would only run services in new shell if we were not
already in a shell or direnv. But this causes us to ignore environment
variables that are set after the environment is created due to bin
wrappers. Fix: always run services in new shell.

Root cause:
* I'm not 100% sure what caused this. It's possible the [PATH stack
PR](c11031a)
did, but I don't fully understand the mechanism. I'm still confident the
changes in this PR are good and simplify uncertainty about how services
are called.

Sorry in advance for all the boolean params. We should move to structs,
but I didn't want to make those changes here.

## How was it tested?

* Copied setup from #1565. 
* Tried changing port and restarting services.
* Tested outside of devbox shell
* Tested inside of devbox shell
* Tested with direnv enabled
* Tested with direnv disabled

Author: mikeland73

devbox.go

@@ -34,12 +34,12 @@ type Devbox interface {
 	Update(ctx context.Context, opts devopt.UpdateOpts) error
 
 	// Interact with services
-	ListServices(ctx context.Context) error
-	RestartServices(ctx context.Context, services ...string) error
+	ListServices(ctx context.Context, runInCurrentShell bool) error
+	RestartServices(ctx context.Context, runInCurrentShell bool, services ...string) error
 	Services() (services.Services, error)
-	StartProcessManager(ctx context.Context, requestedServices []string, background bool, processComposeFileOrDir string) error
-	StartServices(ctx context.Context, services ...string) error
-	StopServices(ctx context.Context, allProjects bool, services ...string) error
+	StartProcessManager(ctx context.Context, runInCurrentShell bool, requestedServices []string, background bool, processComposeFileOrDir string) error
+	StartServices(ctx context.Context, runInCurrentShell bool, services ...string) error
+	StopServices(ctx context.Context, runInCurrentShell, allProjects bool, services ...string) error
 
 	// Generate files
 	Generate(ctx context.Context) error

examples/stacks/lapp-stack/devbox.json

@@ -7,7 +7,8 @@
     "[email protected]"
   ],
   "env": {
-    "PGPORT": "5432"
+    "PGPORT": "5432",
+    "PGHOST": "/tmp/devbox/lapp"
   },
   "shell": {
     "scripts": {
@@ -19,7 +20,6 @@
       "init_db": "initdb",
       "run_test": [
         "mkdir -p /tmp/devbox/lapp",
-        "export PGHOST=/tmp/devbox/lapp",
         "initdb",
         "devbox services start",
         "echo 'sleep 1 second for the postgres server to initialize.' && sleep 1",

examples/stacks/lepp-stack/devbox.json

@@ -9,7 +9,8 @@
   "env": {
     "NGINX_WEB_PORT": "8089",
     "NGINX_WEB_ROOT": "../../../my_app",
-    "PGPORT": "5433"
+    "PGPORT": "5433",
+    "PGHOST": "/tmp/devbox/lepp"
   },
   "shell": {
     "scripts": {
@@ -21,7 +22,7 @@
       "init_db": "initdb",
       "run_test": [
         "mkdir -p /tmp/devbox/lepp",
-        "export PGHOST=/tmp/devbox/lepp",
+        "rm -rf .devbox/virtenv/postgresql/data",
         "initdb",
         "devbox services up -b",
         "echo 'sleep 2 second for the postgres server to initialize.' && sleep 2",

examples/stacks/lepp-stack/devbox.lock

@@ -22,7 +22,7 @@
     },
     "nginx@latest": {
       "last_modified": "2023-09-04T16:24:30Z",
-      "plugin_version": "0.0.3",
+      "plugin_version": "0.0.4",
       "resolved": "github:NixOS/nixpkgs/3c15feef7770eb5500a4b8792623e2d6f598c9c1#nginx",
       "source": "devbox-search",
       "version": "1.24.0",

internal/boxcli/run.go

@@ -5,9 +5,7 @@ package boxcli
 
 import (
 	"fmt"
-	"os"
 	"slices"
-	"strconv"
 	"strings"
 
 	"github.com/samber/lo"
@@ -99,19 +97,12 @@ func runScriptCmd(cmd *cobra.Command, args []string, flags runCmdFlags) error {
 		return err
 	}
 
-	omitBinWrappersFromPath := true
-	// This is for testing. Once we completely remove bin wrappers we can remove
-	// this. It helps simulate shell using "run".
-	if ok, _ := strconv.ParseBool(os.Getenv("DEVBOX_INCLUDE_BIN_WRAPPERS_IN_PATH")); ok {
-		omitBinWrappersFromPath = false
-	}
 	// Check the directory exists.
 	box, err := devbox.Open(&devopt.Opts{
-		Dir:                     path,
-		Stderr:                  cmd.ErrOrStderr(),
-		Pure:                    flags.pure,
-		Env:                     env,
-		OmitBinWrappersFromPath: omitBinWrappersFromPath,
+		Dir:    path,
+		Stderr: cmd.ErrOrStderr(),
+		Pure:   flags.pure,
+		Env:    env,
 	})
 	if err != nil {
 		return redact.Errorf("error reading devbox.json: %w", err)

internal/boxcli/services.go

@@ -12,7 +12,8 @@ import (
 
 type servicesCmdFlags struct {
 	envFlag
-	config configFlags
+	config            configFlags
+	runInCurrentShell bool
 }
 
 type serviceUpFlags struct {
@@ -47,7 +48,13 @@ func servicesCmd(persistentPreRunE ...cobraFunc) *cobra.Command {
 	serviceStopFlags := serviceStopFlags{}
 	servicesCommand := &cobra.Command{
 		Use:   "services",
-		Short: "Interact with devbox services",
+		Short: "Interact with devbox services.",
+		Long: "Interact with devbox services. Services start in a new shell. " +
+			"Plugin services use environment variables specified by plugin unless " +
+			"overridden by the user. To override plugin environment variables, use " +
+			"the --env or --env-file flag. You may also override in devbox.json by " +
+			"using the `env` field or exporting an environment variable in the " +
+			"init hook.",
 		PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
 			preruns := append([]cobraFunc{ensureNixInstalled}, persistentPreRunE...)
 			for _, fn := range preruns {
@@ -105,6 +112,13 @@ func servicesCmd(persistentPreRunE ...cobraFunc) *cobra.Command {
 
 	flags.envFlag.register(servicesCommand)
 	flags.config.registerPersistent(servicesCommand)
+	servicesCommand.PersistentFlags().BoolVar(
+		&flags.runInCurrentShell,
+		"run-in-current-shell",
+		false,
+		"Run the command in the current shell instead of a new shell",
+	)
+	servicesCommand.Flag("run-in-current-shell").Hidden = true
 	serviceUpFlags.register(upCommand)
 	serviceStopFlags.register(stopCommand)
 	servicesCommand.AddCommand(lsCommand)
@@ -124,7 +138,7 @@ func listServices(cmd *cobra.Command, flags servicesCmdFlags) error {
 		return errors.WithStack(err)
 	}
 
-	return box.ListServices(cmd.Context())
+	return box.ListServices(cmd.Context(), flags.runInCurrentShell)
 }
 
 func startServices(cmd *cobra.Command, services []string, flags servicesCmdFlags) error {
@@ -141,7 +155,7 @@ func startServices(cmd *cobra.Command, services []string, flags servicesCmdFlags
 		return errors.WithStack(err)
 	}
 
-	return box.StartServices(cmd.Context(), services...)
+	return box.StartServices(cmd.Context(), flags.runInCurrentShell, services...)
 }
 
 func stopServices(
@@ -165,7 +179,8 @@ func stopServices(
 	if len(services) > 0 && flags.allProjects {
 		return errors.New("cannot use both services and --all-projects arguments simultaneously")
 	}
-	return box.StopServices(cmd.Context(), flags.allProjects, services...)
+	return box.StopServices(
+		cmd.Context(), servicesFlags.runInCurrentShell, flags.allProjects, services...)
 }
 
 func restartServices(
@@ -186,7 +201,7 @@ func restartServices(
 		return errors.WithStack(err)
 	}
 
-	return box.RestartServices(cmd.Context(), services...)
+	return box.RestartServices(cmd.Context(), flags.runInCurrentShell, services...)
 }
 
 func startProcessManager(
@@ -209,5 +224,11 @@ func startProcessManager(
 		return errors.WithStack(err)
 	}
 
-	return box.StartProcessManager(cmd.Context(), args, flags.background, flags.processComposeFile)
+	return box.StartProcessManager(
+		cmd.Context(),
+		servicesFlags.runInCurrentShell,
+		args,
+		flags.background,
+		flags.processComposeFile,
+	)
 }

internal/impl/devbox.go

@@ -30,6 +30,7 @@ import (
 	"go.jetpack.io/devbox/internal/searcher"
 	"go.jetpack.io/devbox/internal/shellgen"
 	"go.jetpack.io/devbox/internal/telemetry"
+	"go.jetpack.io/devbox/internal/wrapnix"
 
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/cmdutil"
@@ -67,7 +68,6 @@ type Devbox struct {
 	pure                     bool
 	allowInsecureAdds        bool
 	customProcessComposeFile string
-	OmitBinWrappersFromPath  bool
 
 	// This is needed because of the --quiet flag.
 	stderr io.Writer
@@ -98,7 +98,6 @@ func Open(opts *devopt.Opts) (*Devbox, error) {
 		pure:                     opts.Pure,
 		customProcessComposeFile: opts.CustomProcessComposeFile,
 		allowInsecureAdds:        opts.AllowInsecureAdds,
-		OmitBinWrappersFromPath:  opts.OmitBinWrappersFromPath,
 	}
 
 	lock, err := lock.GetFile(box)
@@ -221,6 +220,19 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string
 	if err != nil {
 		return err
 	}
+
+	// By default we always remove bin wrappers when using `run`. This env var is
+	// for testing. Once we completely remove bin wrappers we can remove this.
+	// It helps simulate shell using "run".
+	if includeBinWrappers, _ := strconv.ParseBool(
+		os.Getenv("DEVBOX_INCLUDE_BIN_WRAPPERS_IN_PATH"),
+	); !includeBinWrappers {
+		env["PATH"] = envpath.RemoveFromPath(
+			env["PATH"],
+			wrapnix.WrapperBinPath(d.projectDir),
+		)
+	}
+
 	// Used to determine whether we're inside a shell (e.g. to prevent shell inception)
 	// This is temporary because StartServices() needs it but should be replaced with
 	// better alternative since devbox run and devbox shell are not the same.
@@ -526,15 +538,22 @@ func (d *Devbox) Services() (services.Services, error) {
 	return result, nil
 }
 
-func (d *Devbox) StartServices(ctx context.Context, serviceNames ...string) error {
-	if !d.IsEnvEnabled() {
-		return d.RunScript(ctx, "devbox", append([]string{"services", "start"}, serviceNames...))
+func (d *Devbox) StartServices(
+	ctx context.Context, runInCurrentShell bool, serviceNames ...string,
+) error {
+	if !runInCurrentShell {
+		return d.RunScript(ctx, "devbox",
+			append(
+				[]string{"services", "start", "--run-in-current-shell"},
+				serviceNames...,
+			),
+		)
 	}
 
 	if !services.ProcessManagerIsRunning(d.projectDir) {
 		fmt.Fprintln(d.stderr, "Process-compose is not running. Starting it now...")
 		fmt.Fprintln(d.stderr, "\nNOTE: We recommend using `devbox services up` to start process-compose and your services")
-		return d.StartProcessManager(ctx, serviceNames, true, "")
+		return d.StartProcessManager(ctx, runInCurrentShell, serviceNames, true, "")
 	}
 
 	svcSet, err := d.Services()
@@ -563,9 +582,9 @@ func (d *Devbox) StartServices(ctx context.Context, serviceNames ...string) erro
 	return nil
 }
 
-func (d *Devbox) StopServices(ctx context.Context, allProjects bool, serviceNames ...string) error {
-	if !d.IsEnvEnabled() {
-		args := []string{"services", "stop"}
+func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell, allProjects bool, serviceNames ...string) error {
+	if !runInCurrentShell {
+		args := []string{"services", "stop", "--run-in-current-shell"}
 		args = append(args, serviceNames...)
 		if allProjects {
 			args = append(args, "--all-projects")
@@ -602,9 +621,10 @@ func (d *Devbox) StopServices(ctx context.Context, allProjects bool, serviceName
 	return nil
 }
 
-func (d *Devbox) ListServices(ctx context.Context) error {
-	if !d.IsEnvEnabled() {
-		return d.RunScript(ctx, "devbox", []string{"services", "ls"})
+func (d *Devbox) ListServices(ctx context.Context, runInCurrentShell bool) error {
+	if !runInCurrentShell {
+		return d.RunScript(ctx,
+			"devbox", []string{"services", "ls", "--run-in-current-shell"})
 	}
 
 	svcSet, err := d.Services()
@@ -640,15 +660,22 @@ func (d *Devbox) ListServices(ctx context.Context) error {
 	return nil
 }
 
-func (d *Devbox) RestartServices(ctx context.Context, serviceNames ...string) error {
-	if !d.IsEnvEnabled() {
-		return d.RunScript(ctx, "devbox", append([]string{"services", "restart"}, serviceNames...))
+func (d *Devbox) RestartServices(
+	ctx context.Context, runInCurrentShell bool, serviceNames ...string,
+) error {
+	if !runInCurrentShell {
+		return d.RunScript(ctx, "devbox",
+			append(
+				[]string{"services", "restart", "--run-in-current-shell"},
+				serviceNames...,
+			),
+		)
 	}
 
 	if !services.ProcessManagerIsRunning(d.projectDir) {
 		fmt.Fprintln(d.stderr, "Process-compose is not running. Starting it now...")
 		fmt.Fprintln(d.stderr, "\nTip: We recommend using `devbox services up` to start process-compose and your services")
-		return d.StartProcessManager(ctx, serviceNames, true, "")
+		return d.StartProcessManager(ctx, runInCurrentShell, serviceNames, true, "")
 	}
 
 	// TODO: Restart with no services should restart the _currently running_ services. This means we should get the list of running services from the process-compose, then restart them all.
@@ -674,12 +701,13 @@ func (d *Devbox) RestartServices(ctx context.Context, serviceNames ...string) er
 
 func (d *Devbox) StartProcessManager(
 	ctx context.Context,
+	runInCurrentShell bool,
 	requestedServices []string,
 	background bool,
 	processComposeFileOrDir string,
 ) error {
-	if !d.IsEnvEnabled() {
-		args := []string{"services", "up"}
+	if !runInCurrentShell {
+		args := []string{"services", "up", "--run-in-current-shell"}
 		args = append(args, requestedServices...)
 		if processComposeFileOrDir != "" {
 			args = append(args, "--process-compose-file", processComposeFileOrDir)
@@ -853,17 +881,11 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m
 	addEnvIfNotPreviouslySetByDevbox(env, pluginEnv)
 
 	env["PATH"] = envpath.JoinPathLists(
+		filepath.Join(d.projectDir, plugin.WrapperBinPath),
 		nix.ProfileBinPath(d.projectDir),
 		env["PATH"],
 	)
 
-	if !d.OmitBinWrappersFromPath {
-		env["PATH"] = envpath.JoinPathLists(
-			filepath.Join(d.projectDir, plugin.WrapperBinPath),
-			env["PATH"],
-		)
-	}
-
 	env["PATH"], err = d.addUtilitiesToPath(ctx, env["PATH"])
 	if err != nil {
 		return nil, err

internal/impl/devopt/devboxopts.go

@@ -12,7 +12,6 @@ type Opts struct {
 	Pure                     bool
 	IgnoreWarnings           bool
 	CustomProcessComposeFile string
-	OmitBinWrappersFromPath  bool
 	Stderr                   io.Writer
 }
 

internal/impl/envpath/pathlists.go

@@ -1,6 +1,7 @@
 package envpath
 
 import (
+	"os"
 	"path/filepath"
 	"strings"
 )
@@ -35,3 +36,22 @@ func JoinPathLists(pathLists ...string) string {
 	}
 	return strings.Join(cleaned, string(filepath.ListSeparator))
 }
+
+func RemoveFromPath(path, pathToRemove string) string {
+	paths := filepath.SplitList(path)
+
+	// Create a new slice to store the modified paths
+	var newPaths []string
+
+	// Iterate through the paths and add them to the newPaths slice if they are not equal to pathToRemove
+	for _, p := range paths {
+		if p != pathToRemove {
+			newPaths = append(newPaths, p)
+		}
+	}
+
+	// Join the modified paths using ":" as the delimiter
+	newPath := strings.Join(newPaths, string(os.PathListSeparator))
+
+	return newPath
+}