From 265f10047fdcfed31facaa8ec4c52e23c03eec3a Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 6 Oct 2023 16:31:37 -0700 Subject: [PATCH 1/3] [perf] skip forcing printDevEnv when add/rm/update outside shellenv --- internal/impl/devbox.go | 16 ++++++++-------- internal/impl/envvars.go | 4 ++-- internal/impl/packages.go | 35 ++++++++++++++++++++++------------- internal/impl/update.go | 2 +- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 7d0089d8e14..154a38952a9 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -259,7 +259,7 @@ func (d *Devbox) Install(ctx context.Context) error { ctx, task := trace.NewTask(ctx, "devboxInstall") defer task.End() - return d.ensurePackagesAreInstalled(ctx, ensure) + return d.ensureDevboxEnvIsUpToDate(ctx, ensure) } func (d *Devbox) ListScripts() []string { @@ -477,7 +477,7 @@ func (d *Devbox) GenerateEnvrcFile(ctx context.Context, force bool, envFlags dev } // generate all shell files to ensure we can refer to them in the .envrc script - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { + if err := d.ensureDevboxEnvIsUpToDate(ctx, ensure); err != nil { return err } @@ -931,9 +931,9 @@ func (d *Devbox) ensurePackagesAreInstalledAndComputeEnv( ) (map[string]string, error) { defer debug.FunctionTimer().End() - // When ensurePackagesAreInstalled is called with ensure=true, it always + // When ensureDevboxEnvIsUpToDate 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 { + if err := d.ensureDevboxEnvIsUpToDate(ctx, ensure); err != nil { return nil, err } @@ -1172,10 +1172,10 @@ func (d *Devbox) parseEnvAndExcludeSpecialCases(currentEnv []string) (map[string if ignoreCurrentEnvVar[key] { continue } - // handling special cases to for pure shell - // Passing HOME for pure shell to leak through otherwise devbox binary won't work - // We also include PATH to find the nix installation. It is cleaned for pure mode below - // TERM leaking through is to enable colored text in the pure shell + // handling special cases for pure shell + // - 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" { env[key] = val } diff --git a/internal/impl/envvars.go b/internal/impl/envvars.go index 488e9ad0508..2013a05ef1c 100644 --- a/internal/impl/envvars.go +++ b/internal/impl/envvars.go @@ -69,8 +69,8 @@ func markEnvsAsSetByDevbox(envs ...map[string]string) { } } -// IsEnvEnabled checks if the devbox environment is enabled. We use the ogPathKey -// as a proxy for this. This allows us to differentiate between global and +// IsEnvEnabled checks if the devbox environment is enabled. +// This allows us to differentiate between global and // individual project shells. func (d *Devbox) IsEnvEnabled() bool { fakeEnv := map[string]string{} diff --git a/internal/impl/packages.go b/internal/impl/packages.go index 9db082fea7d..0bc5bc1e422 100644 --- a/internal/impl/packages.go +++ b/internal/impl/packages.go @@ -108,7 +108,7 @@ func (d *Devbox) Add(ctx context.Context, platforms, excludePlatforms []string, } } - // Resolving here ensures we allow insecure before running ensurePackagesAreInstalled + // Resolving here ensures we allow insecure before running ensureDevboxEnvIsUpToDate // which will call print-dev-env. Resolving does not save the lockfile, we // save at the end when everything has succeeded. if d.allowInsecureAdds { @@ -126,7 +126,7 @@ func (d *Devbox) Add(ctx context.Context, platforms, excludePlatforms []string, } } - if err := d.ensurePackagesAreInstalled(ctx, install); err != nil { + if err := d.ensureDevboxEnvIsUpToDate(ctx, install); err != nil { return usererr.WithUserMessage(err, "There was an error installing nix packages") } @@ -190,28 +190,35 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { } // this will clean up the now-extra package from nix profile and the lockfile - if err := d.ensurePackagesAreInstalled(ctx, uninstall); err != nil { + if err := d.ensureDevboxEnvIsUpToDate(ctx, uninstall); err != nil { return err } return d.saveCfg() } -// installMode is an enum for helping with ensurePackagesAreInstalled implementation +// installMode is an enum for helping with ensureDevboxEnvIsUpToDate implementation type installMode string const ( install installMode = "install" uninstall installMode = "uninstall" - ensure installMode = "ensure" + // update is both install new package version and uninstall old package version + update installMode = "update" + ensure installMode = "ensure" ) -// 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 { +// ensureDevboxEnvIsUpToDate ensures: +// 1. Packages are installed, in nix-profile or runx. +// Extraneous packages are removed (references purged, not uninstalled). +// 2. Files for devbox shellenv are generated +// 3. Env-vars for shellenv are computed +// 4. Lockfile is synced +// +// The `mode` is used for: +// 1. Skipping certain operations that may not apply. +// 2. User messaging to explain what operations are happening, because this function may take time to execute. +func (d *Devbox) ensureDevboxEnvIsUpToDate(ctx context.Context, mode installMode) error { defer trace.StartRegion(ctx, "ensurePackages").End() defer debug.FunctionTimer().End() @@ -248,8 +255,10 @@ func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMod return err } - // Force print-dev-env cache to be recomputed. - nixEnv, err := d.computeNixEnv(ctx, false /*use cache*/) + // Use the printDevEnvCache if we are adding or removing or updating any package, + // AND we are not in the shellenv-enabled environment of the current devbox-project. + usePrintDevEnvCache := mode != ensure && !d.IsEnvEnabled() + nixEnv, err := d.computeNixEnv(ctx, usePrintDevEnvCache) if err != nil { return err } diff --git a/internal/impl/update.go b/internal/impl/update.go index 9679059b7d8..0dea6adcb35 100644 --- a/internal/impl/update.go +++ b/internal/impl/update.go @@ -62,7 +62,7 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { } } - if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { + if err := d.ensureDevboxEnvIsUpToDate(ctx, update); err != nil { return err } From f5ab63d559062b0305dee5acb3888db1457debb3 Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Fri, 6 Oct 2023 16:59:17 -0700 Subject: [PATCH 2/3] lint fix --- internal/impl/shell_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/impl/shell_test.go b/internal/impl/shell_test.go index 98255ef2b79..6813f4a443a 100644 --- a/internal/impl/shell_test.go +++ b/internal/impl/shell_test.go @@ -17,8 +17,8 @@ import ( "go.jetpack.io/devbox/internal/shellgen" ) -// update overwrites golden files with the new test results. -var update = flag.Bool("update", false, "update the golden files with the test results") +// updateFlag overwrites golden files with the new test results. +var updateFlag = flag.Bool("update", false, "update the golden files with the test results") func TestWriteDevboxShellrc(t *testing.T) { testdirs, err := filepath.Glob("testdata/shellrc/*") @@ -83,7 +83,7 @@ func testWriteDevboxShellrc(t *testing.T, testdirs []string) { // Overwrite the golden file if the -update flag was // set, and then proceed normally. The test should // always pass in this case. - if *update { + if *updateFlag { err = os.WriteFile(test.goldShellrcPath, gotShellrc, 0o666) if err != nil { t.Error("Error updating golden files:", err) From 609f2bc33e64c8ee9e18eb0f075514c11c786ece Mon Sep 17 00:00:00 2001 From: Savil Srivastava <676452+savil@users.noreply.github.com> Date: Tue, 10 Oct 2023 10:59:15 -0700 Subject: [PATCH 3/3] rename back to ensurePackagesAreInstalled --- internal/impl/devbox.go | 8 ++++---- internal/impl/packages.go | 13 +++++++------ internal/impl/update.go | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go index 154a38952a9..a4edbfa3293 100644 --- a/internal/impl/devbox.go +++ b/internal/impl/devbox.go @@ -259,7 +259,7 @@ func (d *Devbox) Install(ctx context.Context) error { ctx, task := trace.NewTask(ctx, "devboxInstall") defer task.End() - return d.ensureDevboxEnvIsUpToDate(ctx, ensure) + return d.ensurePackagesAreInstalled(ctx, ensure) } func (d *Devbox) ListScripts() []string { @@ -477,7 +477,7 @@ func (d *Devbox) GenerateEnvrcFile(ctx context.Context, force bool, envFlags dev } // generate all shell files to ensure we can refer to them in the .envrc script - if err := d.ensureDevboxEnvIsUpToDate(ctx, ensure); err != nil { + if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { return err } @@ -931,9 +931,9 @@ func (d *Devbox) ensurePackagesAreInstalledAndComputeEnv( ) (map[string]string, error) { defer debug.FunctionTimer().End() - // When ensureDevboxEnvIsUpToDate is called with ensure=true, it always + // 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.ensureDevboxEnvIsUpToDate(ctx, ensure); err != nil { + if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil { return nil, err } diff --git a/internal/impl/packages.go b/internal/impl/packages.go index 0bc5bc1e422..71823271c32 100644 --- a/internal/impl/packages.go +++ b/internal/impl/packages.go @@ -108,7 +108,7 @@ func (d *Devbox) Add(ctx context.Context, platforms, excludePlatforms []string, } } - // Resolving here ensures we allow insecure before running ensureDevboxEnvIsUpToDate + // Resolving here ensures we allow insecure before running ensurePackagesAreInstalled // which will call print-dev-env. Resolving does not save the lockfile, we // save at the end when everything has succeeded. if d.allowInsecureAdds { @@ -126,7 +126,7 @@ func (d *Devbox) Add(ctx context.Context, platforms, excludePlatforms []string, } } - if err := d.ensureDevboxEnvIsUpToDate(ctx, install); err != nil { + if err := d.ensurePackagesAreInstalled(ctx, install); err != nil { return usererr.WithUserMessage(err, "There was an error installing nix packages") } @@ -190,14 +190,14 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { } // this will clean up the now-extra package from nix profile and the lockfile - if err := d.ensureDevboxEnvIsUpToDate(ctx, uninstall); err != nil { + if err := d.ensurePackagesAreInstalled(ctx, uninstall); err != nil { return err } return d.saveCfg() } -// installMode is an enum for helping with ensureDevboxEnvIsUpToDate implementation +// installMode is an enum for helping with ensurePackagesAreInstalled implementation type installMode string const ( @@ -208,7 +208,7 @@ const ( ensure installMode = "ensure" ) -// ensureDevboxEnvIsUpToDate ensures: +// ensurePackagesAreInstalled ensures: // 1. Packages are installed, in nix-profile or runx. // Extraneous packages are removed (references purged, not uninstalled). // 2. Files for devbox shellenv are generated @@ -218,7 +218,8 @@ const ( // The `mode` is used for: // 1. Skipping certain operations that may not apply. // 2. User messaging to explain what operations are happening, because this function may take time to execute. -func (d *Devbox) ensureDevboxEnvIsUpToDate(ctx context.Context, mode installMode) error { +// TODO: Rename method since it does more than just ensure packages are installed. +func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMode) error { defer trace.StartRegion(ctx, "ensurePackages").End() defer debug.FunctionTimer().End() diff --git a/internal/impl/update.go b/internal/impl/update.go index 0dea6adcb35..69064e18f97 100644 --- a/internal/impl/update.go +++ b/internal/impl/update.go @@ -62,7 +62,7 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { } } - if err := d.ensureDevboxEnvIsUpToDate(ctx, update); err != nil { + if err := d.ensurePackagesAreInstalled(ctx, update); err != nil { return err }