[rm-nixpkgs] Fix profile list bug for packages added by store path #1416

ipince · 2023-08-23T17:48:07Z

Summary

When a package is added by store path, then when we query nix profile list, it'll look like this:

> nix profile list --profile .devbox/nix/profile/default
Index:              0
Store paths:        /nix/store/yjkac3v6c1v129fph0gvy9c7ndnx738b-cowsay-3.04

instead of this:

> nix profile list --profile .devbox/nix/profile/default
Index:              0
Flake attribute:    legacyPackages.x86_64-darwin.hello
Original flake URL: github:NixOS/nixpkgs/3007746b3f5bfcb49e102b517bca891822a41b31
Locked flake URL:   github:NixOS/nixpkgs/3007746b3f5bfcb49e102b517bca891822a41b31
Store paths:        /nix/store/gyh1bri77vlcs1pwxyg4x3z06aql2c6p-hello-2.12.1

Thus, when we read the profile items and try to build a map out of the item's unlockedRef, we were always ending up with a map of size 1.

To fix, I changed the map to a slice, and changed the callsites to handle the slice instead.

I made a few other code organization changes that are no-ops.

How was it tested?

Tested by calling devbox install and inspecting the profile with nix profile list, with and without the rm-nixpkgs feature enabled. I attempted to add unit tests but it's not as easy as I had thought.

ipince · 2023-08-23T17:48:28Z

internal/nix/nixprofile/profile.go

 	// are confident index has not changed.
-	List       map[string]*NixProfileListItem
+	Items      []*NixProfileListItem


this is the main change

ipince · 2023-08-23T17:51:02Z

internal/nix/nixprofile/profile.go

 			if pathInStore == item.nixStorePath {
 				return item.index, nil
 			}
 		}
+		return -1, errors.Wrap(nix.ErrPackageNotFound, args.DevPkg.String())


this is a somewhat subtle change that I thought I'd highlight.

If a package is inCache, then it must have been added by store path. So it follows that we must find a store path match in the items, and if not, then the package is Not Found--that is, there's no need to fall back to the other checks below.

Is this a safe assumption? I think so, because the cache doesn't change much and a package is either in it or not.. right? Please let me know if this isn't as safe an assumption as I think

I think that makes sense.

One scenario I was thinking about is (sequentially):

Existing devbox user has installed a package.

We ship a Devbox update that has Remove Nixpkgs feature enabled.

This code here runs. The Package happens to be in the binary cache, BUT it had previously been installed the "old way" in the nix profile.

I think this code should still work, since the store path is input-addressed and would match here.

I just tested a few different scenarios and verified this is safe. Most importantly, I tested:

User has installed a package in the "old" way. Then the user upgrades devbox and runs a devbox command that triggers the installation path. The package is now going to be installed by store-path. Because the store-paths match, then we do nothing. That is, we keep the "old-style" package installed in the profile, because we found it. This is OK.

Same as above, but the package store path does NOT match. In this case, devbox will end up removing the old package and adding the new package, which is good.

savil

LGTM!

