Remove pkgs from nix profile that are no longer on devbox.json (jetif…

…y-com#1181) ## Summary The goal is simple: if a package no longer exists in `devbox.json`, then it should be removed from the nix profile. The implementation is not as straightforward, because comparing packages can be very slow. Here we: * Add memoization to `nix.Input` so that it remembers the normalized package attribute path if it ever had to resolve it. * Compares list of of n packages in devbox.json against m pkgs in the profile, resulting in n+m calls, but _only_ if n != m, which is usually not the case in the happy path. * Removes pkgs from the profile by passing in the indeces. This is so that we don't have to compare again whether an Input.Raw exists in the profile. Notes/questions for future improvements: * We're still creating a bunch of `nix.Input`s in a bunch of places, and we end up making more calls to get the normalized attribute path when comparing packages. We should add a global cache or something to avoid duplicate calls. * Apart from avoiding _duplicate_ calls, I think there's ways to avoid the calls altogether: 1. Could we change `devbox.lock` to already contain the normalized attribute paths in its `resolved` field? That way we can probably skip many of the normalization calls. 2. When reading an item from the nix profile, can't we assume that the resolvedReference already has the normalized attribute path? Fixes jetify-com#1138 ## How was it tested? Added a bunch of packages with `devbox add`. Removed them manually from `devbox.json` and called a devbox command. Added fine logging to inspect what was going on and ensure not too many extra expensive calls are being added. Example output below: ``` > nix profile list --profile .devbox/nix/profile/default 0 github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.go github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.go /nix/store/ihz3bv2mzrwxrjzq0i2ysin7kj4i61bv-go-1.20.3 1 github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#legacyPackages.x86_64-darwin.actionlint github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#legacyPackages.x86_64-darwin.actionlint /nix/store/68valrl03a40yzf3qcppfyq468cbc18k-actionlint-1.6.23 2 github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.golangci-lint github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.golangci-lint /nix/store/sc143w85a2sb6xxg6lp073az6rraa1s8-golangci-lint-1.52.2 3 github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.cowsay github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.cowsay /nix/store/c2a8wbqcss1y8yhnx5n1p3awn8cxw0pv-cowsay-3.7.0 /nix/store/v9dccca6yrg53j6zxa9wi46h5n5vxsbl-cowsay-3.7.0-man > cat devbox.json { "packages": [ "go@1.20", "actionlint@1.6.23", "golangci-lint@1.52.2" ], ... > db run -- cowsay 'hi' # after cowsay was manually removed Ensuring packages are installed. checking profile input (unresolved): github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.cowsay against: go@1.20 (resolved=github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go) !! searched: legacyPackages.x86_64-darwin.cowsay !! searched: legacyPackages.x86_64-darwin.go against: actionlint@1.6.23 (resolved=github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#actionlint) against: golangci-lint@1.52.2 (resolved=github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#golangci-lint) memoized: legacyPackages.x86_64-darwin.cowsay !! searched: legacyPackages.x86_64-darwin.golangci-lint checking profile input (unresolved): github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.go against: go@1.20 (resolved=github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go) !! searched: legacyPackages.x86_64-darwin.go memoized: legacyPackages.x86_64-darwin.go checking profile input (unresolved): github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#legacyPackages.x86_64-darwin.actionlint against: go@1.20 (resolved=github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go) against: actionlint@1.6.23 (resolved=github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#actionlint) !! searched: legacyPackages.x86_64-darwin.actionlint !! searched: legacyPackages.x86_64-darwin.actionlint checking profile input (unresolved): github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.golangci-lint against: go@1.20 (resolved=github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go) !! searched: legacyPackages.x86_64-darwin.golangci-lint memoized: legacyPackages.x86_64-darwin.go against: actionlint@1.6.23 (resolved=github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#actionlint) against: golangci-lint@1.52.2 (resolved=github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#golangci-lint) memoized: legacyPackages.x86_64-darwin.golangci-lint memoized: legacyPackages.x86_64-darwin.golangci-lint found 1 extras to remove .cmd.sh: line 3: cowsay: command not found > nix profile list --profile .devbox/nix/profile/default 0 github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.go github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.go /nix/store/ihz3bv2mzrwxrjzq0i2ysin7kj4i61bv-go-1.20.3 1 github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#legacyPackages.x86_64-darwin.actionlint github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#legacyPackages.x86_64-darwin.actionlint /nix/store/68valrl03a40yzf3qcppfyq468cbc18k-actionlint-1.6.23 2 github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.golangci-lint github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.golangci-lint /nix/store/sc143w85a2sb6xxg6lp073az6rraa1s8-golangci-lint-1.52.2 ```
zack-sys · Jun 20, 2023 · 1e60344 · 1e60344
1 parent 64313c1
commit 1e60344
Show file tree

Hide file tree

Showing 6 changed files with 155 additions and 22 deletions.
diff --git a/internal/impl/packages.go b/internal/impl/packages.go
@@ -72,10 +72,7 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {
 	}
 
 	if err := d.ensurePackagesAreInstalled(ctx, install); err != nil {
-		return usererr.WithUserMessage(
-			err,
-			"There was an error installing nix packages",
-		)
+		return usererr.WithUserMessage(err, "There was an error installing nix packages")
 	}
 
 	if err := d.saveCfg(); err != nil {
@@ -186,7 +183,7 @@ func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMod
 		fmt.Fprintln(d.writer, "Ensuring packages are installed.")
 	}
 
-	if err := d.addPackagesToProfile(ctx, mode); err != nil {
+	if err := d.syncPackagesToProfile(ctx, mode); err != nil {
 		return err
 	}
 
@@ -219,6 +216,18 @@ func (d *Devbox) profilePath() (string, error) {
 	return absPath, errors.WithStack(os.MkdirAll(filepath.Dir(absPath), 0755))
 }
 
+// syncPackagesToProfile ensures that all packages in devbox.json exist in the nix profile,
+// and no more.
+func (d *Devbox) syncPackagesToProfile(ctx context.Context, mode installMode) error {
+	// 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 {
+		return err
+	}
+
+	return d.tidyProfile(ctx)
+}
+
 // 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.
@@ -320,6 +329,24 @@ func (d *Devbox) removePackagesFromProfile(ctx context.Context, pkgs []string) e
 	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 nix.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
@@ -355,6 +382,49 @@ func (d *Devbox) pendingPackagesForInstallation(ctx context.Context) ([]string,
 	return pending, nil
 }
 
+// extraPkgsInProfile 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) ([]*nix.NixProfileListItem, error) {
+	defer trace.StartRegion(ctx, "extraPackagesInProfile").End()
+
+	profileDir, err := d.profilePath()
+	if err != nil {
+		return nil, err
+	}
+
+	profileItems, err := nix.ProfileListItems(d.writer, profileDir)
+	if err != nil {
+		return nil, err
+	}
+	devboxInputs := d.packagesAsInputs()
+
+	if len(devboxInputs) == 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 := []*nix.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 {
+		profileInput := nix.InputFromProfileItem(item, d.lockfile)
+		for _, devboxInput := range devboxInputs {
+			if profileInput.Equals(devboxInput) {
+				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/nix/input.go b/internal/nix/input.go
@@ -24,6 +24,8 @@ type Input struct {
 	url.URL
 	lockfile lock.Locker
 	Raw      string
+
+	normalizedPackageAttributePathCache string // memoized value from normalizedPackageAttributePath()
 }
 
 func InputsFromStrings(names []string, l lock.Locker) []*Input {
@@ -45,7 +47,11 @@ func InputFromString(raw string, locker lock.Locker) *Input {
 		}
 		u, _ = url.Parse(normalizedURL)
 	}
-	return &Input{*u, locker, raw}
+	return &Input{*u, locker, raw, ""}
+}
+
+func InputFromProfileItem(item *NixProfileListItem, locker lock.Locker) *Input {
+	return InputFromString(item.unlockedReference, locker)
 }
 
 func (i *Input) IsLocal() bool {
@@ -170,17 +176,33 @@ func (i *Input) FullPackageAttributePath() (string, error) {
 
 // NormalizedPackageAttributePath returns an attribute path normalized by nix
 // search. This is useful for comparing different attribute paths that may
-// point to the same package. Note, it's an expensive call.
+// point to the same package. Note, it may be an expensive call.
 func (i *Input) NormalizedPackageAttributePath() (string, error) {
+	if i.normalizedPackageAttributePathCache != "" {
+		return i.normalizedPackageAttributePathCache, nil
+	}
+	path, err := i.normalizePackageAttributePath()
+	if err != nil {
+		return path, err
+	}
+	i.normalizedPackageAttributePathCache = path
+	return i.normalizedPackageAttributePathCache, nil
+}
+
+// normalizePackageAttributePath calls nix search to find the normalized attribute
+// path. It is an expensive call (~100ms).
+func (i *Input) normalizePackageAttributePath() (string, error) {
 	var query string
-	if i.isVersioned() {
-		entry, err := i.lockfile.Resolve(i.Raw)
-		if err != nil {
-			return "", err
+	if i.IsDevboxPackage() {
+		if i.isVersioned() {
+			entry, err := i.lockfile.Resolve(i.Raw)
+			if err != nil {
+				return "", err
+			}
+			query = entry.Resolved
+		} else {
+			query = i.lockfile.LegacyNixpkgsPath(i.String())
 		}
-		query = entry.Resolved
-	} else if i.IsDevboxPackage() {
-		query = i.lockfile.LegacyNixpkgsPath(i.String())
 	} else {
 		query = i.String()
 	}
@@ -262,7 +284,7 @@ func (i *Input) ValidateExists() (bool, error) {
 	return info != "", err
 }
 
-func (i *Input) equals(other *Input) bool {
+func (i *Input) Equals(other *Input) bool {
 	if i.String() == other.String() {
 		return true
 	}

diff --git a/internal/nix/profile.go b/internal/nix/profile.go
@@ -25,15 +25,12 @@ import (
 
 const DefaultPriority = 5
 
-// ProfileListItems returns a list of the installed packages
+// ProfileListItems returns a list of the installed packages.
 func ProfileListItems(
 	writer io.Writer,
 	profileDir string,
 ) (map[string]*NixProfileListItem, error) {
-	cmd := exec.Command(
-		"nix", "profile", "list",
-		"--profile", profileDir,
-	)
+	cmd := exec.Command("nix", "profile", "list", "--profile", profileDir)
 	cmd.Args = append(cmd.Args, ExperimentalFlags()...)
 
 	// We set stderr to a different output than stdout
@@ -107,9 +104,9 @@ func ProfileListIndex(args *ProfileListIndexArgs) (int, error) {
 	}
 
 	for _, item := range list {
-		existing := InputFromString(item.unlockedReference, args.Lockfile)
+		existing := InputFromProfileItem(item, args.Lockfile)
 
-		if args.Input.equals(existing) {
+		if args.Input.Equals(existing) {
 			return item.index, nil
 		}
 	}
@@ -264,6 +261,32 @@ func ProfileInstall(args *ProfileInstallArgs) error {
 	return nil
 }
 
+// ProfileRemoveItems removes the items from the profile, in a single call, using their indexes.
+// It is up to the caller to ensure that the underlying profile has not changed since the items
+// were queried.
+func ProfileRemoveItems(profilePath string, items []*NixProfileListItem) error {
+	if items == nil {
+		return nil
+	}
+
+	indexes := []string{}
+	for _, item := range items {
+		indexes = append(indexes, strconv.Itoa(item.index))
+	}
+	cmd := exec.Command("nix", append([]string{"profile", "remove",
+		"--profile", profilePath,
+		"--impure"}, // for NIXPKGS_ALLOW_UNFREE
+		indexes...)...,
+	)
+	cmd.Env = allowUnfreeEnv()
+	cmd.Args = append(cmd.Args, ExperimentalFlags()...)
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		return redact.Errorf("error running \"nix profile remove\": %s: %w", out, err)
+	}
+	return nil
+}
+
 func ProfileRemove(profilePath, pkg string, lock lock.Locker) error {
 	info := PkgInfo(pkg, lock)
 	if info == nil {

diff --git a/testscripts/rm/add-rm.text.txt → testscripts/rm/add-rm.test.txt b/testscripts/rm/add-rm.text.txt → testscripts/rm/add-rm.test.txt
diff --git a/testscripts/rm/manual.test.txt b/testscripts/rm/manual.test.txt
@@ -0,0 +1,18 @@
+exec devbox run hello
+stdout 'Hello, world!'
+
+# Simulate deleting the packages manually.
+cp empty.json devbox.json
+
+! exec devbox run hello
+! stdout 'Hello, world!'
+
+-- devbox.json --
+{
+  "packages": ["hello"]
+}
+
+-- empty.json --
+{
+  "packages": []
+}
diff --git a/testscripts/rm/multi.text.txt → testscripts/rm/multi.test.txt b/testscripts/rm/multi.text.txt → testscripts/rm/multi.test.txt