devpkg: auto-patch python (#2250)

Add a `devbox add --patch <mode>` flag that replaces `--patch-glibc` and
a corresponding `patch` JSON field that replaces `patch_glibc`. The new
name reflects the new patching behavior, which affects more than just
glibc.

The new patch flag/field is a string instead of a bool. Valid values are
`auto`, `always` and `never`. The default is `auto`.

	devbox add --patch <auto/always/never>

- `auto` - let Devbox decide if a package should be patched. Currently
  only enables patching for Python or if `patch_glibc` is true in the
  config.
- `always` - always attempt to patch a package. Corresponds to the
  `--patch-glibc=true` behavior.
- `never` - never patch a package, even if `auto` would.

If a config has an existing package with the `"patch_glibc": true`
field, it's interpreted as `"patch": "always"` but the config itself
isn't modified. However, if the user runs a command that does write to
the config, then `patch_glibc` will be migrated to `patch`.

Example `devbox.json`:

	{
	  "packages": {
	    "ruby": {
	      "version": "latest",
	      "patch":   "always"
	    },
	    "python": {
	      "version": "latest"
	
	      // No patch field implies "auto".
	      // "patch": "auto"
	    }
	  }
	}

.golangci.yml

@@ -70,6 +70,7 @@ linters-settings:
       - m map[string]int
       - n int
       - ns string
+      - ok bool
       - r *http.Request
       - r io.Reader
       - r *os.File

internal/boxcli/add.go

@@ -24,6 +24,7 @@ type addCmdFlags struct {
 	platforms        []string
 	excludePlatforms []string
 	patchGlibc       bool
+	patch            string
 	outputs          []string
 }
 
@@ -68,10 +69,16 @@ func addCmd() *cobra.Command {
 	command.Flags().BoolVar(
 		&flags.patchGlibc, "patch-glibc", false,
 		"patch any ELF binaries to use the latest glibc version in nixpkgs")
+	command.Flags().StringVar(
+		&flags.patch, "patch", "auto",
+		"allow Devbox to patch the package to fix known issues (auto, always, never)")
 	command.Flags().StringSliceVarP(
 		&flags.outputs, "outputs", "o", []string{},
 		"specify the outputs to select for the nix package")
 
+	_ = command.Flags().MarkDeprecated("patch-glibc", `use --patch=always instead`)
+	command.MarkFlagsMutuallyExclusive("patch", "patch-glibc")
+
 	return command
 }
 
@@ -85,12 +92,17 @@ func addCmdFunc(cmd *cobra.Command, args []string, flags addCmdFlags) error {
 		return errors.WithStack(err)
 	}
 
-	return box.Add(cmd.Context(), args, devopt.AddOpts{
+	opts := devopt.AddOpts{
 		AllowInsecure:    flags.allowInsecure,
 		DisablePlugin:    flags.disablePlugin,
 		Platforms:        flags.platforms,
 		ExcludePlatforms: flags.excludePlatforms,
-		PatchGlibc:       flags.patchGlibc,
+		Patch:            flags.patch,
 		Outputs:          flags.outputs,
-	})
+	}
+	if flags.patchGlibc {
+		// Backwards compatibility so --patch-glibc still works.
+		opts.Patch = "always"
+	}
+	return box.Add(cmd.Context(), args, opts)
 }

internal/devbox/devopt/devboxopts.go

@@ -51,7 +51,7 @@ type AddOpts struct {
 	Platforms        []string
 	ExcludePlatforms []string
 	DisablePlugin    bool
-	PatchGlibc       bool
+	Patch            string
 	Outputs          []string
 }
 

internal/devbox/packages.go

@@ -142,9 +142,11 @@ func (d *Devbox) setPackageOptions(pkgs []string, opts devopt.AddOpts) error {
 			pkg, opts.DisablePlugin); err != nil {
 			return err
 		}
