From c350e4ccbb38dcde37332a926e3e86e259e58746 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Wed, 4 Oct 2023 10:49:34 -0700 Subject: [PATCH 1/6] [RFC] [refactor] Consolidate profile update code under syncPackagesToProfile --- internal/impl/packages.go | 264 +++++++++++++------------------------- internal/impl/update.go | 6 - 2 files changed, 90 insertions(+), 180 deletions(-) diff --git a/internal/impl/packages.go b/internal/impl/packages.go index 4d49ca11c71..a2302632d7e 100644 --- a/internal/impl/packages.go +++ b/internal/impl/packages.go @@ -8,7 +8,6 @@ import ( "fmt" "io/fs" "os" - "os/exec" "path/filepath" "runtime/trace" "slices" @@ -188,18 +187,11 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { return err } - if err := d.removePackagesFromProfile(ctx, packagesToUninstall); err != nil { - return err - } - + // this will clean up the now-extra package from nix profile and the lockfile if err := d.ensurePackagesAreInstalled(ctx, uninstall); err != nil { return err } - if err := d.lockfile.Remove(packagesToUninstall...); err != nil { - return err - } - return d.saveCfg() } @@ -215,15 +207,19 @@ const ( // ensurePackagesAreInstalled ensures that the nix profile has the packages specified // in the config (devbox.json). The `mode` is used for user messaging to explain // what operations are happening, because this function may take time to execute. +// TODO we should rename this to ensureDevboxEnvironmentIsUpToDate since it does +// much more than ensuring packages are installed. func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMode) error { defer trace.StartRegion(ctx, "ensurePackages").End() defer debug.FunctionTimer().End() - if upToDate, err := d.lockfile.IsUpToDateAndInstalled(); err != nil || upToDate { - return err - } - + // if mode is install or uninstall, then we need to update the nix-profile + // and lockfile, so we must continue below. if mode == ensure { + // if mode is ensure, then we only continue if needed. + if upToDate, err := d.lockfile.IsUpToDateAndInstalled(); err != nil || upToDate { + return err + } fmt.Fprintln(d.stderr, "Ensuring packages are installed.") } @@ -294,30 +290,98 @@ func (d *Devbox) profilePath() (string, error) { // and no more. func (d *Devbox) syncPackagesToProfile(ctx context.Context, mode installMode) error { defer debug.FunctionTimer().End() - // TODO: we can probably merge these two operations to be faster and minimize chances of - // the devbox.json and nix profile falling out of sync. - if err := d.addPackagesToProfile(ctx, mode); err != nil { + defer trace.StartRegion(ctx, "syncPackagesToProfile").End() + + // First, fetch the profile items from the nix-profile, + // and get the installable packages + profileDir, err := d.profilePath() + if err != nil { + return err + } + profileItems, err := nixprofile.ProfileListItems(d.stderr, profileDir) + if err != nil { + return err + } + packages, err := d.AllInstallablePackages() + if err != nil { return err } - return d.tidyProfile(ctx) -} + if err := devpkg.FillNarInfoCache(ctx, packages...); err != nil { + return err + } -// addPackagesToProfile inspects the packages in devbox.json, checks which of them -// are missing from the nix profile, and then installs each package individually into the -// nix profile. -func (d *Devbox) addPackagesToProfile(ctx context.Context, mode installMode) error { - defer trace.StartRegion(ctx, "addNixProfilePkgs").End() + // Second, remove any packages from the nix-profile that are not in the config + itemsToKeep, err := d.removeExtraItemsFromProfile(ctx, profileDir, profileItems, packages) + if err != nil { + return err + } + // we are done if mode is uninstall if mode == uninstall { return nil } - pkgs, err := d.pendingPackagesForInstallation(ctx) - if err != nil { - return err + // Last, find the pending packages, and ensure they are added to the nix-profile + // Important to maintain the order of packages as specified by + // Devbox.InstallablePackages() (higher priority first) + pending := []*devpkg.Package{} + for _, pkg := range packages { + _, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{ + Items: itemsToKeep, + Lockfile: d.lockfile, + Writer: d.stderr, + Package: pkg, + ProfileDir: profileDir, + }) + if err != nil { + if !errors.Is(err, nix.ErrPackageNotFound) { + return err + } + pending = append(pending, pkg) + } } + return d.addPackagesToProfile(ctx, pending) +} + +func (d *Devbox) removeExtraItemsFromProfile( + ctx context.Context, + profileDir string, + profileItems []*nixprofile.NixProfileListItem, + packages []*devpkg.Package, +) ([]*nixprofile.NixProfileListItem, error) { + defer debug.FunctionTimer().End() + defer trace.StartRegion(ctx, "removeExtraPackagesFromProfile").End() + + itemsToKeep := []*nixprofile.NixProfileListItem{} + extras := []*nixprofile.NixProfileListItem{} + // Note: because devpkg.Package uses memoization when normalizing attribute paths (slow operation), + // and since we're reusing the Package objects, this O(n*m) loop becomes O(n+m) wrt the slow operation. +outer: + for _, item := range profileItems { + for _, pkg := range packages { + if item.Matches(pkg, d.lockfile) { + itemsToKeep = append(itemsToKeep, item) + continue outer + } + } + extras = append(extras, item) + } + // Remove by index to avoid comparing nix.ProfileListItem <> nix.Inputs again. + if err := nixprofile.ProfileRemoveItems(profileDir, extras); err != nil { + return nil, err + } + return itemsToKeep, nil +} + +// addPackagesToProfile inspects the packages in devbox.json, checks which of them +// are missing from the nix profile, and then installs each package individually into the +// nix profile. +func (d *Devbox) addPackagesToProfile(ctx context.Context, pkgs []*devpkg.Package) error { + defer debug.FunctionTimer().End() + defer trace.StartRegion(ctx, "addPackagesToProfile").End() + if len(pkgs) == 0 { return nil } @@ -363,154 +427,6 @@ func (d *Devbox) addPackagesToProfile(ctx context.Context, mode installMode) err return nil } -func (d *Devbox) removePackagesFromProfile(ctx context.Context, pkgs []string) error { - defer trace.StartRegion(ctx, "removeNixProfilePkgs").End() - - profileDir, err := d.profilePath() - if err != nil { - return err - } - - for _, pkg := range devpkg.PackageFromStrings(pkgs, d.lockfile) { - index, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{ - Lockfile: d.lockfile, - Writer: d.stderr, - Package: pkg, - ProfileDir: profileDir, - }) - if err != nil { - debug.Log( - "Info: Package %s not found in nix profile. Skipping removing from profile.\n", - pkg.Raw, - ) - continue - } - - // TODO: unify this with nix.ProfileRemove - cmd := exec.Command("nix", "profile", "remove", - "--profile", profileDir, - fmt.Sprintf("%d", index), - ) - cmd.Args = append(cmd.Args, nix.ExperimentalFlags()...) - cmd.Stdout = d.stderr - cmd.Stderr = d.stderr - err = cmd.Run() - if err != nil { - return err - } - } - return nil -} - -// tidyProfile removes any packages in the nix profile that are not in devbox.json. -func (d *Devbox) tidyProfile(ctx context.Context) error { - defer trace.StartRegion(ctx, "tidyProfile").End() - - extras, err := d.extraPackagesInProfile(ctx) - if err != nil { - return err - } - - profileDir, err := d.profilePath() - if err != nil { - return err - } - - // Remove by index to avoid comparing nix.ProfileListItem <> nix.Inputs again. - return nixprofile.ProfileRemoveItems(profileDir, extras) -} - -// pendingPackagesForInstallation returns a list of packages that are in -// devbox.json or global devbox.json but are not yet installed in the nix -// profile. It maintains the order of packages as specified by -// Devbox.AllPackages() (higher priority first) -func (d *Devbox) pendingPackagesForInstallation(ctx context.Context) ([]*devpkg.Package, error) { - defer trace.StartRegion(ctx, "pendingPackages").End() - - profileDir, err := d.profilePath() - if err != nil { - return nil, err - } - - pending := []*devpkg.Package{} - items, err := nixprofile.ProfileListItems(d.stderr, profileDir) - if err != nil { - return nil, err - } - packages, err := d.AllInstallablePackages() - if err != nil { - return nil, err - } - - // Fill the narinfo cache for all packages so we can check if they are in the - // binary cache. - if err := devpkg.FillNarInfoCache(ctx, packages...); err != nil { - return nil, err - } - - for _, pkg := range packages { - _, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{ - Items: items, - Lockfile: d.lockfile, - Writer: d.stderr, - Package: pkg, - ProfileDir: profileDir, - }) - if err != nil { - if !errors.Is(err, nix.ErrPackageNotFound) { - return nil, err - } - pending = append(pending, pkg) - } - } - return pending, nil -} - -// extraPackagesInProfile returns a list of packages that are in the nix profile, -// but are NOT in devbox.json or global devbox.json. -// -// NOTE: as an optimization, this implementation assumes that all packages in -// devbox.json have already been added to the nix profile. -func (d *Devbox) extraPackagesInProfile(ctx context.Context) ([]*nixprofile.NixProfileListItem, error) { - defer trace.StartRegion(ctx, "extraPackagesInProfile").End() - - profileDir, err := d.profilePath() - if err != nil { - return nil, err - } - - profileItems, err := nixprofile.ProfileListItems(d.stderr, profileDir) - if err != nil { - return nil, err - } - packages, err := d.AllInstallablePackages() - if err != nil { - return nil, err - } - - if len(packages) == len(profileItems) { - // Optimization: skip comparison if number of packages are the same. This only works - // because we assume that all packages in `devbox.json` have just been added to the - // profile. - return nil, nil - } - - extras := []*nixprofile.NixProfileListItem{} - // Note: because nix.Input uses memoization when normalizing attribute paths (slow operation), - // and since we're reusing the Input objects, this O(n*m) loop becomes O(n+m) wrt the slow operation. -outer: - for _, item := range profileItems { - for _, pkg := range packages { - if item.Matches(pkg, d.lockfile) { - continue outer - } - } - extras = append(extras, item) - } - - return extras, nil -} - var resetCheckDone = false // resetProfileDirForFlakes ensures the profileDir directory is cleared of old diff --git a/internal/impl/update.go b/internal/impl/update.go index 0bd9f38cae2..895af03fdc0 100644 --- a/internal/impl/update.go +++ b/internal/impl/update.go @@ -102,7 +102,6 @@ func (d *Devbox) updateDevboxPackage( } func (d *Devbox) mergeResolvedPackageToLockfile( - ctx context.Context, pkg *devpkg.Package, resolved *lock.Package, lockfile *lock.File, @@ -116,11 +115,6 @@ func (d *Devbox) mergeResolvedPackageToLockfile( if existing.Version != resolved.Version { ux.Finfo(d.stderr, "Updating %s %s -> %s\n", pkg, existing.Version, resolved.Version) - if err := d.removePackagesFromProfile(ctx, []string{pkg.Raw}); err != nil { - // Warn but continue. TODO(landau): ensurePackagesAreInstalled should - // sync the profile so we don't need to do this manually. - ux.Fwarning(d.stderr, "Failed to remove %s from profile: %s\n", pkg, err) - } useResolvedPackageInLockfile(lockfile, pkg, resolved, existing) return nil } From ef04e16cda80b1f24b7401890f680c2b3afbce08 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Wed, 4 Oct 2023 11:47:10 -0700 Subject: [PATCH 2/6] fix: code should compile --- internal/impl/update.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/impl/update.go b/internal/impl/update.go index 895af03fdc0..fbcdfc1dab1 100644 --- a/internal/impl/update.go +++ b/internal/impl/update.go @@ -98,7 +98,7 @@ func (d *Devbox) updateDevboxPackage( return err } - return d.mergeResolvedPackageToLockfile(ctx, pkg, resolved, d.lockfile) + return d.mergeResolvedPackageToLockfile(pkg, resolved, d.lockfile) } func (d *Devbox) mergeResolvedPackageToLockfile( From 32f7f8b31085c0b4dfc65e477619a61b29c7f1f4 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Wed, 4 Oct 2023 13:33:00 -0700 Subject: [PATCH 3/6] lint fix --- internal/impl/update.go | 7 ++----- internal/impl/update_test.go | 9 ++++----- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/internal/impl/update.go b/internal/impl/update.go index fbcdfc1dab1..9679059b7d8 100644 --- a/internal/impl/update.go +++ b/internal/impl/update.go @@ -56,7 +56,7 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { return err } } else { - if err = d.updateDevboxPackage(ctx, pkg); err != nil { + if err = d.updateDevboxPackage(pkg); err != nil { return err } } @@ -89,10 +89,7 @@ func (d *Devbox) inputsToUpdate( return pkgsToUpdate, nil } -func (d *Devbox) updateDevboxPackage( - ctx context.Context, - pkg *devpkg.Package, -) error { +func (d *Devbox) updateDevboxPackage(pkg *devpkg.Package) error { resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw) if err != nil { return err diff --git a/internal/impl/update_test.go b/internal/impl/update_test.go index 8f3efe08db8..e5209e1145b 100644 --- a/internal/impl/update_test.go +++ b/internal/impl/update_test.go @@ -1,7 +1,6 @@ package impl import ( - "context" "testing" "github.com/stretchr/testify/require" @@ -25,7 +24,7 @@ func TestUpdateNewPackageIsAdded(t *testing.T) { Packages: map[string]*lock.Package{}, // empty } - err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile) + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) require.NoError(t, err, "update failed") require.Contains(t, lockfile.Packages, raw) @@ -57,7 +56,7 @@ func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) { }, } - err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile) + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) require.NoError(t, err, "update failed") require.Contains(t, lockfile.Packages, raw) @@ -100,7 +99,7 @@ func TestUpdateNewSysInfoIsAdded(t *testing.T) { }, } - err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile) + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) require.NoError(t, err, "update failed") require.Contains(t, lockfile.Packages, raw) @@ -146,7 +145,7 @@ func TestUpdateOtherSysInfoIsReplaced(t *testing.T) { }, } - err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile) + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) require.NoError(t, err, "update failed") require.Contains(t, lockfile.Packages, raw) From d5748d72f503431b5cabd571160971b081d74f00 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Thu, 5 Oct 2023 13:55:44 -0700 Subject: [PATCH 4/6] simplify removeExtraItemsFromProfile --- internal/impl/packages.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/impl/packages.go b/internal/impl/packages.go index a2302632d7e..4c150033044 100644 --- a/internal/impl/packages.go +++ b/internal/impl/packages.go @@ -358,15 +358,18 @@ func (d *Devbox) removeExtraItemsFromProfile( extras := []*nixprofile.NixProfileListItem{} // Note: because devpkg.Package uses memoization when normalizing attribute paths (slow operation), // and since we're reusing the Package objects, this O(n*m) loop becomes O(n+m) wrt the slow operation. -outer: for _, item := range profileItems { + found := false for _, pkg := range packages { if item.Matches(pkg, d.lockfile) { itemsToKeep = append(itemsToKeep, item) - continue outer + found = true + break } } - extras = append(extras, item) + if !found { + extras = append(extras, item) + } } // Remove by index to avoid comparing nix.ProfileListItem <> nix.Inputs again. if err := nixprofile.ProfileRemoveItems(profileDir, extras); err != nil { From bef145cca9d21af012900caf44f524c58bf725bf Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 5 Oct 2023 20:45:53 -0700 Subject: [PATCH 5/6] [global] global shellenv should not recompute env --- devbox.go | 2 +- internal/boxcli/global.go | 13 +++++++++- internal/boxcli/root.go | 3 ++- internal/boxcli/shell.go | 2 +- internal/boxcli/shellenv.go | 15 ++++++++--- internal/impl/devbox.go | 41 +++++++++++++++++------------- internal/impl/devopt/devboxopts.go | 5 ++++ 7 files changed, 56 insertions(+), 25 deletions(-) diff --git a/devbox.go b/devbox.go index cbdc8c1fd27..fc911dc1a0c 100644 --- a/devbox.go +++ b/devbox.go @@ -23,7 +23,7 @@ type Devbox interface { Install(ctx context.Context) error IsEnvEnabled() bool ListScripts() []string - NixEnv(ctx context.Context, includeHooks bool) (string, error) + NixEnv(ctx context.Context, opts devopt.NixEnvOpts) (string, error) PackageNames() []string ProjectDir() string Pull(ctx context.Context, opts devopt.PullboxOpts) error diff --git a/internal/boxcli/global.go b/internal/boxcli/global.go index 17ace55e036..3cfce8f75cd 100644 --- a/internal/boxcli/global.go +++ b/internal/boxcli/global.go @@ -14,7 +14,12 @@ import ( "go.jetpack.io/devbox/internal/ux" ) +type globalShellEnvCmdFlags struct { + recompute bool +} + func globalCmd() *cobra.Command { + globalShellEnvCmdFlags := globalShellEnvCmdFlags{} globalCmd := &cobra.Command{} persistentPreRunE := setGlobalConfigForDelegatedCommands(globalCmd) *globalCmd = cobra.Command{ @@ -28,6 +33,12 @@ func globalCmd() *cobra.Command { PersistentPostRunE: ensureGlobalEnvEnabled, } + shellEnv := shellEnvCmd(&globalShellEnvCmdFlags.recompute) + shellEnv.Flags().BoolVarP( + &globalShellEnvCmdFlags.recompute, "recompute", "r", false, + "Recompute environment if needed", + ) + addCommandAndHideConfigFlag(globalCmd, addCmd()) addCommandAndHideConfigFlag(globalCmd, installCmd()) addCommandAndHideConfigFlag(globalCmd, pathCmd()) @@ -36,7 +47,7 @@ func globalCmd() *cobra.Command { addCommandAndHideConfigFlag(globalCmd, removeCmd()) addCommandAndHideConfigFlag(globalCmd, runCmd()) addCommandAndHideConfigFlag(globalCmd, servicesCmd(persistentPreRunE)) - addCommandAndHideConfigFlag(globalCmd, shellEnvCmd()) + addCommandAndHideConfigFlag(globalCmd, shellEnv) addCommandAndHideConfigFlag(globalCmd, updateCmd()) // Create list for non-global? Mike: I want it :) diff --git a/internal/boxcli/root.go b/internal/boxcli/root.go index 560da3dbc95..422ee083b8e 100644 --- a/internal/boxcli/root.go +++ b/internal/boxcli/root.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/samber/lo" "github.com/spf13/cobra" "go.jetpack.io/devbox/internal/boxcli/featureflag" @@ -72,7 +73,7 @@ func RootCmd() *cobra.Command { command.AddCommand(servicesCmd()) command.AddCommand(setupCmd()) command.AddCommand(shellCmd()) - command.AddCommand(shellEnvCmd()) + command.AddCommand(shellEnvCmd(lo.ToPtr(false) /*dontRecomputeEnv*/)) command.AddCommand(updateCmd()) command.AddCommand(versionCmd()) // Preview commands diff --git a/internal/boxcli/shell.go b/internal/boxcli/shell.go index 4cb5cff95f5..43031f571ce 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(), devopt.NixEnvOpts{}) if err != nil { return err } diff --git a/internal/boxcli/shellenv.go b/internal/boxcli/shellenv.go index 2e6fbbd5e47..fa35e1b8f78 100644 --- a/internal/boxcli/shellenv.go +++ b/internal/boxcli/shellenv.go @@ -22,7 +22,7 @@ type shellEnvCmdFlags struct { preservePathStack bool } -func shellEnvCmd() *cobra.Command { +func shellEnvCmd(recomputeEnvIfNeeded *bool) *cobra.Command { flags := shellEnvCmdFlags{} command := &cobra.Command{ Use: "shellenv", @@ -30,7 +30,7 @@ func shellEnvCmd() *cobra.Command { Args: cobra.ExactArgs(0), PreRunE: ensureNixInstalled, RunE: func(cmd *cobra.Command, args []string) error { - s, err := shellEnvFunc(cmd, flags) + s, err := shellEnvFunc(cmd, flags, *recomputeEnvIfNeeded) if err != nil { return err } @@ -63,7 +63,11 @@ func shellEnvCmd() *cobra.Command { return command } -func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { +func shellEnvFunc( + cmd *cobra.Command, + flags shellEnvCmdFlags, + recomputeEnvIfNeeded bool, +) (string, error) { env, err := flags.Env(flags.config.path) if err != nil { return "", err @@ -85,7 +89,10 @@ func shellEnvFunc(cmd *cobra.Command, flags shellEnvCmdFlags) (string, error) { } } - envStr, err := box.NixEnv(cmd.Context(), flags.runInitHook) + envStr, err := box.NixEnv(cmd.Context(), devopt.NixEnvOpts{ + DontRecomputeEnvironment: !recomputeEnvIfNeeded, + RunHooks: flags.runInitHook, + }) if err != nil { return "", err } diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 45e94101bb2..f9a9e68cc91 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -167,9 +167,6 @@ func (d *Devbox) Shell(ctx context.Context) error { ctx, task := trace.NewTask(ctx, "devboxShell") defer task.End() - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { - return err - } fmt.Fprintln(d.stderr, "Starting a devbox shell...") profileDir, err := d.profilePath() @@ -209,10 +206,6 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string ctx, task := trace.NewTask(ctx, "devboxRun") defer task.End() - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { - return err - } - if err := shellgen.WriteScriptsToFiles(d); err != nil { return err } @@ -273,22 +266,35 @@ 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, opts devopt.NixEnvOpts) (string, error) { ctx, task := trace.NewTask(ctx, "devboxNixEnv") defer task.End() - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { - return "", err + var envs map[string]string + var err error + + if opts.DontRecomputeEnvironment { + upToDate, _ := d.lockfile.IsUpToDateAndInstalled() + if !upToDate { + ux.Finfo( + d.stderr, + "Your devbox environment may be out of date. Please run \n\n"+ + "eval \"$(devbox global shellenv -r)\"\n\n", + ) + } + + envs, err = d.computeNixEnv(ctx, true /*usePrintDevEnvCache*/) + } else { + envs, err = d.nixEnv(ctx) } - envs, err := d.nixEnv(ctx) if err != nil { return "", err } envStr := exportify(envs) - if includeHooks { + if opts.RunHooks { hooksStr := ". " + shellgen.ScriptPath(d.ProjectDir(), shellgen.HooksFilename) envStr = fmt.Sprintf("%s\n%s;\n", envStr, hooksStr) } @@ -299,11 +305,6 @@ func (d *Devbox) NixEnv(ctx context.Context, includeHooks bool) (string, error) 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 - // and excluding "export " prefix in "export key=value" format - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { - return nil, err - } envs, err := d.nixEnv(ctx) if err != nil { @@ -913,6 +914,12 @@ func (d *Devbox) nixEnv(ctx context.Context) (map[string]string, error) { usePrintDevEnvCache = true } + if !usePrintDevEnvCache { + if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { + return nil, err + } + } + return d.computeNixEnv(ctx, usePrintDevEnvCache) } diff --git a/internal/impl/devopt/devboxopts.go b/internal/impl/devopt/devboxopts.go index 438f2af9e89..bdd916b4e9e 100644 --- a/internal/impl/devopt/devboxopts.go +++ b/internal/impl/devopt/devboxopts.go @@ -43,3 +43,8 @@ type UpdateOpts struct { Pkgs []string IgnoreMissingPackages bool } + +type NixEnvOpts struct { + DontRecomputeEnvironment bool + RunHooks bool +} From 57d3cd9e5bb56d9196d67df19f3f123a4d72539c Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Sun, 8 Oct 2023 16:43:52 -0700 Subject: [PATCH 6/6] Requested changes --- internal/boxcli/global.go | 4 +-- internal/boxcli/root.go | 3 +- internal/impl/devbox.go | 58 +++++++++++++++++++-------------------- 3 files changed, 33 insertions(+), 32 deletions(-) diff --git a/internal/boxcli/global.go b/internal/boxcli/global.go index 3cfce8f75cd..3aeabffc0a0 100644 --- a/internal/boxcli/global.go +++ b/internal/boxcli/global.go @@ -34,8 +34,8 @@ func globalCmd() *cobra.Command { } shellEnv := shellEnvCmd(&globalShellEnvCmdFlags.recompute) - shellEnv.Flags().BoolVarP( - &globalShellEnvCmdFlags.recompute, "recompute", "r", false, + shellEnv.Flags().BoolVar( + &globalShellEnvCmdFlags.recompute, "recompute", false, "Recompute environment if needed", ) diff --git a/internal/boxcli/root.go b/internal/boxcli/root.go index 422ee083b8e..e83f805acfb 100644 --- a/internal/boxcli/root.go +++ b/internal/boxcli/root.go @@ -73,7 +73,8 @@ func RootCmd() *cobra.Command { command.AddCommand(servicesCmd()) command.AddCommand(setupCmd()) command.AddCommand(shellCmd()) - command.AddCommand(shellEnvCmd(lo.ToPtr(false) /*dontRecomputeEnv*/)) + // False to avoid recomputing the env in global shellenv: + command.AddCommand(shellEnvCmd(lo.ToPtr(false))) command.AddCommand(updateCmd()) command.AddCommand(versionCmd()) // Preview commands diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 0c878e75724..555be208646 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -168,18 +168,22 @@ func (d *Devbox) Shell(ctx context.Context) error { ctx, task := trace.NewTask(ctx, "devboxShell") defer task.End() - fmt.Fprintln(d.stderr, "Starting a devbox shell...") - profileDir, err := d.profilePath() if err != nil { return err } - envs, err := d.nixEnv(ctx) + envs, err := d.ensurePackagesAreInstalledAndComputeEnv(ctx) if err != nil { return err } + + fmt.Fprintln(d.stderr, "Starting a devbox shell...") + // Used to determine whether we're inside a shell (e.g. to prevent shell inception) + // TODO: This is likely obsolete but we need to decide what happens when + // the user does shell-ception. One option is to leave the current shell and + // join a new one (that way they are not in nested shells.) envs[envir.DevboxShellEnabled] = "1" if err = createDevboxSymlink(d); err != nil { @@ -211,7 +215,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string return err } - env, err := d.nixEnv(ctx) + env, err := d.ensurePackagesAreInstalledAndComputeEnv(ctx) if err != nil { return err } @@ -277,16 +281,20 @@ func (d *Devbox) NixEnv(ctx context.Context, opts devopt.NixEnvOpts) (string, er if opts.DontRecomputeEnvironment { upToDate, _ := d.lockfile.IsUpToDateAndInstalled() if !upToDate { + cmd := `eval "$(devbox global shellenv --recompute)"` + if strings.HasSuffix(os.Getenv("SHELL"), "fish") { + cmd = `devbox global shellenv --recompute | source` + } ux.Finfo( d.stderr, - "Your devbox environment may be out of date. Please run \n\n"+ - "eval \"$(devbox global shellenv -r)\"\n\n", + "Your devbox environment may be out of date. Please run \n\n%s\n\n", + cmd, ) } envs, err = d.computeNixEnv(ctx, true /*usePrintDevEnvCache*/) } else { - envs, err = d.nixEnv(ctx) + envs, err = d.ensurePackagesAreInstalledAndComputeEnv(ctx) } if err != nil { @@ -306,8 +314,8 @@ func (d *Devbox) NixEnv(ctx context.Context, opts devopt.NixEnvOpts) (string, er func (d *Devbox) EnvVars(ctx context.Context) ([]string, error) { ctx, task := trace.NewTask(ctx, "devboxEnvVars") defer task.End() - - envs, err := d.nixEnv(ctx) + // this only returns env variables for the shell environment excluding hooks + envs, err := d.ensurePackagesAreInstalledAndComputeEnv(ctx) if err != nil { return nil, err } @@ -904,31 +912,23 @@ func (d *Devbox) computeNixEnv(ctx context.Context, usePrintDevEnvCache bool) (m return env, d.addHashToEnv(env) } -// nixEnv is a wrapper around computeNixEnv that returns a cached result if -// it has previously computed and the local lock file is up to date. -// 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) { +// ensurePackagesAreInstalledAndComputeEnv does what it says :P +func (d *Devbox) ensurePackagesAreInstalledAndComputeEnv( + ctx context.Context, +) (map[string]string, error) { defer debug.FunctionTimer().End() - usePrintDevEnvCache := false - - // If lockfile is up-to-date, we can use the print-dev-env cache. - upToDate, err := d.lockfile.IsUpToDateAndInstalled() - if err != nil { + // When ensurePackagesAreInstalled is called with ensure=true, it always + // returns early if the lockfile is up to date. So we don't need to check here + if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { return nil, err } - if upToDate { - usePrintDevEnvCache = true - } - - if !usePrintDevEnvCache { - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { - return nil, err - } - } - return d.computeNixEnv(ctx, usePrintDevEnvCache) + // Since ensurePackagesAreInstalled calls computeNixEnv when not up do date, + // 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.computeNixEnv(ctx, true) } func (d *Devbox) nixPrintDevEnvCachePath() string {