From 1573ae8f82a8c9a8c17d65ee2096193e8106ac73 Mon Sep 17 00:00:00 2001 From: savil <676452+savil@users.noreply.github.com> Date: Wed, 19 Jun 2024 15:20:39 -0700 Subject: [PATCH] [Devbox] introduce devopt.EnvOptions (#2159) ## Summary From the EnvOptions docblock: ``` // EnvOptions configure the Devbox Environment in the `computeEnv` function. // - These options are commonly set by flags in some Devbox commands // like `shellenv`, `shell` and `run`. // - The struct is designed for the "common case" to be zero-initialized as `EnvOptions{}`. ``` This gets rid of the pseudo-global state in the `Devbox` struct where we were setting `pure`, `preservePathStack` and `omitNixEnv` values. ## How was it tested? TODO: - [x] CICD should pass - [x] Ensure `devbox global` and `devbox` omitNixEnv works as expected - [x] Ensure `pure` works as expected for `devbox shell --pure`. --- internal/boxcli/run.go | 9 ++-- internal/boxcli/shell.go | 7 +-- internal/boxcli/shellenv.go | 20 ++++---- internal/devbox/devbox.go | 68 ++++++++++++++++------------ internal/devbox/devbox_test.go | 12 ++--- internal/devbox/devopt/devboxopts.go | 14 ++++-- internal/devbox/shell.go | 9 ++-- 7 files changed, 80 insertions(+), 59 deletions(-) diff --git a/internal/boxcli/run.go b/internal/boxcli/run.go index d154c5cbcf1..ac4f832b2af 100644 --- a/internal/boxcli/run.go +++ b/internal/boxcli/run.go @@ -73,7 +73,6 @@ func listScripts(cmd *cobra.Command, flags runCmdFlags) []string { Dir: flags.config.path, Environment: flags.config.environment, Stderr: cmd.ErrOrStderr(), - Pure: flags.pure, IgnoreWarnings: true, }) if err != nil { @@ -114,15 +113,17 @@ func runScriptCmd(cmd *cobra.Command, args []string, flags runCmdFlags) error { Dir: path, Environment: flags.config.environment, Stderr: cmd.ErrOrStderr(), - OmitNixEnv: flags.omitNixEnv, - Pure: flags.pure, Env: env, }) if err != nil { return redact.Errorf("error reading devbox.json: %w", err) } - if err := box.RunScript(cmd.Context(), script, scriptArgs); err != nil { + envOpts := devopt.EnvOptions{ + OmitNixEnv: flags.omitNixEnv, + Pure: flags.pure, + } + if err := box.RunScript(cmd.Context(), envOpts, script, scriptArgs); err != nil { return redact.Errorf("error running script %q in Devbox: %w", script, err) } return nil diff --git a/internal/boxcli/shell.go b/internal/boxcli/shell.go index b1aea2d06d9..48e0df4ed1f 100644 --- a/internal/boxcli/shell.go +++ b/internal/boxcli/shell.go @@ -69,8 +69,6 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error { Dir: flags.config.path, Env: env, Environment: flags.config.environment, - OmitNixEnv: flags.omitNixEnv, - Pure: flags.pure, Stderr: cmd.ErrOrStderr(), }) if err != nil { @@ -93,7 +91,10 @@ func runShellCmd(cmd *cobra.Command, flags shellCmdFlags) error { return shellInceptionErrorMsg("devbox shell") } - return box.Shell(cmd.Context()) + return box.Shell(cmd.Context(), devopt.EnvOptions{ + OmitNixEnv: flags.omitNixEnv, + Pure: flags.pure, + }) } func shellInceptionErrorMsg(cmdPath string) error { diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index 5b55de61ef1..228dbb46ebe 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -95,13 +95,10 @@ func shellEnvFunc( return "", err } box, err := devbox.Open(&devopt.Opts{ - Dir: flags.config.path, - Environment: flags.config.environment, - OmitNixEnv: flags.omitNixEnv, - Stderr: cmd.ErrOrStderr(), - PreservePathStack: flags.preservePathStack, - Pure: flags.pure, - Env: env, + Dir: flags.config.path, + Environment: flags.config.environment, + Stderr: cmd.ErrOrStderr(), + Env: env, }) if err != nil { return "", err @@ -115,8 +112,13 @@ func shellEnvFunc( envStr, err := box.EnvExports(cmd.Context(), devopt.EnvExportsOpts{ DontRecomputeEnvironment: !flags.recomputeEnv, - NoRefreshAlias: flags.noRefreshAlias, - RunHooks: flags.runInitHook, + EnvOptions: devopt.EnvOptions{ + OmitNixEnv: flags.omitNixEnv, + PreservePathStack: flags.preservePathStack, + Pure: flags.pure, + }, + NoRefreshAlias: flags.noRefreshAlias, + RunHooks: flags.runInitHook, }) if err != nil { return "", err diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 2ed8e9d3deb..8fd6473d200 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -59,14 +59,11 @@ const ( type Devbox struct { cfg *devconfig.Config env map[string]string - omitNixEnv bool environment string lockfile *lock.File nix nix.Nixer projectDir string pluginManager *plugin.Manager - preservePathStack bool - pure bool customProcessComposeFile string // This is needed because of the --quiet flag. @@ -98,14 +95,11 @@ func Open(opts *devopt.Opts) (*Devbox, error) { box := &Devbox{ cfg: cfg, env: opts.Env, - omitNixEnv: opts.OmitNixEnv, environment: environment, nix: &nix.Nix{}, projectDir: projectDir, pluginManager: plugin.NewManager(), stderr: opts.Stderr, - preservePathStack: opts.PreservePathStack, - pure: opts.Pure, customProcessComposeFile: opts.CustomProcessComposeFile, } @@ -200,11 +194,11 @@ func (d *Devbox) Generate(ctx context.Context) error { return errors.WithStack(shellgen.GenerateForPrintEnv(ctx, d)) } -func (d *Devbox) Shell(ctx context.Context) error { +func (d *Devbox) Shell(ctx context.Context, envOpts devopt.EnvOptions) error { ctx, task := trace.NewTask(ctx, "devboxShell") defer task.End() - envs, err := d.ensureStateIsUpToDateAndComputeEnv(ctx) + envs, err := d.ensureStateIsUpToDateAndComputeEnv(ctx, envOpts) if err != nil { return err } @@ -228,7 +222,7 @@ func (d *Devbox) Shell(ctx context.Context) error { WithShellStartTime(telemetry.ShellStart()), } - shell, err := NewDevboxShell(d, opts...) + shell, err := NewDevboxShell(d, envOpts, opts...) if err != nil { return err } @@ -236,7 +230,14 @@ func (d *Devbox) Shell(ctx context.Context) error { return shell.Run() } -func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string) error { +// runDevboxServicesScript invokes RunScript with the envOptions set to the appropriate +// defaults for the `devbox services` scenario. +// TODO: move to services.go +func (d *Devbox) runDevboxServicesScript(ctx context.Context, cmdArgs []string) error { + return d.RunScript(ctx, devopt.EnvOptions{}, "devbox", cmdArgs) +} + +func (d *Devbox) RunScript(ctx context.Context, envOpts devopt.EnvOptions, cmdName string, cmdArgs []string) error { ctx, task := trace.NewTask(ctx, "devboxRun") defer task.End() @@ -256,7 +257,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string env[d.SkipInitHookEnvName()] = "true" } else { var err error - env, err = d.ensureStateIsUpToDateAndComputeEnv(ctx) + env, err = d.ensureStateIsUpToDateAndComputeEnv(ctx, envOpts) if err != nil { return err } @@ -341,9 +342,9 @@ func (d *Devbox) EnvExports(ctx context.Context, opts devopt.EnvExportsOpts) (st ) } - envs, err = d.computeEnv(ctx, true /*usePrintDevEnvCache*/) + envs, err = d.computeEnv(ctx, true /*usePrintDevEnvCache*/, opts.EnvOptions) } else { - envs, err = d.ensureStateIsUpToDateAndComputeEnv(ctx) + envs, err = d.ensureStateIsUpToDateAndComputeEnv(ctx, opts.EnvOptions) } if err != nil { @@ -368,7 +369,7 @@ func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { ctx, task := trace.NewTask(ctx, "devboxEnvVars") defer task.End() // this only returns env variables for the shell environment excluding hooks - envs, err := d.ensureStateIsUpToDateAndComputeEnv(ctx) + envs, err := d.ensureStateIsUpToDateAndComputeEnv(ctx, devopt.EnvOptions{}) if err != nil { return nil, err } @@ -578,11 +579,12 @@ func (d *Devbox) Services() (services.Services, error) { return result, nil } +// TODO: move to services.go func (d *Devbox) StartServices( ctx context.Context, runInCurrentShell bool, serviceNames ...string, ) error { if !runInCurrentShell { - return d.RunScript(ctx, "devbox", + return d.runDevboxServicesScript(ctx, append( []string{"services", "start", "--run-in-current-shell"}, serviceNames..., @@ -622,6 +624,7 @@ func (d *Devbox) StartServices( return nil } +// TODO: move to services.go func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell, allProjects bool, serviceNames ...string) error { if !runInCurrentShell { args := []string{"services", "stop", "--run-in-current-shell"} @@ -629,7 +632,7 @@ func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell, allProject if allProjects { args = append(args, "--all-projects") } - return d.RunScript(ctx, "devbox", args) + return d.runDevboxServicesScript(ctx, args) } if allProjects { @@ -661,10 +664,10 @@ func (d *Devbox) StopServices(ctx context.Context, runInCurrentShell, allProject return nil } +// TODO: move to services.go func (d *Devbox) ListServices(ctx context.Context, runInCurrentShell bool) error { if !runInCurrentShell { - return d.RunScript(ctx, - "devbox", []string{"services", "ls", "--run-in-current-shell"}) + return d.runDevboxServicesScript(ctx, []string{"services", "ls", "--run-in-current-shell"}) } svcSet, err := d.Services() @@ -700,11 +703,12 @@ func (d *Devbox) ListServices(ctx context.Context, runInCurrentShell bool) error return nil } +// TODO: move to services.go func (d *Devbox) RestartServices( ctx context.Context, runInCurrentShell bool, serviceNames ...string, ) error { if !runInCurrentShell { - return d.RunScript(ctx, "devbox", + return d.runDevboxServicesScript(ctx, append( []string{"services", "restart", "--run-in-current-shell"}, serviceNames..., @@ -739,6 +743,7 @@ func (d *Devbox) RestartServices( return nil } +// TODO: move to services.go func (d *Devbox) StartProcessManager( ctx context.Context, runInCurrentShell bool, @@ -762,7 +767,7 @@ func (d *Devbox) StartProcessManager( args = append(args, "--pcflags", flag) } - return d.RunScript(ctx, "devbox", args) + return d.runDevboxServicesScript(ctx, args) } svcs, err := d.Services() @@ -886,13 +891,17 @@ func (d *Devbox) execPrintDevEnv(ctx context.Context, usePrintDevEnvCache bool) // Note that the shellrc.tmpl template (which sources this environment) does // some additional processing. The computeEnv environment won't necessarily // represent the final "devbox run" or "devbox shell" environments. -func (d *Devbox) computeEnv(ctx context.Context, usePrintDevEnvCache bool) (map[string]string, error) { +func (d *Devbox) computeEnv( + ctx context.Context, + usePrintDevEnvCache bool, + envOpts devopt.EnvOptions, +) (map[string]string, error) { defer debug.FunctionTimer().End() defer trace.StartRegion(ctx, "devboxComputeEnv").End() // Append variables from current env if --pure is not passed currentEnv := os.Environ() - env, err := d.parseEnvAndExcludeSpecialCases(currentEnv) + env, err := d.parseEnvAndExcludeSpecialCases(currentEnv, envOpts.Pure) if err != nil { return nil, err } @@ -910,7 +919,7 @@ func (d *Devbox) computeEnv(ctx context.Context, usePrintDevEnvCache bool) (map[ originalEnv := make(map[string]string, len(env)) maps.Copy(originalEnv, env) - if !d.omitNixEnv { + if !envOpts.OmitNixEnv { nixEnv, err := d.execPrintDevEnv(ctx, usePrintDevEnvCache) if err != nil { return nil, err @@ -989,13 +998,13 @@ func (d *Devbox) computeEnv(ctx context.Context, usePrintDevEnvCache bool) (map[ devboxEnvPath = envpath.JoinPathLists(devboxEnvPath, runXPaths) pathStack := envpath.Stack(env, originalEnv) - pathStack.Push(env, d.ProjectDirHash(), devboxEnvPath, d.preservePathStack) + pathStack.Push(env, d.ProjectDirHash(), devboxEnvPath, envOpts.PreservePathStack) env["PATH"] = pathStack.Path(env) slog.Debug("new path stack is", "path_stack", pathStack) slog.Debug("computed environment PATH", "path", env["PATH"]) - if !d.pure { + if !envOpts.Pure { // preserve the original XDG_DATA_DIRS by prepending to it env["XDG_DATA_DIRS"] = envpath.JoinPathLists(env["XDG_DATA_DIRS"], os.Getenv("XDG_DATA_DIRS")) } @@ -1011,6 +1020,7 @@ func (d *Devbox) computeEnv(ctx context.Context, usePrintDevEnvCache bool) (map[ // while ensuring these reflect the current (up to date) state of the project. func (d *Devbox) ensureStateIsUpToDateAndComputeEnv( ctx context.Context, + envOpts devopt.EnvOptions, ) (map[string]string, error) { defer debug.FunctionTimer().End() @@ -1037,7 +1047,7 @@ func (d *Devbox) ensureStateIsUpToDateAndComputeEnv( // it's ok to use usePrintDevEnvCache=true here always. This does end up // doing some non-nix work twice if lockfile is not up to date. // TODO: Improve this to avoid extra work. - return d.computeEnv(ctx, true /*usePrintDevEnvCache*/) + return d.computeEnv(ctx, true /*usePrintDevEnvCache*/, envOpts) } func (d *Devbox) nixPrintDevEnvCachePath() string { @@ -1256,7 +1266,7 @@ func (d *Devbox) addHashToEnv(env map[string]string) error { // parseEnvAndExcludeSpecialCases converts env as []string to map[string]string // In case of pure shell, it leaks HOME and it leaks PATH with some modifications -func (d *Devbox) parseEnvAndExcludeSpecialCases(currentEnv []string) (map[string]string, error) { +func (d *Devbox) parseEnvAndExcludeSpecialCases(currentEnv []string, pure bool) (map[string]string, error) { env := make(map[string]string, len(currentEnv)) for _, kv := range currentEnv { key, val, found := strings.Cut(kv, "=") @@ -1270,13 +1280,13 @@ func (d *Devbox) parseEnvAndExcludeSpecialCases(currentEnv []string) (map[string // - HOME required for devbox binary to work // - PATH to find the nix installation. It is cleaned for pure mode below. // - TERM to enable colored text in the pure shell - if !d.pure || key == "HOME" || key == "PATH" || key == "TERM" { + if !pure || key == "HOME" || key == "PATH" || key == "TERM" { env[key] = val } } // handling special case for PATH - if d.pure { + if pure { // Setting a custom env variable to differentiate pure and regular shell env["DEVBOX_PURE_SHELL"] = "1" // Finding nix executables in path and passing it through diff --git a/internal/devbox/devbox_test.go b/internal/devbox/devbox_test.go index 1a58691f488..579b5b7a496 100644 --- a/internal/devbox/devbox_test.go +++ b/internal/devbox/devbox_test.go @@ -46,7 +46,6 @@ func testShellPlan(t *testing.T, testPath string) { _, err := Open(&devopt.Opts{ Dir: baseDir, Stderr: os.Stderr, - Pure: false, }) assert.NoErrorf(err, "%s should be a valid devbox project", baseDir) }) @@ -71,7 +70,7 @@ func TestComputeEnv(t *testing.T) { d := devboxForTesting(t) d.nix = &testNix{} ctx := context.Background() - env, err := d.computeEnv(ctx, false /*use cache*/) + env, err := d.computeEnv(ctx, false /*use cache*/, devopt.EnvOptions{}) require.NoError(t, err, "computeEnv should not fail") assert.NotNil(t, env, "computeEnv should return a valid env") } @@ -80,7 +79,7 @@ func TestComputeDevboxPathIsIdempotent(t *testing.T) { devbox := devboxForTesting(t) devbox.nix = &testNix{"/tmp/my/path"} ctx := context.Background() - env, err := devbox.computeEnv(ctx, false /*use cache*/) + env, err := devbox.computeEnv(ctx, false /*use cache*/, devopt.EnvOptions{}) require.NoError(t, err, "computeEnv should not fail") path := env["PATH"] assert.NotEmpty(t, path, "path should not be nil") @@ -90,7 +89,7 @@ func TestComputeDevboxPathIsIdempotent(t *testing.T) { t.Setenv(envpath.PathStackEnv, env[envpath.PathStackEnv]) t.Setenv(envpath.Key(devbox.ProjectDirHash()), env[envpath.Key(devbox.ProjectDirHash())]) - env, err = devbox.computeEnv(ctx, false /*use cache*/) + env, err = devbox.computeEnv(ctx, false /*use cache*/, devopt.EnvOptions{}) require.NoError(t, err, "computeEnv should not fail") path2 := env["PATH"] @@ -101,7 +100,7 @@ func TestComputeDevboxPathWhenRemoving(t *testing.T) { devbox := devboxForTesting(t) devbox.nix = &testNix{"/tmp/my/path"} ctx := context.Background() - env, err := devbox.computeEnv(ctx, false /*use cache*/) + env, err := devbox.computeEnv(ctx, false /*use cache*/, devopt.EnvOptions{}) require.NoError(t, err, "computeEnv should not fail") path := env["PATH"] assert.NotEmpty(t, path, "path should not be nil") @@ -113,7 +112,7 @@ func TestComputeDevboxPathWhenRemoving(t *testing.T) { t.Setenv(envpath.Key(devbox.ProjectDirHash()), env[envpath.Key(devbox.ProjectDirHash())]) devbox.nix.(*testNix).path = "" - env, err = devbox.computeEnv(ctx, false /*use cache*/) + env, err = devbox.computeEnv(ctx, false /*use cache*/, devopt.EnvOptions{}) require.NoError(t, err, "computeEnv should not fail") path2 := env["PATH"] assert.NotContains(t, path2, "/tmp/my/path", "path should not contain /tmp/my/path") @@ -128,7 +127,6 @@ func devboxForTesting(t *testing.T) *Devbox { d, err := Open(&devopt.Opts{ Dir: path, Stderr: os.Stderr, - Pure: false, }) require.NoError(t, err, "Open should not fail") diff --git a/internal/devbox/devopt/devboxopts.go b/internal/devbox/devopt/devboxopts.go index f46f334c9d5..d5fbf978a8a 100644 --- a/internal/devbox/devopt/devboxopts.go +++ b/internal/devbox/devopt/devboxopts.go @@ -12,9 +12,6 @@ type Opts struct { Dir string Env map[string]string Environment string - OmitNixEnv bool - PreservePathStack bool - Pure bool IgnoreWarnings bool CustomProcessComposeFile string Stderr io.Writer @@ -65,6 +62,17 @@ type UpdateOpts struct { type EnvExportsOpts struct { DontRecomputeEnvironment bool + EnvOptions EnvOptions NoRefreshAlias bool RunHooks bool } + +// EnvOptions configure the Devbox Environment in the `computeEnv` function. +// - These options are commonly set by flags in some Devbox commands +// like `shellenv`, `shell` and `run`. +// - The struct is designed for the "common case" to be zero-initialized as `EnvOptions{}`. +type EnvOptions struct { + OmitNixEnv bool + PreservePathStack bool + Pure bool +} diff --git a/internal/devbox/shell.go b/internal/devbox/shell.go index 8e94e96f234..7b3aeff2d1a 100644 --- a/internal/devbox/shell.go +++ b/internal/devbox/shell.go @@ -18,6 +18,7 @@ import ( "github.com/alessio/shellescape" "github.com/pkg/errors" + "go.jetpack.io/devbox/internal/devbox/devopt" "go.jetpack.io/devbox/internal/shellgen" "go.jetpack.io/devbox/internal/telemetry" @@ -70,8 +71,8 @@ type ShellOption func(*DevboxShell) // NewDevboxShell initializes the DevboxShell struct so it can be used to start a shell environment // for the devbox project. -func NewDevboxShell(devbox *Devbox, opts ...ShellOption) (*DevboxShell, error) { - shPath, err := shellPath(devbox) +func NewDevboxShell(devbox *Devbox, envOpts devopt.EnvOptions, opts ...ShellOption) (*DevboxShell, error) { + shPath, err := shellPath(devbox, envOpts) if err != nil { return nil, err } @@ -87,14 +88,14 @@ func NewDevboxShell(devbox *Devbox, opts ...ShellOption) (*DevboxShell, error) { } // shellPath returns the path to a shell binary, or error if none found. -func shellPath(devbox *Devbox) (path string, err error) { +func shellPath(devbox *Devbox, envOpts devopt.EnvOptions) (path string, err error) { defer func() { if err != nil { path = filepath.Clean(path) } }() - if !devbox.pure { + if !envOpts.Pure { // First, check the SHELL environment variable. path = os.Getenv(envir.Shell) if path != "" {