Repository: jetpack-io/devbox
Pull request #1940 (2024-03-28 18:18:58)

[plugins] Fix rm for packages with __remove_trigger_package
## Summary

Fixes issue where user could not remove package that is replaced by a flake in plugin (uses `__remove_trigger_package`)

Fixes https://github.com/jetpack-io/devbox/issues/1936

We need two different functions for packages:

* `AllPackages` which is used for instalation and lock file. Includes packages added/removed by plugins.
* `TopLevelPackages` is only what is in root devbox.json

## How was it tested?

* Tested manually
* Added php to rm unit test


Patch:
diff --git a/docs/app/docs/faq.md b/docs/app/docs/faq.md
index c77d3d8c..0b782ca4 100644
--- a/docs/app/docs/faq.md
+++ b/docs/app/docs/faq.md
@@ -15,7 +15,7 @@ ## Where does Devbox install my packages?
 
 ## How do I clean up unused packages from the Nix Store?
 
-You can use `devbox run -- nix store gc` to automatically clean up packages that are no longer needed for your projects.
+You can use `devbox run -- nix store gc --extra-experimental-features nix-command` to automatically clean up packages that are no longer needed for your projects.
 
 ## Does Devbox require Docker or Containers to work?
 
diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go
index 4ddbbf1a..895b752d 100644
--- a/internal/devbox/devbox.go
+++ b/internal/devbox/devbox.go
@@ -176,7 +176,7 @@ func (d *Devbox) ConfigHash() (string, error) {
 
 	buf := bytes.Buffer{}
 	buf.WriteString(h)
-	for _, pkg := range d.ConfigPackages() {
+	for _, pkg := range d.AllPackages() {
 		buf.WriteString(pkg.Hash())
 	}
 	for _, pluginConfig := range d.cfg.IncludedPluginConfigs() {
@@ -282,7 +282,7 @@ func (d *Devbox) RunScript(ctx context.Context, cmdName string, cmdArgs []string
 		env["DEVBOX_RUN_CMD"] = strings.Join(append([]string{cmdName}, cmdArgs...), " ")
 	}
 
-	return nix.RunScript(d.projectDir, cmdWithArgs, env)
+	return nix.RunScript(d.projectDir, strings.Join(cmdWithArgs, " "), env)
 }
 
 // Install ensures that all the packages in the config are installed
@@ -1033,21 +1033,27 @@ func (d *Devbox) PackageNames() []string {
 	return d.cfg.PackagesVersionedNames()
 }
 
-// ConfigPackages returns the packages that are defined in devbox.json
-// NOTE: the return type is different from devconfig.Packages
-func (d *Devbox) ConfigPackages() []*devpkg.Package {
+// AllPackages returns the packages that are defined in devbox.json and
+// recursively added by plugins.
+// NOTE: This will not return packages removed by their plugin with the
+// __remove_trigger_package field.
+func (d *Devbox) AllPackages() []*devpkg.Package {
 	return devpkg.PackagesFromConfig(d.cfg.Packages(), d.lockfile)
 }
 
+func (d *Devbox) TopLevelPackages() []*devpkg.Package {
+	return devpkg.PackagesFromConfig(d.cfg.Root.TopLevelPackages(), d.lockfile)
+}
+
 // InstallablePackages returns the packages that are to be installed
 func (d *Devbox) InstallablePackages() []*devpkg.Package {
-	return lo.Filter(d.ConfigPackages(), func(pkg *devpkg.Package, _ int) bool {
+	return lo.Filter(d.AllPackages(), func(pkg *devpkg.Package, _ int) bool {
 		return pkg.IsInstallable()
 	})
 }
 
 func (d *Devbox) HasDeprecatedPackages() bool {
-	for _, pkg := range d.ConfigPackages() {
+	for _, pkg := range d.AllPackages() {
 		if pkg.IsLegacy() {
 			return true
 		}
@@ -1060,7 +1066,7 @@ func (d *Devbox) findPackageByName(name string) (*devpkg.Package, error) {
 		return nil, errors.New("package name cannot be empty")
 	}
 	results := map[*devpkg.Package]bool{}
-	for _, pkg := range d.ConfigPackages() {
+	for _, pkg := range d.TopLevelPackages() {
 		if pkg.Raw == name || pkg.CanonicalName() == name {
 			results[pkg] = true
 		}
diff --git a/internal/devbox/docgen/docgen.go b/internal/devbox/docgen/docgen.go
index 0d2bd42d..537ac862 100644
--- a/internal/devbox/docgen/docgen.go
+++ b/internal/devbox/docgen/docgen.go
@@ -56,7 +56,8 @@ func GenerateReadme(
 		"Scripts":     devbox.Config().Scripts(),
 		"EnvVars":     devbox.Config().Env(),
 		"InitHook":    devbox.Config().InitHook(),
-		"Packages":    devbox.ConfigPackages(),
+		"Packages":    devbox.TopLevelPackages(),
+		// TODO add includes
 	})
 }
 
diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go
index e69a00ba..55caf537 100644
--- a/internal/devbox/packages.go
+++ b/internal/devbox/packages.go
@@ -294,7 +294,7 @@ func (d *Devbox) updateLockfile(recomputeState bool) error {
 	d.lockfile.Tidy()
 
 	// Update lockfile with new packages that are not to be installed
-	for _, pkg := range d.ConfigPackages() {
+	for _, pkg := range d.AllPackages() {
 		if err := pkg.EnsureUninstallableIsInLockfile(); err != nil {
 			return err
 		}
diff --git a/internal/devbox/shellrc.tmpl b/internal/devbox/shellrc.tmpl
index 44b4780d..6f0e5f52 100644
--- a/internal/devbox/shellrc.tmpl
+++ b/internal/devbox/shellrc.tmpl
@@ -56,7 +56,7 @@ working_dir="$(pwd)"
 cd "{{ .ProjectDir }}" || exit
 
 # Source the hooks file, which contains the project's init hooks and plugin hooks.
-. "{{ .HooksFilePath }}"
+. {{ .HooksFilePath }}
 
 cd "$working_dir" || exit
 
diff --git a/internal/devbox/shellrc_fish.tmpl b/internal/devbox/shellrc_fish.tmpl
index 26ecfb7e..a50b3fb2 100644
--- a/internal/devbox/shellrc_fish.tmpl
+++ b/internal/devbox/shellrc_fish.tmpl
@@ -59,7 +59,7 @@ set workingDir (pwd)
 cd "{{ .ProjectDir }}" || exit
 
 # Source the hooks file, which contains the project's init hooks and plugin hooks.
-source "{{ .HooksFilePath }}"
+source {{ .HooksFilePath }}
 
 cd "$workingDir" || exit
 
diff --git a/internal/devbox/update.go b/internal/devbox/update.go
index 23d84cb3..59380b93 100644
--- a/internal/devbox/update.go
+++ b/internal/devbox/update.go
@@ -81,7 +81,7 @@ func (d *Devbox) inputsToUpdate(
 	opts devopt.UpdateOpts,
 ) ([]*devpkg.Package, error) {
 	if len(opts.Pkgs) == 0 {
-		return d.ConfigPackages(), nil
+		return d.AllPackages(), nil
 	}
 
 	var pkgsToUpdate []*devpkg.Package
diff --git a/internal/nix/nix.go b/internal/nix/nix.go
index 47075019..0f858d17 100644
--- a/internal/nix/nix.go
+++ b/internal/nix/nix.go
@@ -19,7 +19,6 @@
 	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/redact"
-	"go.jetpack.io/devbox/nix/flake"
 
 	"go.jetpack.io/devbox/internal/debug"
 )
@@ -70,19 +69,15 @@ func (*Nix) PrintDevEnv(ctx context.Context, args *PrintDevEnvArgs) (*PrintDevEn
 		return nil, errors.WithStack(err)
 	}
 
-	flakeRef, err := flake.ParseRef("path:" + flakeDirResolved)
-	if err != nil {
-		return nil, err
-	}
-
 	if len(data) == 0 {
 		cmd := exec.CommandContext(
 			ctx,
-			"nix", "print-dev-env", flakeRef.String(),
+			"nix", "print-dev-env",
+			"path:"+flakeDirResolved,
 		)
 		cmd.Args = append(cmd.Args, ExperimentalFlags()...)
 		cmd.Args = append(cmd.Args, "--json")
-		debug.Log("Running print-dev-env cmd: %s\n\n\n", cmd)
+		debug.Log("Running print-dev-env cmd: %s\n", cmd)
 		data, err = cmd.Output()
 		if insecure, insecureErr := IsExitErrorInsecurePackage(err, "" /*pkgName*/, "" /*installable*/); insecure {
 			return nil, insecureErr
diff --git a/internal/nix/run.go b/internal/nix/run.go
index 34010731..81028081 100644
--- a/internal/nix/run.go
+++ b/internal/nix/run.go
@@ -8,15 +8,14 @@
 	"fmt"
 	"os"
 	"os/exec"
-	"strings"
 
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/cmdutil"
 	"go.jetpack.io/devbox/internal/debug"
 )
 
-func RunScript(projectDir string, cmdWithArgs []string, env map[string]string) error {
-	if len(cmdWithArgs) == 0 {
+func RunScript(projectDir, cmdWithArgs string, env map[string]string) error {
+	if cmdWithArgs == "" {
 		return errors.New("attempted to run an empty command or script")
 	}
 
@@ -25,13 +24,9 @@ func RunScript(projectDir string, cmdWithArgs []string, env map[string]string) e
 		envPairs = append(envPairs, fmt.Sprintf("%s=%s", k, v))
 	}
 
-	// Wrap in quotations since the command's path may contain spaces.
-	cmdWithArgs[0] = "\"" + cmdWithArgs[0] + "\""
-	cmdWithArgsStr := strings.Join(cmdWithArgs, " ")
-
 	// Try to find sh in the PATH, if not, default to a well known absolute path.
 	shPath := cmdutil.GetPathOrDefault("sh", "/bin/sh")
-	cmd := exec.Command(shPath, "-c", cmdWithArgsStr)
+	cmd := exec.Command(shPath, "-c", cmdWithArgs)
 	cmd.Env = envPairs
 	cmd.Dir = projectDir
 	cmd.Stdin = os.Stdin
diff --git a/internal/shellgen/tmpl/init-hook-wrapper.tmpl b/internal/shellgen/tmpl/init-hook-wrapper.tmpl
index 3da5fddd..abe3d4ce 100644
--- a/internal/shellgen/tmpl/init-hook-wrapper.tmpl
+++ b/internal/shellgen/tmpl/init-hook-wrapper.tmpl
@@ -5,5 +5,5 @@ Code here should be fish and POSIX compatible. That's why we use export to
 remove the value
 */ -}}
 export {{ .InitHookHash }}=true
-. "{{ .RawHooksFile }}"
+. {{ .RawHooksFile }}
 export {{ .InitHookHash }}=""
diff --git a/internal/shellgen/tmpl/script-wrapper.tmpl b/internal/shellgen/tmpl/script-wrapper.tmpl
index d048c083..f6b04721 100644
--- a/internal/shellgen/tmpl/script-wrapper.tmpl
+++ b/internal/shellgen/tmpl/script-wrapper.tmpl
@@ -9,7 +9,7 @@
 
 if [ -z "${{ .InitHookHash }}" ]; then
     {{/* init hooks will export InitHookHash ensuring no recursive sourcing*/ -}}
-    . "{{ .InitHookPath }}"
+    . {{ .InitHookPath }}
 fi
 
 {{ .Body }}


Test patch:
diff --git a/internal/devbox/testdata/shellrc/basic/shellrc.golden b/internal/devbox/testdata/shellrc/basic/shellrc.golden
index 19e5813c..9e110a4a 100644
--- a/internal/devbox/testdata/shellrc/basic/shellrc.golden
+++ b/internal/devbox/testdata/shellrc/basic/shellrc.golden
@@ -21,7 +21,7 @@ working_dir="$(pwd)"
 cd "/path/to/projectDir" || exit
 
 # Source the hooks file, which contains the project's init hooks and plugin hooks.
-. "/path/to/projectDir/.devbox/gen/scripts/.hooks.sh"
+. /path/to/projectDir/.devbox/gen/scripts/.hooks.sh
 
 cd "$working_dir" || exit
 
diff --git a/internal/devbox/testdata/shellrc/noshellrc/shellrc.golden b/internal/devbox/testdata/shellrc/noshellrc/shellrc.golden
index f21ece85..e7ac3d7b 100644
--- a/internal/devbox/testdata/shellrc/noshellrc/shellrc.golden
+++ b/internal/devbox/testdata/shellrc/noshellrc/shellrc.golden
@@ -15,7 +15,7 @@ working_dir="$(pwd)"
 cd "/path/to/projectDir" || exit
 
 # Source the hooks file, which contains the project's init hooks and plugin hooks.
-. "/path/to/projectDir/.devbox/gen/scripts/.hooks.sh"
+. /path/to/projectDir/.devbox/gen/scripts/.hooks.sh
 
 cd "$working_dir" || exit
 
diff --git a/testscripts/rm/add-rm.test.txt b/testscripts/rm/add-rm.test.txt
index a34ae6f3..a0aea6ba 100644
--- a/testscripts/rm/add-rm.test.txt
+++ b/testscripts/rm/add-rm.test.txt
@@ -1,20 +1,20 @@
 exec devbox init
 
-exec devbox add hello vim cowsay
+exec devbox add hello vim cowsay php
 json.superset devbox.json all.json
 
-exec devbox rm vim hello
+exec devbox rm vim hello php
 json.superset devbox.json cowsay.json
 
-exec devbox add vim hello vim hello vim hello vim hello cowsay
+exec devbox add vim hello vim hello vim hello vim hello cowsay php php
 json.superset devbox.json all.json
 
-exec devbox rm vim hello cowsay cowsay
+exec devbox rm vim hello cowsay cowsay php
 json.superset devbox.json empty.json
 
 -- all.json --
 {
-  "packages": ["hello@latest", "vim@latest", "cowsay@latest"]
+  "packages": ["hello@latest", "vim@latest", "cowsay@latest", "php@latest"]
 }
 
 -- cowsay.json --