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" } }	}

Author: gcurtis

.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: