Remove pkgs from nix profile that are no longer on devbox.json #1181

ipince · 2023-06-19T21:10:08Z

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.Inputs 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 #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

mikeland73 · 2023-06-20T12:50:52Z

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.

We would need to save normalized names for devbox.json packages and also profile unlocked references.

I would suggest local.lock instead since it's purely a caching mechanism and not data that we care about.

mikeland73

Nice! this LGTM. Just need to test perf edge cases. Especially flakes. You can try:

Add many devbox packages (~20)
Add flakes (local and remote)
Add php Haskel packages (that have plugins that create flakes)
Manually edit devbox.json to remove a few things.
Call something that triggers tidy and time it.

Also, take a look at these optimizations to make sure nothing regressed:
#1050

mikeland73 · 2023-06-20T12:51:39Z

internal/impl/packages.go

-		return usererr.WithUserMessage(
-			err,
-			"There was an error installing nix packages",
-		)


ineffectual change

Indeed, but I included it on purpose, to save on vertical space.

mikeland73 · 2023-06-20T12:52:13Z

internal/impl/packages.go

@@ -219,6 +216,16 @@ func (d *Devbox) profilePath() (string, error) {
 	return absPath, errors.WithStack(os.MkdirAll(filepath.Dir(absPath), 0755))
 }