I think we do need a fix in extraPackagesInProfile. See my discussion below...
Can be done in separate PR (or I can do it, if you'd like).

savil · 2023-08-23T22:56:00Z

internal/impl/packages.go

 		index, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{
 			Lockfile:   d.lockfile,
 			Writer:     d.writer,
-			Input:      input,
+			DevPkg:     pkg,


suggestion: make the key Package since we're standardizing on that (mostly).

I say "(mostly)" because there is a devconfig.Package but that is usually consumed by devcfg.Package and not used directly in most of the codebase.

savil · 2023-08-23T22:56:22Z

internal/impl/packages.go

@@ -428,10 +428,10 @@ func (d *Devbox) pendingPackagesForInstallation(ctx context.Context) ([]*devpkg.
 	}
 	for _, pkg := range packages {
 		_, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{
-			List:       list,
+			Items:      items,


savil · 2023-08-23T23:01:40Z

internal/nix/nixprofile/item.go

+// ToPackage constructs a nix.Package using the unlocked reference.
+// TODO: this doesn't handle well the case where item.unlockedReference == "#"
+func (item *NixProfileListItem) ToPackage(locker lock.Locker) *devpkg.Package {
+	return devpkg.PackageFromString(item.unlockedReference, locker)


hmm this could be problematic for Items created via store-paths! 😬

Seems to be used in:

https://github.com/jetpack-io/devbox/blob/main/internal/nix/nixprofile/profile.go#L144
This call is okay, because we have a separate if-condition that handles BinaryCache (i.e. store-paths) installs and is checked before this call.

https://github.com/jetpack-io/devbox/blob/main/internal/impl/packages.go#L481
This one in extraPackagesInProfile does feel problematic. Because the item.UnlockedReference here will be # for items-from-store-paths.

Hmm not sure how to fix (2).

I'm re-thinking installing via store-path in the profile. Did we go down a wrong path by installing by store-paths in the nix profile?

Downside:

we cannot recover a devpkg.Package from these item-from-store-paths

Since we switched from content-addressed paths back to input-addressed-paths, I'm wondering if we should continue with the old way of installing packages in profiles. What do we gain (to remind me) from installing by store-path? In theory, it should be the same I believe.

WDYT?

Okay, looking at the PR where I made install happen via nix store path: #1255

the reason we must do it is to avoid downloading nixpkgs. Cool, we need it!

Okay, to fix (2), I think we can do something like:

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 { profileInput := item.ToPackage(d.lockfile) for _, devboxInput := range devboxInputs { // assuming devpkg.FillNarInfoCache() is already called if devboxInput.IsInBinaryCache() { for _, path := range item.storePaths { if devboxInput.InputAddressedPath() == path { // mark as present } } } if profileInput.Equals(devboxInput) { continue outer } } extras = append(extras, item) }

WDYT?

Sorry for delay answering. Yes, this function, ProfileListItem.ToPackage() is definitely a problem with store path-added Items. That's why I left a TODO in there that wasn't there before. And this is the problem I was saying where we sometimes "resolve" a package to <default-hash>##, which is, well, dumb.

But yes, I think the code as it is today will continue to work just fine, but we should indeed do better here. I think we'll have to change the callsites like you're suggesting, but let's do that in a separate PR

While testing the upgrade path, I concluded that this is an important bug to fix (in separate PR). That's because tidyProfile will run the pairwise checks to match profile items with devbox packages every time the number of items in the profile differs from the number of items in devbox.json. And thus, every single time we run into this condition (which I think will be often), we'll end up removing all store path-installed items from the profile, because none of them will ever match with what's in devbox.json

savil · 2023-08-23T23:21:31Z

internal/nix/nixprofile/profile.go

 			if pathInStore == item.nixStorePath {
 				return item.index, nil
 			}
 		}
+		return -1, errors.Wrap(nix.ErrPackageNotFound, args.DevPkg.String())


I think that makes sense.

One scenario I was thinking about is (sequentially):

Existing devbox user has installed a package.

We ship a Devbox update that has Remove Nixpkgs feature enabled.

This code here runs. The Package happens to be in the binary cache, BUT it had previously been installed the "old way" in the nix profile.

I think this code should still work, since the store path is input-addressed and would match here.

ipince requested a review from savil August 23, 2023 17:48

ipince commented Aug 23, 2023

View reviewed changes

savil approved these changes Aug 23, 2023

View reviewed changes

ipince force-pushed the rodrigo/rm-nixpkgs-profile-bug branch from 3b27d43 to 2ca4543 Compare August 28, 2023 21:18

ipince added 5 commits August 28, 2023 22:57

[rm-nixpkgs] Fix profile list bug for packages added by store path

5e5b774

add missing file

789f114

as string

104be8b

lint

65ea680

DevPkg -> Package

2166973

ipince force-pushed the rodrigo/rm-nixpkgs-profile-bug branch from 2ca4543 to 2166973 Compare August 29, 2023 02:57

bump timeout to 30m

74f8bf5

ipince merged commit 6807da6 into main Aug 29, 2023

ipince deleted the rodrigo/rm-nixpkgs-profile-bug branch August 29, 2023 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[rm-nixpkgs] Fix profile list bug for packages added by store path #1416

[rm-nixpkgs] Fix profile list bug for packages added by store path #1416

Uh oh!

ipince commented Aug 23, 2023

Uh oh!

ipince Aug 23, 2023

Uh oh!

ipince Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

ipince Aug 28, 2023

Uh oh!

savil left a comment •

edited

Loading

Uh oh!

savil Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

ipince Aug 28, 2023

Uh oh!

ipince Aug 28, 2023

Uh oh!

savil Aug 23, 2023

Uh oh!

Uh oh!

[rm-nixpkgs] Fix profile list bug for packages added by store path #1416

[rm-nixpkgs] Fix profile list bug for packages added by store path #1416

Uh oh!

Conversation

ipince commented Aug 23, 2023

Summary

How was it tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil left a comment •

edited

Loading