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/4] [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/4] 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/4] 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/4] 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 {