From b9c51730591a0f8d7c86dfc16a0a009cb512a74b Mon Sep 17 00:00:00 2001 From: Greg Curtis Date: Tue, 17 Sep 2024 18:30:39 -0400 Subject: [PATCH] devpkg: fix empty patch mode (#2278) Fix a case where an empty patch mode wouldn't default to "auto". This was causing Devbox to skip patching when it should not have. Got rid of the sync.OnceValue function for determining if a package needs to be patched. This was originally done to avoid a call to nix.System(), but that call doesn't happen anymore. --- internal/devpkg/narinfo_cache.go | 4 +-- internal/devpkg/package.go | 50 +++++++++++++++++--------------- internal/shellgen/flake_input.go | 6 ++-- internal/shellgen/flake_plan.go | 2 +- 4 files changed, 32 insertions(+), 30 deletions(-) diff --git a/internal/devpkg/narinfo_cache.go b/internal/devpkg/narinfo_cache.go index b0c4d87591b..e1feee2d0f1 100644 --- a/internal/devpkg/narinfo_cache.go +++ b/internal/devpkg/narinfo_cache.go @@ -219,8 +219,8 @@ func (p *Package) outputsForOutputName(output string) ([]lock.Output, error) { // the package to query it from the binary cache. func (p *Package) isEligibleForBinaryCache() (bool, error) { defer debug.FunctionTimer().End() - // Patched glibc packages are not in the binary cache. - if p.PatchGlibc() { + // Patched packages are not in the binary cache. + if p.Patch { return false, nil } sysInfo, err := p.sysInfoIfExists() diff --git a/internal/devpkg/package.go b/internal/devpkg/package.go index 892569e8dc1..c4341dbff33 100644 --- a/internal/devpkg/package.go +++ b/internal/devpkg/package.go @@ -4,9 +4,11 @@ package devpkg import ( + "cmp" "context" "fmt" "io" + "log/slog" "path/filepath" "regexp" "strings" @@ -81,10 +83,9 @@ type Package struct { // Outputs is a list of outputs to build from the package's derivation. outputs outputs - // 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. - patch func() bool + // Patch controls if Devbox environments will do additional patching to + // address known issues with the package. + Patch bool // AllowInsecure are a list of nix packages that are whitelisted to be // installed even if they are marked as insecure. @@ -110,7 +111,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.patch = patchGlibcFunc(pkg.CanonicalName(), cfgPkg.Patch) + pkg.Patch = pkgNeedsPatch(pkg.CanonicalName(), cfgPkg.Patch) pkg.outputs.selectedNames = lo.Uniq(append(pkg.outputs.selectedNames, cfgPkg.Outputs...)) pkg.AllowInsecure = cfgPkg.AllowInsecure result = append(result, pkg) @@ -125,7 +126,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.patch = patchGlibcFunc(pkg.CanonicalName(), configfile.PatchMode(opts.Patch)) + pkg.Patch = pkgNeedsPatch(pkg.CanonicalName(), configfile.PatchMode(opts.Patch)) pkg.outputs.selectedNames = lo.Uniq(append(pkg.outputs.selectedNames, opts.Outputs...)) pkg.AllowInsecure = opts.AllowInsecure return pkg @@ -155,6 +156,7 @@ func newPackage(raw string, isInstallable func() bool, locker lock.Locker) *Pack pkg.resolve = sync.OnceValue(func() error { return nil }) pkg.setInstallable(parsed, locker.ProjectDir()) pkg.outputs = outputs{selectedNames: strings.Split(parsed.Outputs, ",")} + pkg.Patch = pkgNeedsPatch(pkg.CanonicalName(), configfile.PatchAuto) return pkg } @@ -177,20 +179,6 @@ 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 - } - return patch - }) -} - 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) @@ -198,6 +186,24 @@ func (p *Package) setInstallable(i flake.Installable, projectDir string) { p.installable = i } +func pkgNeedsPatch(canonicalName string, mode configfile.PatchMode) (patch bool) { + mode = cmp.Or(mode, configfile.PatchAuto) + switch mode { + case configfile.PatchAuto: + patch = canonicalName == "python" + case configfile.PatchAlways: + patch = true + case configfile.PatchNever: + patch = false + } + if patch { + slog.Debug("package needs patching", "pkg", canonicalName, "mode", mode) + } else { + slog.Debug("package doesn't need patching", "pkg", canonicalName, "mode", mode) + } + return patch +} + var inputNameRegex = regexp.MustCompile("[^a-zA-Z0-9-]+") // FlakeInputName generates a name for the input that will be used in the @@ -249,10 +255,6 @@ func (p *Package) IsInstallable() bool { return p.isInstallable() } -func (p *Package) PatchGlibc() bool { - return p.patch != nil && p.patch() -} - // Installables for this package. Installables is a nix concept defined here: // https://nixos.org/manual/nix/stable/command-ref/new-cli/nix.html#installables func (p *Package) Installables() ([]string, error) { diff --git a/internal/shellgen/flake_input.go b/internal/shellgen/flake_input.go index 620647433ad..2f96b2bb81c 100644 --- a/internal/shellgen/flake_input.go +++ b/internal/shellgen/flake_input.go @@ -84,7 +84,7 @@ func (f *flakeInput) BuildInputsForSymlinkJoin() ([]*SymlinkJoin, error) { return nil, err } - if pkg.PatchGlibc() { + if pkg.Patch { return nil, errors.New("patch_glibc is not yet supported for packages with non-default outputs") } @@ -125,7 +125,7 @@ func (f *flakeInput) BuildInputs() ([]string, error) { if attributePathErr != nil { err = attributePathErr } - if pkg.PatchGlibc() { + if pkg.Patch { // When the package comes from the glibc flake, the // "legacyPackages" portion of the attribute path // becomes just "packages" (matching the standard flake @@ -179,7 +179,7 @@ func flakeInputs(ctx context.Context, packages []*devpkg.Package) []flakeInput { // Packages that need a glibc patch are assigned to the special // glibc-patched flake input. This input refers to the // glibc-patch.nix flake. - if pkg.PatchGlibc() { + if pkg.Patch { nixpkgsGlibc := flakeInputs.getOrAppend(glibcPatchFlakeRef) nixpkgsGlibc.Name = "glibc-patch" nixpkgsGlibc.URL = glibcPatchFlakeRef diff --git a/internal/shellgen/flake_plan.go b/internal/shellgen/flake_plan.go index 7262a57a0c7..ca529db48de 100644 --- a/internal/shellgen/flake_plan.go +++ b/internal/shellgen/flake_plan.go @@ -106,7 +106,7 @@ func newGlibcPatchFlake(nixpkgsGlibcRev string, packages []*devpkg.Package) (gli NixpkgsGlibcFlakeRef: "flake:nixpkgs/" + nixpkgsGlibcRev, } for _, pkg := range packages { - if !pkg.PatchGlibc() { + if !pkg.Patch { continue }