[bug-fix] Fix lockfile rewrite (#2007)

## Summary

The source of this bug is that when calling `lockfile.Tidy()` we were
passing in only top level packages and not including plugin packages as
well.

This bug was also affecting dev container construction and some edge
case plugin template logic. This bug only affects users that are using
custom plugins with packages.

I renamed `PackageNames` to `AllPackageNames` for clarity. Also removed
a redundant function in config package that was no longer needed.
 
## How was it tested?

In other project added golang monorepo plugin and logged to ensure Tidy
was not recreating lockfile. Also tested adding go to that project.

Author: mikeland73

internal/boxcli/global.go

@@ -49,9 +49,7 @@ func globalCmd() *cobra.Command {
 	addCommandAndHideConfigFlag(globalCmd, servicesCmd(persistentPreRunE))
 	addCommandAndHideConfigFlag(globalCmd, shellEnv)
 	addCommandAndHideConfigFlag(globalCmd, updateCmd())
-
-	// Create list for non-global? Mike: I want it :)
-	globalCmd.AddCommand(globalListCmd())
+	addCommandAndHideConfigFlag(globalCmd, listCmd())
 
 	return globalCmd
 }
@@ -61,33 +59,33 @@ func addCommandAndHideConfigFlag(parent, child *cobra.Command) {
 	_ = child.Flags().MarkHidden("config")
 }
 
-func globalListCmd() *cobra.Command {
-	return &cobra.Command{
+type listCmdFlags struct {
+	config configFlags
+}
+
+func listCmd() *cobra.Command {
+	flags := listCmdFlags{}
+	cmd := &cobra.Command{
 		Use:     "list",
 		Aliases: []string{"ls"},
 		Short:   "List global packages",
 		PreRunE: ensureNixInstalled,
-		RunE:    listGlobalCmdFunc,
-	}
-}
-
-func listGlobalCmdFunc(cmd *cobra.Command, args []string) error {
-	path, err := ensureGlobalConfig()
-	if err != nil {
-		return errors.WithStack(err)
-	}
-
-	box, err := devbox.Open(&devopt.Opts{
-		Dir:    path,
-		Stderr: cmd.ErrOrStderr(),
-	})
-	if err != nil {
-		return errors.WithStack(err)
-	}
-	for _, p := range box.PackageNames() {
-		fmt.Fprintf(cmd.OutOrStdout(), "* %s\n", p)
+		RunE: func(cmd *cobra.Command, args []string) error {
+			box, err := devbox.Open(&devopt.Opts{
+				Dir:    flags.config.path,
+				Stderr: cmd.ErrOrStderr(),
+			})
+			if err != nil {
+				return errors.WithStack(err)
+			}
+			for _, p := range box.AllPackageNamesIncludingRemovedTriggerPackages() {
+				fmt.Fprintf(cmd.OutOrStdout(), "* %s\n", p)
+			}
+			return nil
+		},
 	}
-	return nil
+	flags.config.register(cmd)
+	return cmd
 }
 
 var globalConfigPath string

internal/boxcli/midcobra/telemetry.go

@@ -108,5 +108,6 @@ func getPackagesAndCommitHash(c *cobra.Command) ([]string, string) {
 		return []string{}, ""
 	}
 
-	return box.Config().PackagesVersionedNames(), box.Config().NixPkgsCommitHash()
+	return box.AllPackageNamesIncludingRemovedTriggerPackages(),
+		box.Config().NixPkgsCommitHash()
 }

internal/boxcli/root.go

@@ -69,6 +69,7 @@ func RootCmd() *cobra.Command {
 	command.AddCommand(initCmd())
 	command.AddCommand(installCmd())
 	command.AddCommand(integrateCmd())
+	command.AddCommand(listCmd())
 	command.AddCommand(logCmd())
 	command.AddCommand(removeCmd())
 	command.AddCommand(runCmd())

internal/devbox/devbox.go

@@ -446,7 +446,7 @@ func (d *Devbox) GenerateDevcontainer(ctx context.Context, generateOpts devopt.G
 		Path:           devContainerPath,
 		RootUser:       generateOpts.RootUser,
 		IsDevcontainer: true,
-		Pkgs:           d.PackageNames(),
+		Pkgs:           d.AllPackageNamesIncludingRemovedTriggerPackages(),
 		LocalFlakeDirs: d.getLocalFlakesDirs(),
 	}
 
@@ -485,7 +485,7 @@ func (d *Devbox) GenerateDockerfile(ctx context.Context, generateOpts devopt.Gen
 		Path:           d.projectDir,
 		RootUser:       generateOpts.RootUser,
 		IsDevcontainer: false,
-		Pkgs:           d.PackageNames(),
+		Pkgs:           d.AllPackageNamesIncludingRemovedTriggerPackages(),
 		LocalFlakeDirs: d.getLocalFlakesDirs(),
 	}
 
@@ -1039,19 +1039,29 @@ func (d *Devbox) flakeDir() string {
 	return filepath.Join(d.projectDir, ".devbox/gen/flake")
 }
 
-// ConfigPackageNames returns the package names as defined in devbox.json
-func (d *Devbox) PackageNames() []string {
-	// TODO savil: centralize implementation by calling d.configPackages and getting pkg.Raw
-	// Skipping for now to avoid propagating the error value.
-	return d.cfg.PackagesVersionedNames()
+// AllPackageNamesIncludingRemovedTriggerPackages returns the all package names,
+// including those added by plugins and also those removed by builtins.
+// This has a gross name to differentiate it from AllPackages.
+// Some uses cases for this are the lockfile and devbox list command.
+//
+// TODO: We may want to get rid of this function and have callers
+// build their own list. e.g. Some callers need different representations of
+// flakes  (lockfile vs devbox list)
+func (d *Devbox) AllPackageNamesIncludingRemovedTriggerPackages() []string {
+	result := []string{}
+	for _, p := range d.cfg.Packages(true /*includeRemovedTriggerPackages*/) {
+		result = append(result, p.VersionedName())
+	}
+	return result
 }
 
 // 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)
+	packages := d.cfg.Packages(false /*includeRemovedTriggerPackages*/)
+	return devpkg.PackagesFromConfig(packages, d.lockfile)
 }
 
 func (d *Devbox) TopLevelPackages() []*devpkg.Package {

internal/devbox/flakes.go

@@ -14,7 +14,7 @@ func (d *Devbox) getLocalFlakesDirs() []string {
 	localFlakeDirs := []string{}
 
 	// searching through installed packages to get location of local flakes
-	for _, pkg := range d.PackageNames() {
+	for _, pkg := range d.AllPackageNamesIncludingRemovedTriggerPackages() {
 		// filtering local flakes packages
 		if strings.HasPrefix(pkg, "path:") {
 			pkgDirAndName, _ := strings.CutPrefix(pkg, "path:")

internal/devbox/packages.go

@@ -20,6 +20,7 @@ import (
 	"github.com/samber/lo"
 	"go.jetpack.io/devbox/internal/devbox/devopt"
 	"go.jetpack.io/devbox/internal/devconfig"
+	"go.jetpack.io/devbox/internal/devconfig/configfile"
 	"go.jetpack.io/devbox/internal/devpkg"
 	"go.jetpack.io/devbox/internal/devpkg/pkgtype"
 	"go.jetpack.io/devbox/internal/lock"
@@ -53,7 +54,10 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt
 	// names of added packages (even if they are already in config). We use this
 	// to know the exact name to mark as allowed insecure later on.
 	addedPackageNames := []string{}
-	existingPackageNames := d.PackageNames()
+	existingPackageNames := lo.Map(
+		d.cfg.Root.TopLevelPackages(), func(p configfile.Package, _ int) string {
+			return p.VersionedName()
+		})
 	for _, pkg := range pkgs {
 		// If exact versioned package is already in the config, we can skip the
 		// next loop that only deals with newPackages.

internal/devconfig/config.go

@@ -193,13 +193,20 @@ func (c *Config) IncludedPluginConfigs() []*plugin.Config {
 	return configs
 }
 
-func (c *Config) Packages() []configfile.Package {
+// Returns all packages including those from included plugins.
+// If includeRemovedTriggerPackages is true, then trigger packages that have
+// been removed will also be returned. These are only used for built-ins
+// (e.g. php) when the plugin creates a flake that is meant to replace the
+// original package.
+func (c *Config) Packages(
+	includeRemovedTriggerPackages bool,
+) []configfile.Package {
 	packages := []configfile.Package{}
 	packagesToRemove := map[string]bool{}
 
 	for _, i := range c.included {
-		packages = append(packages, i.Packages()...)
-		if i.pluginData.RemoveTriggerPackage {
+		packages = append(packages, i.Packages(includeRemovedTriggerPackages)...)
+		if i.pluginData.RemoveTriggerPackage && !includeRemovedTriggerPackages {
 			packagesToRemove[i.pluginData.Source.LockfileKey()] = true
 		}
 	}
@@ -219,19 +226,6 @@ func (c *Config) Packages() []configfile.Package {
 	))
 }
 
-// PackagesVersionedNames returns a list of package names with versions.
-// NOTE: if the package is unversioned, the version will be omitted (doesn't default to @latest).
-//
-// example:
-// ["package1", "package2@latest", "[email protected]"]
-func (c *Config) PackagesVersionedNames() []string {
-	result := make([]string, 0, len(c.Root.TopLevelPackages()))
-	for _, p := range c.Root.TopLevelPackages() {
-		result = append(result, p.VersionedName())
-	}
-	return result
-}
-
 func (c *Config) NixPkgsCommitHash() string {
 	return c.Root.NixPkgsCommitHash()
 }

internal/lock/interfaces.go

@@ -6,7 +6,7 @@ package lock
 type devboxProject interface {
 	ConfigHash() (string, error)
 	NixPkgsCommitHash() string
-	PackageNames() []string
+	AllPackageNamesIncludingRemovedTriggerPackages() []string
 	ProjectDir() string
 }
 

internal/lock/lockfile.go

@@ -173,7 +173,10 @@ func IsLegacyPackage(pkg string) bool {
 // Tidy ensures that the lockfile has the set of packages corresponding to the devbox.json config.
 // It gets rid of older packages that are no longer needed.
 func (f *File) Tidy() {
-	f.Packages = lo.PickByKeys(f.Packages, f.devboxProject.PackageNames())
+	f.Packages = lo.PickByKeys(
+		f.Packages,
+		f.devboxProject.AllPackageNamesIncludingRemovedTriggerPackages(),
+	)
 }
 
 // IsUpToDateAndInstalled returns true if the lockfile is up to date and the

internal/plugin/manager.go

@@ -14,7 +14,7 @@ type Manager struct {
 }
 
 type devboxProject interface {
-	PackageNames() []string
+	AllPackageNamesIncludingRemovedTriggerPackages() []string
 	ProjectDir() string
 }