[versioned] If user adds new version, remove previous versions of canonical (#1105)

## Summary

If there's an existing package with same canonical, remove it. If
there's more than one do nothing.

## How was it tested?

Need to test this a bit more to ensure no weird side-effects of doing a
`remove` inside an `add`

On this repo did:

```bash
cat devbox.json
devbox add [email protected]
cat devbox.json
devbox add [email protected]
cat devbox.json
```

Author: mikeland73

internal/impl/packages.go

@@ -26,31 +26,39 @@ import (
 	"go.jetpack.io/devbox/internal/wrapnix"
 )
 
-// packages.go has functions for adding, removing and getting info about nix packages
+// packages.go has functions for adding, removing and getting info about nix
+// packages
 
-// Add adds the `pkgs` to the config (i.e. devbox.json) and nix profile for this devbox project
+// Add adds the `pkgs` to the config (i.e. devbox.json) and nix profile for this
+// devbox project
 func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {
 	ctx, task := trace.NewTask(ctx, "devboxAdd")
 	defer task.End()
 
-	pkgs := nix.InputsFromStrings(lo.Uniq(pkgsNames), d.lockfile)
-
-	versionedPackages := []*nix.Input{}
-	// Add to Packages of the config only if it's not already there. We do this
-	// before addin @latest to ensure we don't accidentally add a package that
-	// is already in the config.
-	for _, pkg := range pkgs {
+	// Only add packages that are not already in config. If same canonical exists,
+	// replace it.
+	pkgs := []*nix.Input{}
+	for _, pkg := range nix.InputsFromStrings(lo.Uniq(pkgsNames), d.lockfile) {
 		versioned := pkg.Versioned()
-		versionedPackages = append(
-			versionedPackages,
-			nix.InputFromString(versioned, d.lockfile),
-		)
-		// Only add if the package doesn't exist versioned or unversioned.
-		if !slices.Contains(d.cfg.Packages, pkg.Raw) && !slices.Contains(d.cfg.Packages, versioned) {
-			d.cfg.Packages = append(d.cfg.Packages, versioned)
+
+		// If exact versioned package is already in the config, skip.
+		if slices.Contains(d.cfg.Packages, versioned) {
+			continue
 		}
+
+		// On the other hand, if there's a package with same canonical name, replace
+		// it. Ignore error (which is either missing or more than one). We search by
+		// CanonicalName so any legacy or versioned packages will be removed if they
+		// match.
+		if name, _ := d.findPackageByName(pkg.CanonicalName()); name != "" {
+			if err := d.Remove(ctx, name); err != nil {
+				return err
+			}
+		}
+
+		pkgs = append(pkgs, nix.InputFromString(versioned, d.lockfile))
+		d.cfg.Packages = append(d.cfg.Packages, versioned)
 	}
-	pkgs = versionedPackages
 
 	// Check packages are valid before adding.
 	for _, pkg := range pkgs {

testscripts/add/add_replace.test.txt

@@ -0,0 +1,18 @@
+# Testscript for exercising adding packages
+
+exec devbox init
+
+exec devbox add [email protected]
+devboxjson.packages.contains devbox.json [email protected]
+! devboxjson.packages.contains devbox.json [email protected]
+
+exec devbox add [email protected]
+! devboxjson.packages.contains devbox.json [email protected]
+devboxjson.packages.contains devbox.json [email protected]
+
+-- devbox.json --
+{
+  "packages": [
+    "[email protected]"
+  ]
+}

testscripts/testrunner/assert.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/rogpeppe/go-internal/testscript"
 
+	"go.jetpack.io/devbox/internal/devconfig"
 	"go.jetpack.io/devbox/internal/envir"
 )
 
@@ -33,6 +34,31 @@ func assertPathLength(script *testscript.TestScript, neg bool, args []string) {
 	}
 }
 
+func assertDevboxJSONPackagesContains(script *testscript.TestScript, neg bool, args []string) {
+	if len(args) != 2 {
+		script.Fatalf("usage: json.list.contains list.json value")
+	}
+
+	data := script.ReadFile(args[0])
+	list := devconfig.Config{}
+	err := json.Unmarshal([]byte(data), &list)
+	script.Check(err)
+
+	expected := args[1]
+	for _, actual := range list.Packages {
+		if actual == expected {
+			if neg {
+				script.Fatalf("value '%s' found in '%s'", expected, list.Packages)
+			}
+			return
+		}
+	}
+
+	if !neg {
+		script.Fatalf("value '%s' not found in '%s'", expected, list.Packages)
+	}
+}
+
 // Usage: json.superset superset.json subset.json
 // Checks that the JSON in superset.json contains all the keys and values
 // present in subset.json.

testscripts/testrunner/testrunner.go

@@ -74,10 +74,11 @@ func getTestscriptParams(t *testing.T, dir string) testscript.Params {
 		TestWork:            false, // Set to true if you're trying to debug a test.
 		Setup:               func(env *testscript.Env) error { return setupTestEnv(t, env) },
 		Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){
-			"env.path.len":  assertPathLength,
-			"json.superset": assertJSONSuperset,
-			"path.order":    assertPathOrder,
-			"source.path":   sourcePath,
+			"env.path.len":                 assertPathLength,
+			"devboxjson.packages.contains": assertDevboxJSONPackagesContains,
+			"json.superset":                assertJSONSuperset,
+			"path.order":                   assertPathOrder,
+			"source.path":                  sourcePath,
 		},
 	}
 }