+func (d *Devbox) syncPackagesToProfile(ctx context.Context, mode installMode) error {


Add docblock

mikeland73 · 2023-06-20T12:55:23Z

internal/impl/packages.go

+	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


I wonder if this is always strictly true (as in does it ever falsly return when in fact packages don't match).

because we always add new package to profile first, I think any change do devbox.json will cause these lengths to missmatch if they are in fact different. So I think it works as expected, but relies on how add packages is implemented.

Yeah it definitely relies on having added all devbox.json packages to the profile immediately beforehand.
I think it's a good compromise for now to avoid the performance hit. But it's better if we can merge the adding/removing into a single pass so we don't have to make these assumptions (and also save some computation time).

mikeland73 · 2023-06-20T12:58:01Z

internal/nix/input.go

@@ -24,6 +24,8 @@ type Input struct {
 	url.URL
 	lockfile lock.Locker
 	Raw      string
+
+	normalizedAttributePath string // memoized value from normalizedPackageAttributePath()


normalizedAttributePathCache so we can rename the method below to normalizedAttributePath

Done. renamed to normalizedPackageAttributePathCache. IT's long af but consistent with the other PackageAttributePath. I think we should drop the "Package" everywhere, but I didn't want to add scope to this PR. We can change that separately.

mikeland73 · 2023-06-20T12:59:38Z

internal/nix/input.go

+	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())


Why did this logic change? Seems like an ineffectual change but more nesting.

Yes, this is a no-op change. I thought it was more clear this way.

It's just that isVersioned implies isDevboxPkg, but that's not immediately clear to a reader. And then the reader might think that the else-if branch applies to devbox packages. But it only really applies to non-versioned devbox packages. So, written this way makes the intention immediately clear without having to jump around the code.

But yeah, it's a bit subjective. I actually had also made the change on line 125 (that condition is always true).

I guess it's just weird/confusing to me to see control flow like:

if A && B { ... } else if A { ... } else { ... }

mikeland73 · 2023-06-20T13:01:13Z

internal/nix/profile.go

+	cmd := exec.Command("nix", "profile", "remove",
+		"--profile", profilePath,
+		"--impure", // for NIXPKGS_ALLOW_UNFREE
+		strings.Join(indexes, " "),


Does this behave OK when multiple indexes? I think we can also remove by unlocked reference but that may be a problem with duplicates

Yes, I tested removing multiple things at the same time. Also tested passing indexes out-of-bounds. The command always succeeds (exit code 0). If any of the indexes passed are invalid, it prints a warning to stderr. Any valid indexes are still removed.

I looked the nix profile remove --help to decide what to use here. The only options are: by position (what we're doing), by attribute path (can use regex), or by nix store path.

Position seemed easiest to implement and hardest to mess up.
Using attribute paths would require us to split the unlockedRef by #, and assume uniqueness (maybe I have two packages with the same attribute path but at different commits for some reason).

Update: I thought we didn't have the nix store path, but we do! But I don't see any benefit to using that instead of the position... hmm maybe a better error message in case of failure?

> nix profile remove --profile .devbox/nix/profile/default /nix/store/asdf warning: '/nix/store/asdf' does not match any packages warning: Use 'nix profile list' to see the current profile. > nix profile remove --profile .devbox/nix/profile/default 4 warning: '4' is not a valid index warning: Use 'nix profile list' to see the current profile.

ipince

Add php Haskel packages (that have plugins that create flakes)

Do you have an example? If not, I'll use haskellPackages.acme-php.

Sidenote: I tried searching for that using devbox search but failed, probably because we only allow single-word searches. Would be nice to allow multiple words in the future.

ipince · 2023-06-20T15:51:50Z

internal/impl/packages.go

-		return usererr.WithUserMessage(
-			err,
-			"There was an error installing nix packages",
-		)


Indeed, but I included it on purpose, to save on vertical space.

ipince · 2023-06-20T15:53:47Z

internal/impl/packages.go

+	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


Yeah it definitely relies on having added all devbox.json packages to the profile immediately beforehand.
I think it's a good compromise for now to avoid the performance hit. But it's better if we can merge the adding/removing into a single pass so we don't have to make these assumptions (and also save some computation time).

ipince · 2023-06-20T17:16:46Z

internal/nix/input.go

+	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())


Yes, this is a no-op change. I thought it was more clear this way.

It's just that isVersioned implies isDevboxPkg, but that's not immediately clear to a reader. And then the reader might think that the else-if branch applies to devbox packages. But it only really applies to non-versioned devbox packages. So, written this way makes the intention immediately clear without having to jump around the code.

But yeah, it's a bit subjective. I actually had also made the change on line 125 (that condition is always true).

ipince · 2023-06-20T17:18:40Z

internal/nix/input.go

+	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())


I guess it's just weird/confusing to me to see control flow like:

if A && B { ... } else if A { ... } else { ... }

ipince · 2023-06-20T17:20:10Z

internal/nix/profile.go

+	cmd := exec.Command("nix", "profile", "remove",
+		"--profile", profilePath,
+		"--impure", // for NIXPKGS_ALLOW_UNFREE
+		strings.Join(indexes, " "),


Yes, I tested removing multiple things at the same time. Also tested passing indexes out-of-bounds. The command always succeeds (exit code 0). If any of the indexes passed are invalid, it prints a warning to stderr. Any valid indexes are still removed.

ipince · 2023-06-20T17:28:32Z

internal/nix/profile.go

+	cmd := exec.Command("nix", "profile", "remove",
+		"--profile", profilePath,
+		"--impure", // for NIXPKGS_ALLOW_UNFREE
+		strings.Join(indexes, " "),


I looked the nix profile remove --help to decide what to use here. The only options are: by position (what we're doing), by attribute path (can use regex), or by nix store path.

Position seemed easiest to implement and hardest to mess up.
Using attribute paths would require us to split the unlockedRef by #, and assume uniqueness (maybe I have two packages with the same attribute path but at different commits for some reason).

Update: I thought we didn't have the nix store path, but we do! But I don't see any benefit to using that instead of the position... hmm maybe a better error message in case of failure?

> nix profile remove --profile .devbox/nix/profile/default /nix/store/asdf warning: '/nix/store/asdf' does not match any packages warning: Use 'nix profile list' to see the current profile. > nix profile remove --profile .devbox/nix/profile/default 4 warning: '4' is not a valid index warning: Use 'nix profile list' to see the current profile.

ipince · 2023-06-20T17:32:07Z

internal/nix/input.go

@@ -24,6 +24,8 @@ type Input struct {
 	url.URL
 	lockfile lock.Locker
 	Raw      string
+
+	normalizedAttributePath string // memoized value from normalizedPackageAttributePath()


ipince · 2023-06-20T17:32:19Z

internal/impl/packages.go

@@ -219,6 +216,16 @@ func (d *Devbox) profilePath() (string, error) {
 	return absPath, errors.WithStack(os.MkdirAll(filepath.Dir(absPath), 0755))
 }

+func (d *Devbox) syncPackagesToProfile(ctx context.Context, mode installMode) error {


ipince · 2023-06-20T19:35:46Z

We would need to save normalized names for devbox.json packages and also profile unlocked references.

Hmm, why?

Anyway, my original suggestion was to save the normalized package, like:

github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#legacyPackages.x86_64-darwin.go

instead of what we do today, which is this:

github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go

But I guess that won't really work, because the normalized attribute paths are platform-dependent. We could save all of them, sure, but at that point it's probably not worth it to save a call to nix search.

--

My other suggestion was that, when we convert a ProfileItem to an Input, we can basically do input.normalizedAttributePath = strings.Spit(item.lockedReference, "#")[-1]. They should always be identical, right?

ipince · 2023-06-20T23:16:25Z

OK I finished testing the performance stuff, and also local and remote flakes.

Performance-wise, the comparisons are fairly quick. It at most took ~4 seconds with doing it with ~20 packages. Since this scenario should not happen very often, I think it's fine to ship.

I did notice that sometimes there's a lot of time spent after the profile syncing, and I'm not sure what is it, but it's unrelated and should be part of our efforts to improve performance overall. I think there's some relatively low-hanging fruit.

Merging after tests pass.

ipince added 2 commits June 19, 2023 16:48

works

9e85a96

clean up

1bb8018

ipince requested a review from mikeland73 June 19, 2023 21:10

Fix lint

ee8f341

ipince requested a review from gcurtis June 19, 2023 21:16

ipince added 2 commits June 19, 2023 17:46

fix tests

6e74929

add script test

cff4848

mikeland73 approved these changes Jun 20, 2023

View reviewed changes

ipince commented Jun 20, 2023

View reviewed changes

savil mentioned this pull request Jun 20, 2023

[Bug]: devbox won't remove packages that aren't declared in devbox.json #1185

Closed

ipince added 2 commits June 20, 2023 19:17

address comments; fix a bug

0822ac0

include files i mistakenly skipped in previous commit

b5fb7cf

ipince merged commit 1e60344 into main Jun 20, 2023

ipince deleted the rodrigo/profile-rm branch June 20, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pkgs from nix profile that are no longer on devbox.json #1181

Remove pkgs from nix profile that are no longer on devbox.json #1181

ipince commented Jun 19, 2023

mikeland73 commented Jun 20, 2023

mikeland73 left a comment

mikeland73 Jun 20, 2023

ipince Jun 20, 2023

mikeland73 Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

mikeland73 Jun 20, 2023

ipince Jun 20, 2023

mikeland73 Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

mikeland73 Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

mikeland73 Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince left a comment

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince Jun 20, 2023

ipince commented Jun 20, 2023 •

edited

Loading

ipince commented Jun 20, 2023

Remove pkgs from nix profile that are no longer on devbox.json #1181

Remove pkgs from nix profile that are no longer on devbox.json #1181

Conversation

ipince commented Jun 19, 2023

Summary

How was it tested?

mikeland73 commented Jun 20, 2023

mikeland73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipince left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipince commented Jun 20, 2023 • edited Loading

ipince commented Jun 20, 2023

ipince commented Jun 20, 2023 •

edited

Loading