Fix bug when comparing profile item added by store path (#1422)

## Summary
When an package is added by store path, we can no longer recover the
original package from the profile item itself, because the only
information we have is the nix store path. So, some comparisons where we
were trying to find if a profile item matched a devbox package were
broken.

Here we introduce a `Item.Matches(Package)` function to return true if
the item was installed from the package. It attempts to hide the
complexity of whether the item was installed by store path or not inside
it, so callers don't have to worry about it. Modified the callsites to
`ToPackage()` to use `Matches` instead, which simplified some logic.

Also added a `len == 0` check to calls to `nix profile remove`, since I
noticed we were sometimes calling it with no arguments. No failures
here, but just wasted time.

## How was it tested?
Manually tested by adding/removing packages and logging all the
profile-related calls and item/package comparisons.

---------

Signed-off-by: Rodrigo Ipince <[email protected]>

Author: ipince

internal/devpkg/package.go

@@ -424,7 +424,7 @@ func (p *Package) LegacyToVersioned() string {
 	return p.Raw + "@latest"
 }
 
-// ensureNixpkgsPrefetched will prefetch flake for the nixpkgs registry for the package.
+// EnsureNixpkgsPrefetched will prefetch flake for the nixpkgs registry for the package.
 // This is an internal method, and should not be called directly.
 func EnsureNixpkgsPrefetched(ctx context.Context, w io.Writer, pkgs []*Package) error {
 	if err := FillNarInfoCache(ctx, pkgs...); err != nil {

internal/impl/packages.go

@@ -461,12 +461,12 @@ func (d *Devbox) extraPackagesInProfile(ctx context.Context) ([]*nixprofile.NixP
 	if err != nil {
 		return nil, err
 	}
-	devboxInputs, err := d.AllInstallablePackages()
+	packages, err := d.AllInstallablePackages()
 	if err != nil {
 		return nil, err
 	}
 
-	if len(devboxInputs) == len(profileItems) {
+	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.
@@ -478,9 +478,8 @@ func (d *Devbox) extraPackagesInProfile(ctx context.Context) ([]*nixprofile.NixP
 	// 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 {
-			if profileInput.Equals(devboxInput) {
+		for _, pkg := range packages {
+			if item.Matches(pkg, d.lockfile) {
 				continue outer
 			}
 		}

internal/nix/nixprofile/item.go

@@ -17,14 +17,15 @@ type NixProfileListItem struct {
 	index int
 
 	// The original ("unlocked") flake reference and output attribute path used at installation time.
-	// NOTE that this can be "#" if the package was added to the nix profile via store path.
+	// NOTE that this will be empty if the package was added to the nix profile via store path.
 	unlockedReference string
 
 	// The locked flake reference to which the unlocked flake reference was resolved.
-	// NOTE that this can be "#" if the package was added to the nix profile via store path.
+	// NOTE that this will be empty if the package was added to the nix profile via store path.
 	lockedReference string
 
-	// The store path(s) of the package.
+	// The store path(s) of the package. Should have at least 1 path, and should have exactly 1 path
+	// if the item was added to the profile through a store path.
 	nixStorePaths []string
 }
 
@@ -33,35 +34,52 @@ type NixProfileListItem struct {
 // For example:
 // if NixProfileListItem.lockedReference = github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19
 // then AttributePath = legacyPackages.x86_64-darwin.go_1_19
-func (item *NixProfileListItem) AttributePath() (string, error) {
+func (i *NixProfileListItem) AttributePath() (string, error) {
 	// lockedReference example:
 	// github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19
 
 	// AttributePath example:
 	// legacyPackages.x86_64.go_1_19
-	_ /*nixpkgs*/, attrPath, found := strings.Cut(item.lockedReference, "#")
+	_ /*nixpkgs*/, attrPath, found := strings.Cut(i.lockedReference, "#")
 	if !found {
 		return "", redact.Errorf(
 			"expected to find # in lockedReference: %s from NixProfileListItem: %s",
-			redact.Safe(item.lockedReference),
-			item,
+			redact.Safe(i.lockedReference),
+			i,
 		)
 	}
 	return attrPath, nil
 }
 
-// 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)
+// Matches compares a devpkg.Package with this profile item and returns true if the profile item
+// was the result of adding the Package to the nix profile.
+func (i *NixProfileListItem) Matches(pkg *devpkg.Package, locker lock.Locker) bool {
+	if i.addedByStorePath() {
+		// If an Item was added via store path, the best we can do when comparing to a Package is to check
+		// if its store path matches that of the Package. Note that the item should only have 1 store path.
+		path, err := pkg.InputAddressedPath()
+		if err != nil {
+			// pkg couldn't have been added by store path if we can't get the store path for it, so return
+			// false. There are some edge cases (e.g. cache is down, index changed, etc., but it's OK to
+			// err on the side of false).
+			return false
+		}
+		return len(i.nixStorePaths) == 1 && i.nixStorePaths[0] == path
+	}
+
+	return pkg.Equals(devpkg.PackageFromString(i.unlockedReference, locker))
+}
+
+func (i *NixProfileListItem) addedByStorePath() bool {
+	return i.unlockedReference == ""
 }
 
 // String serializes the NixProfileListItem back into the format printed by `nix profile list`
-func (item *NixProfileListItem) String() string {
-	return fmt.Sprintf("%d %s %s %s",
-		item.index,
-		item.unlockedReference,
-		item.lockedReference,
-		item.nixStorePaths,
+func (i *NixProfileListItem) String() string {
+	return fmt.Sprintf("{%d %s %s %s}",
+		i.index,
+		i.unlockedReference,
+		i.lockedReference,
+		i.nixStorePaths,
 	)
 }

internal/nix/nixprofile/profile.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/fatih/color"
 	"github.com/pkg/errors"
+	"github.com/samber/lo"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/devpkg"
 	"go.jetpack.io/devbox/internal/lock"
@@ -56,8 +57,8 @@ func ProfileListItems(
 	for index, element := range structOutput.Elements {
 		items = append(items, &NixProfileListItem{
 			index:             index,
-			unlockedReference: element.OriginalURL + "#" + element.AttrPath,
-			lockedReference:   element.URL + "#" + element.AttrPath,
+			unlockedReference: lo.Ternary(element.OriginalURL != "", element.OriginalURL+"#"+element.AttrPath, ""),
+			lockedReference:   lo.Ternary(element.URL != "", element.URL+"#"+element.AttrPath, ""),
 			nixStorePaths:     element.StorePaths,
 		})
 	}
@@ -125,41 +126,25 @@ func ProfileListIndex(args *ProfileListIndexArgs) (int, error) {
 	if err != nil {
 		return -1, err
 	}
-	if inCache {
-		// Packages in cache are added by store path, which means we only need to check
-		// for store path equality to find it.
-		pathInStore, err := args.Package.Installable()
+
+	if !inCache {
+		// This is an optimization for happy path when packages are added by flake reference. A resolved devbox
+		// package *which was added by flake reference* (not by store path) should match the unlockedReference
+		// of an existing profile item.
+		ref, err := args.Package.NormalizedDevboxPackageReference()
 		if err != nil {
 			return -1, errors.Wrapf(err, "failed to get installable for %s", args.Package.String())
 		}
 		for _, item := range items {
-			if len(item.nixStorePaths) == 1 && // this should always be true
-				pathInStore == item.nixStorePaths[0] {
+			if ref == item.unlockedReference {
 				return item.index, nil
 			}
 		}
 		return -1, errors.Wrap(nix.ErrPackageNotFound, args.Package.String())
 	}
 
-	// else: check if the Package matches an item's unlockedReference.
-	// This is an optimization for happy path. A resolved devbox package *which was added by
-	// flake reference* (not by store path) should match the unlockedReference of an existing
-	// profile item.
-	ref, err := args.Package.NormalizedDevboxPackageReference()
-	if err != nil {
-		return -1, err
-	}
 	for _, item := range items {
-		if ref == item.unlockedReference {
-			return item.index, nil
-		}
-	}
-
-	// Still not found? Check for full pkg equality (may be expensive).
-	for _, item := range items {
-		existing := item.ToPackage(args.Lockfile)
-
-		if args.Package.Equals(existing) {
+		if item.Matches(args.Package, args.Lockfile) {
 			return item.index, nil
 		}
 	}
@@ -276,7 +261,7 @@ func ProfileInstall(ctx context.Context, args *ProfileInstallArgs) error {
 // 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 {
+	if len(items) == 0 {
 		return nil
 	}
 	indexes := []string{}