-		if err := d.cfg.PackageMutator().SetPatchGLibc(
-			pkg, opts.PatchGlibc); err != nil {
-			return err
+		if opts.Patch != "" {
+			if err := d.cfg.PackageMutator().SetPatch(
+				pkg, configfile.PatchMode(opts.Patch)); err != nil {
+				return err
+			}
 		}
 		if err := d.cfg.PackageMutator().SetOutputs(
 			d.stderr, pkg, opts.Outputs); err != nil {

internal/devconfig/configfile/ast.go

@@ -173,7 +173,7 @@ func (c *configAST) removePackageElement(arr *hujson.Array, name string) {
 
 // setPackageBool sets a bool field on a package.
 func (c *configAST) setPackageBool(name, fieldName string, val bool) {
-	pkgObject := c.FindPkgObject(name)
+	pkgObject := c.findPkgObject(name)
 	if pkgObject == nil {
 		return
 	}
@@ -216,7 +216,72 @@ func (c *configAST) appendAllowInsecure(name, fieldName string, whitelist []stri
 	c.appendStringSliceField(name, fieldName, whitelist)
 }
 
-func (c *configAST) FindPkgObject(name string) *hujson.Object {
+// removePatch removes the patch field from the named package.
+func (c *configAST) removePatch(name string) {
+	pkgs := c.packagesField(false)
+	obj, ok := pkgs.Value.Value.(*hujson.Object)
+	if !ok {
+		// Packages field is an array.
+		return
+	}
+	i := c.memberIndex(obj, name)
+	if i == -1 {
+		// Package not found.
+		return
+	}
+
+	obj, ok = obj.Members[i].Value.Value.(*hujson.Object)
+	if !ok {
+		// Package is a string, not an object.
+		return
+	}
+	i = c.memberIndex(obj, "patch")
+	if i == -1 {
+		// Patch field doesn't exist.
+		return
+	}
+
+	obj.Members = slices.Delete(obj.Members, i, i+1)
+	c.root.Format()
+}
+
+// setPatch sets the patch field of the named package.
+func (c *configAST) setPatch(name string, mode PatchMode) {
+	pkgObject := c.findPkgObject(name)
+	if pkgObject == nil {
+		return
+	}
+
+	glibcIndex := c.memberIndex(pkgObject, "patch_glibc") // deprecated
+	patchIndex := c.memberIndex(pkgObject, "patch")
+	switch {
+	// Neither patch_glibc or patch exist - append a new field.
+	case patchIndex == -1 && glibcIndex == -1:
+		pkgObject.Members = append(pkgObject.Members, hujson.ObjectMember{
+			Name: hujson.Value{
+				BeforeExtra: []byte{'\n'},
+			},
+		})
+		patchIndex = len(pkgObject.Members) - 1
+		defer c.root.Format()
+	// patch_glibc exists and patch doesn't - rename patch_glibc to
+	// preserve formatting/comments.
+	case patchIndex == -1 && glibcIndex != -1:
+		patchIndex = glibcIndex
+	// Both patch_glibc and patch exist - delete patch_glibc.
+	case patchIndex != -1 && glibcIndex != -1:
+		pkgObject.Members = slices.Delete(pkgObject.Members, glibcIndex, glibcIndex+1)
+		if patchIndex > glibcIndex {
+			patchIndex--
+		}
+		defer c.root.Format()
+	}
+
+	pkgObject.Members[patchIndex].Name.Value = hujson.String("patch")
+	pkgObject.Members[patchIndex].Value.Value = hujson.String(string(mode))
+}
+
+func (c *configAST) findPkgObject(name string) *hujson.Object {
 	pkgs := c.packagesField(true).Value.Value.(*hujson.Object)
 	i := c.memberIndex(pkgs, name)
 	if i == -1 {
@@ -301,7 +366,7 @@ func joinNameVersion(name, version string) string {
 }
 
 func (c *configAST) appendStringSliceField(name, fieldName string, fieldValues []string) {
-	pkgObject := c.FindPkgObject(name)
+	pkgObject := c.findPkgObject(name)
 	if pkgObject == nil {
 		return
 	}

internal/devconfig/configfile/packages.go

@@ -3,6 +3,7 @@ package configfile
 
 import (
 	"encoding/json"
+	"fmt"
 	"io"
 	"slices"
 	"strings"
@@ -154,15 +155,24 @@ func (pkgs *PackagesMutator) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
-func (pkgs *PackagesMutator) SetPatchGLibc(versionedName string, v bool) error {
+func (pkgs *PackagesMutator) SetPatch(versionedName string, mode PatchMode) error {
+	if err := mode.validate(); err != nil {
+		return fmt.Errorf("set patch field for %s: %v", versionedName, err)
+	}
+
 	name, version := parseVersionedName(versionedName)
 	i := pkgs.index(name, version)
 	if i == -1 {
 		return errors.Errorf("package %s not found", versionedName)
 	}
-	if pkgs.collection[i].PatchGlibc != v {
-		pkgs.collection[i].PatchGlibc = v
-		pkgs.ast.setPackageBool(name, "patch_glibc", v)
+
+	pkgs.collection[i].PatchGlibc = false
+	pkgs.collection[i].Patch = mode
+	if mode == PatchAuto {
+		// PatchAuto is the default behavior, so just remove the field.
+		pkgs.ast.removePatch(name)
+	} else {
+		pkgs.ast.setPatch(name, mode)
 	}
 	return nil
 }
@@ -231,6 +241,34 @@ func (pkgs *PackagesMutator) index(name, version string) int {
 	})
 }
 
+// PatchMode specifies when to patch packages.
+type PatchMode string
+
+const (
+	// PatchAuto automatically applies patches to fix known issues with
+	// certain packages. It is the default behavior when the config doesn't
+	// specify a patching mode.
+	PatchAuto PatchMode = "auto"
+
+	// PatchAlways always applies patches to a package, overriding the
+	// default behavior of PatchAuto. It might cause problems with untested
+	// packages.
+	PatchAlways PatchMode = "always"
+
+	// PatchNever disables all patching for a package.
+	PatchNever PatchMode = "never"
+)
+
+func (p PatchMode) validate() error {
+	switch p {
+	case PatchAuto, PatchAlways, PatchNever:
+		return nil
+	default:
+		return fmt.Errorf("invalid patch mode %q (must be %s, %s or %s)",
+			p, PatchAuto, PatchAlways, PatchNever)
+	}
+}
+
 type Package struct {
 	Name    string
 	Version string `json:"version,omitempty"`
@@ -241,8 +279,14 @@ type Package struct {
 
 	// PatchGlibc applies a function to the package's derivation that
 	// patches any ELF binaries to use the latest version of nixpkgs#glibc.
+	//
+	// Deprecated: Use Patch instead, which also patches glibc.
 	PatchGlibc bool `json:"patch_glibc,omitempty"`
 
+	// Patch controls when to patch the package. If empty, it defaults to
+	// [PatchAuto].
+	Patch PatchMode `json:"patch,omitempty"`
+
 	// Outputs is the list of outputs to use for this package, assuming
 	// it is a nix package. If empty, the default output is used.
 	Outputs []string `json:"outputs,omitempty"`
@@ -291,21 +335,28 @@ func (p *Package) VersionedName() string {
 
 func (p *Package) UnmarshalJSON(data []byte) error {
 	// First, attempt to unmarshal as a version-only string
-	var version string
-	if err := json.Unmarshal(data, &version); err == nil {
-		p.Version = version
-		return nil
-	}
-
-	// Second, attempt to unmarshal as a Package struct
-	type packageAlias Package // Use an alias-type to avoid infinite recursion
-	alias := &packageAlias{}
-	if err := json.Unmarshal(data, alias); err != nil {
-		return errors.WithStack(err)
+	if err := json.Unmarshal(data, &p.Version); err != nil {
+		// Second, attempt to unmarshal as a Package struct
+		type packageAlias Package // Use an alias-type to avoid infinite recursion
+		alias := &packageAlias{}
+		if err := json.Unmarshal(data, alias); err != nil {
+			return errors.WithStack(err)
+		}
+		*p = Package(*alias)
+	}
+
+	if p.Patch == "" {
+		if p.PatchGlibc {
+			// Force patching if the user has an old config with the deprecated
+			// patch_glibc field set to true.
+			p.Patch = PatchAlways
+		} else {
+			// Default to PatchAuto if the field is missing, null,
+			// or empty.
+			p.Patch = PatchAuto
+		}
 	}
-
-	*p = Package(*alias)
-	return nil
+	return p.Patch.validate()
 }
 
 // parseVersionedName parses the name and version from package@version representation

internal/devpkg/package.go

@@ -81,10 +81,10 @@ type Package struct {
 	// Outputs is a list of outputs to build from the package's derivation.
 	outputs outputs
 
-	// patchGlibc applies a function to the package's derivation that
+	// patch applies a function to the package's derivation that
 	// patches any ELF binaries to use the latest version of nixpkgs#glibc.
 	// It's a function to allow deferring nix System call until it's needed.
-	patchGlibc func() bool
+	patch func() bool
 
 	// AllowInsecure are a list of nix packages that are whitelisted to be
 	// installed even if they are marked as insecure.
@@ -110,9 +110,7 @@ func PackagesFromConfig(packages []configfile.Package, l lock.Locker) []*Package
 	for _, cfgPkg := range packages {
 		pkg := newPackage(cfgPkg.VersionedName(), cfgPkg.IsEnabledOnPlatform, l)
 		pkg.DisablePlugin = cfgPkg.DisablePlugin
-		pkg.patchGlibc = sync.OnceValue(func() bool {
-			return cfgPkg.PatchGlibc && nix.SystemIsLinux()
-		})
+		pkg.patch = patchGlibcFunc(pkg.CanonicalName(), cfgPkg.Patch)
 		pkg.outputs.selectedNames = lo.Uniq(append(pkg.outputs.selectedNames, cfgPkg.Outputs...))
 		pkg.AllowInsecure = cfgPkg.AllowInsecure
 		result = append(result, pkg)
@@ -127,7 +125,7 @@ func PackageFromStringWithDefaults(raw string, locker lock.Locker) *Package {
 func PackageFromStringWithOptions(raw string, locker lock.Locker, opts devopt.AddOpts) *Package {
 	pkg := PackageFromStringWithDefaults(raw, locker)
 	pkg.DisablePlugin = opts.DisablePlugin
-	pkg.patchGlibc = sync.OnceValue(func() bool { return opts.PatchGlibc })
+	pkg.patch = patchGlibcFunc(pkg.CanonicalName(), configfile.PatchMode(opts.Patch))
 	pkg.outputs.selectedNames = lo.Uniq(append(pkg.outputs.selectedNames, opts.Outputs...))
 	pkg.AllowInsecure = opts.AllowInsecure
 	return pkg
@@ -179,6 +177,22 @@ func resolve(pkg *Package) error {
 	return nil
 }
 
+func patchGlibcFunc(canonicalName string, mode configfile.PatchMode) func() bool {
+	return sync.OnceValue(func() (patch bool) {
+		switch mode {
+		case configfile.PatchAuto:
+			patch = canonicalName == "python"
+		case configfile.PatchAlways:
+			patch = true
+		case configfile.PatchNever:
+			patch = false
+		}
+
+		// Check nix.SystemIsLinux() last because it's slow.
+		return patch && nix.SystemIsLinux()
+	})
+}
+
 func (p *Package) setInstallable(i flake.Installable, projectDir string) {
 	if i.Ref.Type == flake.TypePath && !filepath.IsAbs(i.Ref.Path) {
 		i.Ref.Path = filepath.Join(projectDir, i.Ref.Path)
@@ -238,7 +252,7 @@ func (p *Package) IsInstallable() bool {
 }
 
 func (p *Package) PatchGlibc() bool {
-	return p.patchGlibc != nil && p.patchGlibc()
+	return p.patch != nil && p.patch()
 }
 
 // Installables for this package. Installables is a nix concept defined here: