From ae97e3fe330d7c805917209ca6600ccab9c639c3 Mon Sep 17 00:00:00 2001 From: Greg Curtis Date: Wed, 4 Sep 2024 18:32:36 -0400 Subject: [PATCH] devpkg: add --patch and deprecate --patch-glibc Add a `devbox add --patch ` 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` - 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`: ```json5 { "packages": { "ruby": { "version": "latest", "patch": "always" } "python": { "version": "latest" // No patch field implies "auto". // "patch": "auto" } } } ``` --- .golangci.yml | 1 + internal/boxcli/add.go | 18 ++++- internal/devbox/devopt/devboxopts.go | 2 +- internal/devbox/packages.go | 8 ++- internal/devconfig/configfile/ast.go | 71 +++++++++++++++++- internal/devconfig/configfile/packages.go | 87 ++++++++++++++++++----- internal/devpkg/package.go | 28 ++++++-- 7 files changed, 180 insertions(+), 35 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index bc3e18dcd4a..ea0b02cd7ca 100644 --- a/.golangci.yml +++ b/.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 diff --git a/internal/boxcli/add.go b/internal/boxcli/add.go index 612e04349ba..e62e38f0b2c 100644 --- a/internal/boxcli/add.go +++ b/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) } diff --git a/internal/devbox/devopt/devboxopts.go b/internal/devbox/devopt/devboxopts.go index d5fbf978a8a..8e4ec45c5b4 100644 --- a/internal/devbox/devopt/devboxopts.go +++ b/internal/devbox/devopt/devboxopts.go @@ -51,7 +51,7 @@ type AddOpts struct { Platforms []string ExcludePlatforms []string DisablePlugin bool - PatchGlibc bool + Patch string Outputs []string } diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 8b8355c5559..eb631e26211 100644 --- a/internal/devbox/packages.go +++ b/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 { diff --git a/internal/devconfig/configfile/ast.go b/internal/devconfig/configfile/ast.go index 840504c4bd4..b11df73347a 100644 --- a/internal/devconfig/configfile/ast.go +++ b/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 } diff --git a/internal/devconfig/configfile/packages.go b/internal/devconfig/configfile/packages.go index 92a21ddee3f..76a9af85a6f 100644 --- a/internal/devconfig/configfile/packages.go +++ b/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 diff --git a/internal/devpkg/package.go b/internal/devpkg/package.go index e046520f260..35e7b17a494 100644 --- a/internal/devpkg/package.go +++ b/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: