devpkg: auto-patch python #2250

gcurtis · 2024-09-04T22:35:44Z

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

gcurtis · 2024-09-04T22:39:34Z

internal/devpkg/package.go

@@ -238,6 +238,12 @@ func (p *Package) IsInstallable() bool {
 }

 func (p *Package) PatchGlibc() bool {
+	if p.CanonicalName() == "python" {
+		if p.patchGlibc != nil && !p.patchGlibc() {


I think this actually still needs a tweak to distinguish between missing and false.

Yeah, looks like function returns false in both cases https://github.com/jetify-com/devbox/blob/main/internal/devpkg/package.go#L113C1-L115C5

Lagoja · 2024-09-04T22:39:39Z

Should we rename this option from patch_glibc?
Would enabling this option for other non-Python packages have a similar effect?

mikeland73 · 2024-09-04T23:55:08Z

Should we add a feature flag for the opt out? Or just go for it?

mikeland73

Approve pending opt-out option.

I think we should either feature flag it, or make sure the opt out works before releasing.

Lagoja · 2024-09-05T02:20:12Z

I would suggest that we do a beta release, test it a bunch internally + with the community, and then release. I think an opt-in/flag is too hard for users to discover and activate. I would be open to an opt-out, maybe with a differently named flag.

gcurtis · 2024-09-05T14:29:26Z

@Lagoja

Should we rename this option from patch_glibc?

That's probably a good idea since it does a bit more now. What about autopatch or just patch? I'll leave patch_glibc in as an alias for backwards compatibility.

Would enabling this option for other non-Python packages have a similar effect?

I haven't tested it on any other packages, but the patching logic is pretty general. It'll look for any ELF binaries in the package's bin directory, update glibc, and convert RUNPATH to RPATH on all of them.

The stuff with restoring build inputs is specific to Python and will just be a no-op on other packages.

Lagoja · 2024-09-05T16:27:42Z

Prefer autopatch for resemblance to autopatchElf

gcurtis · 2024-09-06T19:05:56Z

@savil @mikeland73 I made some pretty substantial changes if you want to take another look. I switched things over to use a new --patch auto/always/never flag instead.

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`: ```json5 { "packages": { "ruby": { "version": "latest", "patch": "always" } "python": { "version": "latest" // No patch field implies "auto". // "patch": "auto" } } } ```

mikeland73

LGTM

in a follow up could we add a warning if patch_glibc is set in config?

patch_glibc is deprecated and will be removed in a future version. Use patch: always instead.

mikeland73 · 2024-09-06T22:00:04Z

internal/devconfig/configfile/ast.go

@@ -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) {


Could we generalize these similar to setPackageBool ?

removePackageField for removing.
setPackageString for adding.

setPatch could wrap these (remove patch_glibc add patch) or you could not special case at all and have the code in packages.go do that logic). That keeps the ast more logic agnostic.

Honestly, you could also not remove patch_glibc. It doesn't really do harm and would allow us to have simpler code.

Making them more general sounds good to me. I'd like to do in a follow-up PR though just to get the patching stuff out the door.

gcurtis requested review from savil and mikeland73 September 4, 2024 22:36

gcurtis commented Sep 4, 2024

View reviewed changes

savil approved these changes Sep 4, 2024

View reviewed changes

mikeland73 approved these changes Sep 5, 2024

View reviewed changes

gcurtis force-pushed the gcurtis/patchpkg-auto branch from 465f12b to f7bb72b Compare September 6, 2024 19:01

gcurtis requested review from mikeland73 and savil September 6, 2024 19:06

gcurtis force-pushed the gcurtis/patchpkg-auto branch from f7bb72b to ae97e3f Compare September 6, 2024 19:21

mikeland73 approved these changes Sep 6, 2024

View reviewed changes

gcurtis merged commit 74f4a2a into main Sep 6, 2024
24 checks passed

gcurtis deleted the gcurtis/patchpkg-auto branch September 6, 2024 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devpkg: auto-patch python #2250

devpkg: auto-patch python #2250

gcurtis commented Sep 4, 2024 •

edited

Loading

gcurtis Sep 4, 2024

mikeland73 Sep 4, 2024

mikeland73 Sep 5, 2024

Lagoja commented Sep 4, 2024 •

edited

Loading

mikeland73 commented Sep 4, 2024

mikeland73 left a comment

Lagoja commented Sep 5, 2024

gcurtis commented Sep 5, 2024

Lagoja commented Sep 5, 2024

gcurtis commented Sep 6, 2024

mikeland73 left a comment

mikeland73 Sep 6, 2024

gcurtis Sep 6, 2024

devpkg: auto-patch python #2250

devpkg: auto-patch python #2250

Conversation

gcurtis commented Sep 4, 2024 • edited Loading

gcurtis Sep 4, 2024

Choose a reason for hiding this comment

mikeland73 Sep 4, 2024

Choose a reason for hiding this comment

mikeland73 Sep 5, 2024

Choose a reason for hiding this comment

Lagoja commented Sep 4, 2024 • edited Loading

mikeland73 commented Sep 4, 2024

mikeland73 left a comment

Choose a reason for hiding this comment

Lagoja commented Sep 5, 2024

gcurtis commented Sep 5, 2024

Lagoja commented Sep 5, 2024

gcurtis commented Sep 6, 2024

mikeland73 left a comment

Choose a reason for hiding this comment

mikeland73 Sep 6, 2024

Choose a reason for hiding this comment

gcurtis Sep 6, 2024

Choose a reason for hiding this comment

gcurtis commented Sep 4, 2024 •

edited

Loading

Lagoja commented Sep 4, 2024 •

edited

Loading