From 4689db175c7aafe2b7f13f7cdcfe5ceb98060786 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:27:12 -0800 Subject: [PATCH 1/4] perf(install): batch nix path-info and prefetch nixpkgs once Check which packages need installing with one `nix path-info` call per allow_insecure setting instead of one per installable. path-info output can't be mapped back to installables, so a batch is retried one package at a time only if it fails or reports a missing path; that keeps the per-package error messages. Run `nix flake prefetch --json` once and read the store path from its output instead of running prefetch a second time. Co-Authored-By: Claude Opus 5.5 --- internal/devbox/packages.go | 23 +--- internal/devpkg/package.go | 21 +-- internal/devpkg/storepaths.go | 213 +++++++++++++++++++++++++++++ internal/devpkg/storepaths_test.go | 122 +++++++++++++++++ internal/nix/nixpkgs.go | 54 ++++---- internal/nix/store.go | 17 ++- internal/nix/store_test.go | 17 +++ 7 files changed, 407 insertions(+), 60 deletions(-) create mode 100644 internal/devpkg/storepaths.go create mode 100644 internal/devpkg/storepaths_test.go diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 3cc331e55d2..5aed59a71ac 100644 --- a/internal/devbox/packages.go +++ b/internal/devbox/packages.go @@ -674,33 +674,20 @@ func (d *Devbox) packagesToInstallInStore(ctx context.Context, mode installMode) // Second, check which packages are not in the nix store packagesToInstall := []*devpkg.Package{} - storePathsForPackage := map[*devpkg.Package][]string{} + packagesToCheck := []*devpkg.Package{} for _, pkg := range packages { if mode == update && d.isBeingUpdated(pkg) { packagesToInstall = append(packagesToInstall, pkg) - continue - } - var err error - storePathsForPackage[pkg], err = pkg.GetStorePaths(ctx, d.stderr) - if err != nil { - return nil, err + } else { + packagesToCheck = append(packagesToCheck, pkg) } } - // Batch this for perf - storePathMap, err := nix.StorePathsAreInStore(ctx, lo.Flatten(lo.Values(storePathsForPackage))) + notInStore, err := devpkg.PackagesNotInStore(ctx, d.stderr, packagesToCheck) if err != nil { return nil, err } - - for pkg, storePaths := range storePathsForPackage { - for _, storePath := range storePaths { - if !storePathMap[storePath] { - packagesToInstall = append(packagesToInstall, pkg) - break - } - } - } + packagesToInstall = append(packagesToInstall, notInStore...) return lo.Uniq(packagesToInstall), nil } diff --git a/internal/devpkg/package.go b/internal/devpkg/package.go index 7944c8dca00..2b624e2fbf9 100644 --- a/internal/devpkg/package.go +++ b/internal/devpkg/package.go @@ -16,7 +16,6 @@ import ( "github.com/pkg/errors" "github.com/samber/lo" - "go.jetify.com/devbox/internal/boxcli/featureflag" "go.jetify.com/devbox/internal/boxcli/usererr" "go.jetify.com/devbox/internal/cachehash" "go.jetify.com/devbox/internal/debug" @@ -26,7 +25,6 @@ import ( "go.jetify.com/devbox/internal/lock" "go.jetify.com/devbox/internal/nix" "go.jetify.com/devbox/internal/searcher" - "go.jetify.com/devbox/internal/ux" "go.jetify.com/devbox/nix/flake" "go.jetify.com/devbox/plugins" ) @@ -772,21 +770,14 @@ func (p *Package) GetResolvedStorePaths() ([]string, error) { const MissingStorePathsWarning = "Outputs for %s are not in lockfile. To fix this issue and improve performance, please run " + "`devbox install --tidy-lockfile`\n" -func (p *Package) GetStorePaths(ctx context.Context, w io.Writer) ([]string, error) { - storePathsForPackage, err := p.GetResolvedStorePaths() - if err != nil || len(storePathsForPackage) > 0 { - return storePathsForPackage, err - } - - if featureflag.TidyWarning.Enabled() && p.IsDevboxPackage { - // No fast path, we need to query nix. - ux.FHidableWarning(ctx, w, MissingStorePathsWarning, p.Raw) - } - +// storePathsFromInstallables queries nix for the store paths of p's +// installables, one installable at a time so that errors can be attributed. +func (p *Package) storePathsFromInstallables(ctx context.Context) ([]string, error) { installables, err := p.Installables() if err != nil { return nil, err } + storePaths := []string{} for _, installable := range installables { storePathsForInstallable, err := nix.StorePathsFromInstallable( ctx, installable, p.HasAllowInsecure(), @@ -794,9 +785,9 @@ func (p *Package) GetStorePaths(ctx context.Context, w io.Writer) ([]string, err if err != nil { return nil, packageInstallErrorHandler(err, p, installable) } - storePathsForPackage = append(storePathsForPackage, storePathsForInstallable...) + storePaths = append(storePaths, storePathsForInstallable...) } - return storePathsForPackage, nil + return storePaths, nil } // packageInstallErrorHandler checks for two kinds of errors to print custom messages for so that Devbox users diff --git a/internal/devpkg/storepaths.go b/internal/devpkg/storepaths.go new file mode 100644 index 00000000000..3b1c5695537 --- /dev/null +++ b/internal/devpkg/storepaths.go @@ -0,0 +1,213 @@ +// Copyright 2024 Jetify Inc. and contributors. All rights reserved. +// Use of this source code is governed by the license in the LICENSE file. + +package devpkg + +import ( + "context" + "io" + + "github.com/samber/lo" + + "go.jetify.com/devbox/internal/boxcli/featureflag" + "go.jetify.com/devbox/internal/debug" + "go.jetify.com/devbox/internal/nix" + "go.jetify.com/devbox/internal/ux" +) + +// PackagesNotInStore returns the packages that have at least one output that +// isn't in the local nix store, in the order they appear in pkgs. +// +// Packages with store paths in the lockfile are checked directly. Others +// need `nix path-info` to evaluate their installables; those are batched into +// a single call per allow_insecure setting instead of one call per +// installable. +func PackagesNotInStore(ctx context.Context, w io.Writer, pkgs []*Package) ([]*Package, error) { + defer debug.FunctionTimer().End() + + q := storePathQuery[*Package]{ + resolved: map[*Package][]string{}, + installables: map[*Package][]string{}, + pathInfo: nix.StorePathsFromInstallables, + inStore: nix.StorePathsAreInStore, + single: func(ctx context.Context, p *Package) ([]string, error) { + return p.storePathsFromInstallables(ctx) + }, + } + for _, p := range pkgs { + paths, err := p.GetResolvedStorePaths() + if err != nil { + return nil, err + } + if len(paths) > 0 { + q.resolved[p] = paths + continue + } + if featureflag.TidyWarning.Enabled() && p.IsDevboxPackage { + // No fast path, we need to query nix. + ux.FHidableWarning(ctx, w, MissingStorePathsWarning, p.Raw) + } + installables, err := p.Installables() + if err != nil { + return nil, err + } + q.installables[p] = installables + if p.HasAllowInsecure() { + q.insecure = append(q.insecure, p) + } else { + q.secure = append(q.secure, p) + } + } + return q.notInStore(ctx, pkgs) +} + +// storePathQuery figures out which items (packages) are missing store paths. +// It's generic over the item type so the batching logic can be tested +// without nix. +type storePathQuery[K comparable] struct { + // resolved holds items whose store paths are already known. + resolved map[K][]string + + // secure and insecure hold the remaining items, split by whether nix + // should allow insecure packages when evaluating them. + secure, insecure []K + + // installables holds the installables of each item in secure/insecure. + installables map[K][]string + + // pathInfo returns the union of store paths for some installables. + pathInfo func(ctx context.Context, installables []string, allowInsecure bool) ([]string, error) + + // inStore reports which store paths are in the nix store. + inStore func(ctx context.Context, paths []string) (map[string]bool, error) + + // single returns the store paths for one item, attributing any error to + // that item. + single func(ctx context.Context, item K) ([]string, error) +} + +func (q *storePathQuery[K]) notInStore(ctx context.Context, items []K) ([]K, error) { + // `nix path-info` doesn't say which installable produced which path, so + // a batch result can't be split back up per item. That's fine as long as + // every path is in the store: then none of the batch's items need to be + // installed. A batch that fails or has a missing path gets retried one + // item at a time so the error or missing path is attributed correctly. + type batch struct { + items []K + paths []string + ok bool + } + batches := []*batch{} + allPaths := []string{} + for _, b := range q.resolved { + allPaths = append(allPaths, b...) + } + for _, group := range []struct { + items []K + allowInsecure bool + }{{q.secure, false}, {q.insecure, true}} { + if len(group.items) == 0 { + continue + } + installables := []string{} + for _, item := range group.items { + installables = append(installables, q.installables[item]...) + } + b := &batch{items: group.items} + batches = append(batches, b) + + // A single item doesn't need a batch call; retrying it would just + // repeat the same command. + if len(group.items) == 1 { + continue + } + paths, err := q.pathInfo(ctx, installables, group.allowInsecure) + if err != nil { + continue + } + b.paths, b.ok = paths, true + allPaths = append(allPaths, paths...) + } + + // Individually query items from batches that failed or were skipped. + itemPaths := map[K][]string{} + for _, b := range batches { + if b.ok { + continue + } + if err := q.querySingles(ctx, b.items, itemPaths, &allPaths); err != nil { + return nil, err + } + } + + inStore, err := q.inStore(ctx, lo.Uniq(allPaths)) + if err != nil { + return nil, err + } + + // Batches with a missing path need to be split up to find out which + // items it belongs to. + for _, b := range batches { + if b.ok && !allInStore(b.paths, inStore) { + newPaths := []string{} + if err := q.querySingles(ctx, b.items, itemPaths, &newPaths); err != nil { + return nil, err + } + // Single-item paths are a subset of the batch's paths, so + // we already know whether they're in the store. Query any + // stragglers just in case. + unknown := []string{} + for _, p := range newPaths { + if _, ok := inStore[p]; !ok { + unknown = append(unknown, p) + } + } + if len(unknown) > 0 { + more, err := q.inStore(ctx, unknown) + if err != nil { + return nil, err + } + for p, v := range more { + inStore[p] = v + } + } + } + } + + missing := []K{} + for _, item := range items { + paths, ok := q.resolved[item] + if !ok { + paths, ok = itemPaths[item] + } + if !ok { + // Part of a batch where everything is in the store. + continue + } + if !allInStore(paths, inStore) { + missing = append(missing, item) + } + } + return missing, nil +} + +func (q *storePathQuery[K]) querySingles(ctx context.Context, items []K, itemPaths map[K][]string, allPaths *[]string) error { + for _, item := range items { + paths, err := q.single(ctx, item) + if err != nil { + return err + } + itemPaths[item] = paths + *allPaths = append(*allPaths, paths...) + } + return nil +} + +func allInStore(paths []string, inStore map[string]bool) bool { + for _, p := range paths { + if !inStore[p] { + return false + } + } + return true +} diff --git a/internal/devpkg/storepaths_test.go b/internal/devpkg/storepaths_test.go new file mode 100644 index 00000000000..73bddf651bc --- /dev/null +++ b/internal/devpkg/storepaths_test.go @@ -0,0 +1,122 @@ +// Copyright 2024 Jetify Inc. and contributors. All rights reserved. +// Use of this source code is governed by the license in the LICENSE file. + +package devpkg + +import ( + "context" + "errors" + "slices" + "strings" + "testing" +) + +// fakeNix simulates `nix path-info` for installables of the form +// "#", which produce the store path "/nix/store/-". +type fakeNix struct { + inStore map[string]bool + failing map[string]bool // installables that fail to evaluate + pathInfoArgs [][]string + singleCalls []string +} + +func (f *fakeNix) query(installables map[string][]string, resolved map[string][]string, secure, insecure []string) *storePathQuery[string] { + return &storePathQuery[string]{ + resolved: resolved, + secure: secure, + insecure: insecure, + installables: installables, + pathInfo: func(_ context.Context, inst []string, _ bool) ([]string, error) { + f.pathInfoArgs = append(f.pathInfoArgs, inst) + paths := []string{} + for _, i := range inst { + if f.failing[i] { + return nil, errors.New("eval failed: " + i) + } + paths = append(paths, "/nix/store/"+strings.ReplaceAll(i, "#", "-")) + } + return paths, nil + }, + inStore: func(_ context.Context, paths []string) (map[string]bool, error) { + m := map[string]bool{} + for _, p := range paths { + m[p] = f.inStore[p] + } + return m, nil + }, + single: func(_ context.Context, item string) ([]string, error) { + f.singleCalls = append(f.singleCalls, item) + paths := []string{} + for _, i := range installables[item] { + if f.failing[i] { + return nil, errors.New("error installing package " + item) + } + paths = append(paths, "/nix/store/"+strings.ReplaceAll(i, "#", "-")) + } + return paths, nil + }, + } +} + +var testInstallables = map[string][]string{ + "a": {"a#out", "a#man"}, + "b": {"b#out"}, + "c": {"c#out"}, +} + +func TestNotInStoreAllInstalledUsesOneBatch(t *testing.T) { + f := &fakeNix{inStore: map[string]bool{ + "/nix/store/a-out": true, "/nix/store/a-man": true, + "/nix/store/b-out": true, "/nix/store/c-out": true, + "/nix/store/resolved": true, + }} + q := f.query(testInstallables, map[string][]string{"r": {"/nix/store/resolved"}}, []string{"a", "b", "c"}, nil) + missing, err := q.notInStore(context.Background(), []string{"r", "a", "b", "c"}) + if err != nil { + t.Fatal(err) + } + if len(missing) != 0 { + t.Errorf("got missing %v, want none", missing) + } + if len(f.pathInfoArgs) != 1 || len(f.pathInfoArgs[0]) != 4 { + t.Errorf("got path-info calls %v, want one call with all 4 installables", f.pathInfoArgs) + } + if len(f.singleCalls) != 0 { + t.Errorf("got single calls %v, want none", f.singleCalls) + } +} + +func TestNotInStoreMissingPathIsAttributed(t *testing.T) { + f := &fakeNix{inStore: map[string]bool{ + "/nix/store/a-out": true, // a-man is missing + "/nix/store/b-out": true, + // resolved r is missing + }} + q := f.query(testInstallables, map[string][]string{"r": {"/nix/store/resolved"}}, []string{"a", "b"}, []string{"c"}) + missing, err := q.notInStore(context.Background(), []string{"r", "a", "b", "c"}) + if err != nil { + t.Fatal(err) + } + if want := []string{"r", "a", "c"}; !slices.Equal(missing, want) { + t.Errorf("got missing %v, want %v", missing, want) + } + // The single insecure item skips the batch call. + if len(f.pathInfoArgs) != 1 { + t.Errorf("got path-info calls %v, want 1", f.pathInfoArgs) + } + if want := []string{"c", "a", "b"}; !slices.Equal(f.singleCalls, want) { + t.Errorf("got single calls %v, want %v", f.singleCalls, want) + } +} + +func TestNotInStoreBatchErrorIsAttributed(t *testing.T) { + f := &fakeNix{failing: map[string]bool{"b#out": true}} + q := f.query(testInstallables, nil, []string{"a", "b", "c"}, nil) + _, err := q.notInStore(context.Background(), []string{"a", "b", "c"}) + if err == nil || err.Error() != "error installing package b" { + t.Errorf("got error %v, want error attributed to package b", err) + } + if want := []string{"a", "b"}; !slices.Equal(f.singleCalls, want) { + t.Errorf("got single calls %v, want %v", f.singleCalls, want) + } +} diff --git a/internal/nix/nixpkgs.go b/internal/nix/nixpkgs.go index b1e1553a10f..104547d6a95 100644 --- a/internal/nix/nixpkgs.go +++ b/internal/nix/nixpkgs.go @@ -38,13 +38,15 @@ func EnsureNixpkgsPrefetched(w io.Writer, commit string) error { } fmt.Fprintf(w, "Ensuring nixpkgs registry is downloaded.\n") + // --json makes nix print the store location on stdout, so we don't need + // a second prefetch to look it up. Progress still goes to stderr. cmd := Command( - "flake", "prefetch", + "flake", "prefetch", "--json", FlakeNixpkgs(commit), ) - cmd.Stdout = w - cmd.Stderr = cmd.Stdout - if err := cmd.Run(context.TODO()); err != nil { + cmd.Stderr = w + out, err := cmd.Output(context.TODO()) + if err != nil { fmt.Fprintf(w, "Ensuring nixpkgs registry is downloaded: ") color.New(color.FgRed).Fprintf(w, "Fail\n") return err @@ -52,7 +54,26 @@ func EnsureNixpkgsPrefetched(w io.Writer, commit string) error { fmt.Fprintf(w, "Ensuring nixpkgs registry is downloaded: ") color.New(color.FgGreen).Fprintf(w, "Success\n") - return saveToNixpkgsCommitFile(commit, commitToLocation) + storePath, err := parsePrefetchStorePath(out) + if err != nil { + return err + } + return saveToNixpkgsCommitFile(commit, storePath, commitToLocation) +} + +// parsePrefetchStorePath extracts the store path from the output of +// `nix flake prefetch --json`. +func parsePrefetchStorePath(out []byte) (string, error) { + var prefetchData struct { + StorePath string `json:"storePath"` + } + if err := json.Unmarshal(out, &prefetchData); err != nil { + return "", errors.WithStack(err) + } + if prefetchData.StorePath == "" { + return "", errors.Errorf("nix flake prefetch output has no storePath: %s", out) + } + return prefetchData.StorePath, nil } func nixpkgsCommitFileContents() (map[string]string, error) { @@ -70,33 +91,16 @@ func nixpkgsCommitFileContents() (map[string]string, error) { return commitToLocation, errors.WithStack(json.Unmarshal(contents, &commitToLocation)) } -func saveToNixpkgsCommitFile(commit string, commitToLocation map[string]string) error { - // Make a query to get the /nix/store path for this commit hash. - cmd := Command("flake", "prefetch", "--json", - FlakeNixpkgs(commit), - ) - out, err := cmd.Output(context.TODO()) - if err != nil { - return errors.WithStack(err) - } - - // read the json response - var prefetchData struct { - StorePath string `json:"storePath"` - } - if err := json.Unmarshal(out, &prefetchData); err != nil { - return errors.WithStack(err) - } - +func saveToNixpkgsCommitFile(commit, storePath string, commitToLocation map[string]string) error { // Ensure the nixpkgs commit file path exists so we can write an update to it path := nixpkgsCommitFilePath() - err = os.MkdirAll(filepath.Dir(path), 0o755) + err := os.MkdirAll(filepath.Dir(path), 0o755) if err != nil && !errors.Is(err, fs.ErrExist) { return errors.WithStack(err) } // write to the map, jsonify it, and write that json to the nixpkgsCommit file - commitToLocation[commit] = prefetchData.StorePath + commitToLocation[commit] = storePath serialized, err := json.Marshal(commitToLocation) if err != nil { return errors.WithStack(err) diff --git a/internal/nix/store.go b/internal/nix/store.go index 72a28f85130..03555b7aee8 100644 --- a/internal/nix/store.go +++ b/internal/nix/store.go @@ -26,10 +26,23 @@ func StorePathFromHashPart(ctx context.Context, hash, storeAddr string) (string, } func StorePathsFromInstallable(ctx context.Context, installable string, allowInsecure bool) ([]string, error) { + return StorePathsFromInstallables(ctx, []string{installable}, allowInsecure) +} + +// StorePathsFromInstallables runs a single `nix path-info` for all +// installables and returns the union of their store paths. The output doesn't +// say which installable produced which path. +func StorePathsFromInstallables(ctx context.Context, installables []string, allowInsecure bool) ([]string, error) { defer debug.FunctionTimer().End() + if len(installables) == 0 { + return nil, nil + } // --impure for NIXPKGS_ALLOW_UNFREE - cmd := Command("path-info", FixInstallableArg(installable), "--json", "--impure") + cmd := Command("path-info", "--json", "--impure") + for _, installable := range installables { + cmd.Args = append(cmd.Args, FixInstallableArg(installable)) + } cmd.Env = allowUnfreeEnv(os.Environ()) if allowInsecure { @@ -44,7 +57,7 @@ func StorePathsFromInstallable(ctx context.Context, installable string, allowIns paths, err := parseStorePathFromInstallableOutput(resultBytes) if err != nil { - return nil, fmt.Errorf("failed to parse path-info for %s: %w", installable, err) + return nil, fmt.Errorf("failed to parse path-info for %s: %w", strings.Join(installables, " "), err) } return maps.Keys(paths), nil diff --git a/internal/nix/store_test.go b/internal/nix/store_test.go index 4fed441f9d2..6a8b02aab55 100644 --- a/internal/nix/store_test.go +++ b/internal/nix/store_test.go @@ -56,3 +56,20 @@ func TestParseStorePathFromInstallableOutput(t *testing.T) { }) } } + +func TestParsePrefetchStorePath(t *testing.T) { + out := `{"hash":"sha256-abc=","locked":{"owner":"NixOS","repo":"nixpkgs","type":"github"},"original":{"type":"github"},"storePath":"/nix/store/abc-source"}` + got, err := parsePrefetchStorePath([]byte(out)) + if err != nil { + t.Fatal(err) + } + if want := "/nix/store/abc-source"; got != want { + t.Errorf("got %q, want %q", got, want) + } + if _, err := parsePrefetchStorePath([]byte(`{}`)); err == nil { + t.Error("got nil error for output without storePath") + } + if _, err := parsePrefetchStorePath([]byte(`not json`)); err == nil { + t.Error("got nil error for non-JSON output") + } +} From 60331a4f08dc4826c69e0d3702321d07589ac12a Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:27:21 -0800 Subject: [PATCH 2/4] docs(spec): mark perf items done in #3053 Co-Authored-By: Claude Opus 5.5 --- specs/perf-improvements.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/perf-improvements.md b/specs/perf-improvements.md index 939a43e944d..e52f83f88cc 100644 --- a/specs/perf-improvements.md +++ b/specs/perf-improvements.md @@ -233,7 +233,7 @@ resolve in parallel. Also speeds up `devbox add a b c`. (`internal/devbox/update.go:78`), even when its lockfile entry didn't change. Fix: only pass `--refresh` for packages whose resolved version/hash changed. -### 15. Batch flake `nix path-info` calls +### 15. Batch flake `nix path-info` calls (done in #3053) **Effort:** ~60–100 LOC. @@ -260,7 +260,7 @@ Each ~5–20 LOC: own comment says it probably isn't needed. - Run `nix flake prefetch` once instead of twice (`internal/nix/nixpkgs.go:39-78`): use `--json` on the first call and save - the location from its output. + the location from its output. (done in #3053) - `RunXPaths` (`internal/devbox/devbox.go:1196`) deletes and recreates `.devbox/virtenv/runx/bin` on every env compute. Skip it when there are no runx packages, and otherwise only rewrite changed links. From 91bbb7917d1bcccb9654d30d16cf316762a887ee Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:30:44 -0800 Subject: [PATCH 3/4] fix(devpkg): satisfy golangci-lint in store path batching Split notInStore into helpers to reduce cognitive complexity and use longer variable names. Co-Authored-By: Claude Opus 5.5 --- internal/devpkg/storepaths.go | 175 ++++++++++++++++------------- internal/devpkg/storepaths_test.go | 50 ++++----- 2 files changed, 119 insertions(+), 106 deletions(-) diff --git a/internal/devpkg/storepaths.go b/internal/devpkg/storepaths.go index 3b1c5695537..d7f06bc408f 100644 --- a/internal/devpkg/storepaths.go +++ b/internal/devpkg/storepaths.go @@ -8,7 +8,6 @@ import ( "io" "github.com/samber/lo" - "go.jetify.com/devbox/internal/boxcli/featureflag" "go.jetify.com/devbox/internal/debug" "go.jetify.com/devbox/internal/nix" @@ -25,40 +24,40 @@ import ( func PackagesNotInStore(ctx context.Context, w io.Writer, pkgs []*Package) ([]*Package, error) { defer debug.FunctionTimer().End() - q := storePathQuery[*Package]{ + query := storePathQuery[*Package]{ resolved: map[*Package][]string{}, installables: map[*Package][]string{}, pathInfo: nix.StorePathsFromInstallables, inStore: nix.StorePathsAreInStore, - single: func(ctx context.Context, p *Package) ([]string, error) { - return p.storePathsFromInstallables(ctx) + single: func(ctx context.Context, pkg *Package) ([]string, error) { + return pkg.storePathsFromInstallables(ctx) }, } - for _, p := range pkgs { - paths, err := p.GetResolvedStorePaths() + for _, pkg := range pkgs { + paths, err := pkg.GetResolvedStorePaths() if err != nil { return nil, err } if len(paths) > 0 { - q.resolved[p] = paths + query.resolved[pkg] = paths continue } - if featureflag.TidyWarning.Enabled() && p.IsDevboxPackage { + if featureflag.TidyWarning.Enabled() && pkg.IsDevboxPackage { // No fast path, we need to query nix. - ux.FHidableWarning(ctx, w, MissingStorePathsWarning, p.Raw) + ux.FHidableWarning(ctx, w, MissingStorePathsWarning, pkg.Raw) } - installables, err := p.Installables() + installables, err := pkg.Installables() if err != nil { return nil, err } - q.installables[p] = installables - if p.HasAllowInsecure() { - q.insecure = append(q.insecure, p) + query.installables[pkg] = installables + if pkg.HasAllowInsecure() { + query.insecure = append(query.insecure, pkg) } else { - q.secure = append(q.secure, p) + query.secure = append(query.secure, pkg) } } - return q.notInStore(ctx, pkgs) + return query.notInStore(ctx, pkgs) } // storePathQuery figures out which items (packages) are missing store paths. @@ -86,56 +85,34 @@ type storePathQuery[K comparable] struct { single func(ctx context.Context, item K) ([]string, error) } +// storePathBatch is a group of items whose installables were queried with a +// single `nix path-info` call. +type storePathBatch[K comparable] struct { + items []K + paths []string + ok bool // false if the batch call failed or was skipped +} + func (q *storePathQuery[K]) notInStore(ctx context.Context, items []K) ([]K, error) { // `nix path-info` doesn't say which installable produced which path, so // a batch result can't be split back up per item. That's fine as long as // every path is in the store: then none of the batch's items need to be // installed. A batch that fails or has a missing path gets retried one // item at a time so the error or missing path is attributed correctly. - type batch struct { - items []K - paths []string - ok bool - } - batches := []*batch{} allPaths := []string{} - for _, b := range q.resolved { - allPaths = append(allPaths, b...) - } - for _, group := range []struct { - items []K - allowInsecure bool - }{{q.secure, false}, {q.insecure, true}} { - if len(group.items) == 0 { - continue - } - installables := []string{} - for _, item := range group.items { - installables = append(installables, q.installables[item]...) - } - b := &batch{items: group.items} - batches = append(batches, b) - - // A single item doesn't need a batch call; retrying it would just - // repeat the same command. - if len(group.items) == 1 { - continue - } - paths, err := q.pathInfo(ctx, installables, group.allowInsecure) - if err != nil { - continue - } - b.paths, b.ok = paths, true + for _, paths := range q.resolved { allPaths = append(allPaths, paths...) } + batches := q.runBatches(ctx) // Individually query items from batches that failed or were skipped. itemPaths := map[K][]string{} - for _, b := range batches { - if b.ok { + for _, batch := range batches { + if batch.ok { + allPaths = append(allPaths, batch.paths...) continue } - if err := q.querySingles(ctx, b.items, itemPaths, &allPaths); err != nil { + if err := q.querySingles(ctx, batch.items, itemPaths, &allPaths); err != nil { return nil, err } } @@ -146,31 +123,12 @@ func (q *storePathQuery[K]) notInStore(ctx context.Context, items []K) ([]K, err } // Batches with a missing path need to be split up to find out which - // items it belongs to. - for _, b := range batches { - if b.ok && !allInStore(b.paths, inStore) { - newPaths := []string{} - if err := q.querySingles(ctx, b.items, itemPaths, &newPaths); err != nil { + // items the path belongs to. + for _, batch := range batches { + if batch.ok && !allInStore(batch.paths, inStore) { + if err := q.splitBatch(ctx, batch, itemPaths, inStore); err != nil { return nil, err } - // Single-item paths are a subset of the batch's paths, so - // we already know whether they're in the store. Query any - // stragglers just in case. - unknown := []string{} - for _, p := range newPaths { - if _, ok := inStore[p]; !ok { - unknown = append(unknown, p) - } - } - if len(unknown) > 0 { - more, err := q.inStore(ctx, unknown) - if err != nil { - return nil, err - } - for p, v := range more { - inStore[p] = v - } - } } } @@ -180,17 +138,72 @@ func (q *storePathQuery[K]) notInStore(ctx context.Context, items []K) ([]K, err if !ok { paths, ok = itemPaths[item] } - if !ok { - // Part of a batch where everything is in the store. - continue - } - if !allInStore(paths, inStore) { + // Items with no paths here are part of a batch where everything + // is in the store. + if ok && !allInStore(paths, inStore) { missing = append(missing, item) } } return missing, nil } +// runBatches runs one `nix path-info` per allow_insecure group. +func (q *storePathQuery[K]) runBatches(ctx context.Context) []*storePathBatch[K] { + batches := []*storePathBatch[K]{} + for _, group := range []struct { + items []K + allowInsecure bool + }{{q.secure, false}, {q.insecure, true}} { + if len(group.items) == 0 { + continue + } + batch := &storePathBatch[K]{items: group.items} + batches = append(batches, batch) + + // A single item doesn't need a batch call; retrying it would just + // repeat the same command. + if len(group.items) == 1 { + continue + } + installables := []string{} + for _, item := range group.items { + installables = append(installables, q.installables[item]...) + } + paths, err := q.pathInfo(ctx, installables, group.allowInsecure) + if err == nil { + batch.paths, batch.ok = paths, true + } + } + return batches +} + +// splitBatch queries each item in batch individually, recording their paths +// in itemPaths and adding any paths not yet checked to inStore. +func (q *storePathQuery[K]) splitBatch(ctx context.Context, batch *storePathBatch[K], itemPaths map[K][]string, inStore map[string]bool) error { + newPaths := []string{} + if err := q.querySingles(ctx, batch.items, itemPaths, &newPaths); err != nil { + return err + } + // Single-item paths are a subset of the batch's paths, so we should + // already know whether they're in the store. Query any stragglers just + // in case. + unknown := lo.Filter(newPaths, func(path string, _ int) bool { + _, ok := inStore[path] + return !ok + }) + if len(unknown) == 0 { + return nil + } + more, err := q.inStore(ctx, unknown) + if err != nil { + return err + } + for path, valid := range more { + inStore[path] = valid + } + return nil +} + func (q *storePathQuery[K]) querySingles(ctx context.Context, items []K, itemPaths map[K][]string, allPaths *[]string) error { for _, item := range items { paths, err := q.single(ctx, item) @@ -204,8 +217,8 @@ func (q *storePathQuery[K]) querySingles(ctx context.Context, items []K, itemPat } func allInStore(paths []string, inStore map[string]bool) bool { - for _, p := range paths { - if !inStore[p] { + for _, path := range paths { + if !inStore[path] { return false } } diff --git a/internal/devpkg/storepaths_test.go b/internal/devpkg/storepaths_test.go index 73bddf651bc..9a76d98d4ab 100644 --- a/internal/devpkg/storepaths_test.go +++ b/internal/devpkg/storepaths_test.go @@ -20,17 +20,17 @@ type fakeNix struct { singleCalls []string } -func (f *fakeNix) query(installables map[string][]string, resolved map[string][]string, secure, insecure []string) *storePathQuery[string] { +func (fake *fakeNix) query(installables map[string][]string, resolved map[string][]string, secure, insecure []string) *storePathQuery[string] { return &storePathQuery[string]{ resolved: resolved, secure: secure, insecure: insecure, installables: installables, pathInfo: func(_ context.Context, inst []string, _ bool) ([]string, error) { - f.pathInfoArgs = append(f.pathInfoArgs, inst) + fake.pathInfoArgs = append(fake.pathInfoArgs, inst) paths := []string{} for _, i := range inst { - if f.failing[i] { + if fake.failing[i] { return nil, errors.New("eval failed: " + i) } paths = append(paths, "/nix/store/"+strings.ReplaceAll(i, "#", "-")) @@ -40,15 +40,15 @@ func (f *fakeNix) query(installables map[string][]string, resolved map[string][] inStore: func(_ context.Context, paths []string) (map[string]bool, error) { m := map[string]bool{} for _, p := range paths { - m[p] = f.inStore[p] + m[p] = fake.inStore[p] } return m, nil }, single: func(_ context.Context, item string) ([]string, error) { - f.singleCalls = append(f.singleCalls, item) + fake.singleCalls = append(fake.singleCalls, item) paths := []string{} for _, i := range installables[item] { - if f.failing[i] { + if fake.failing[i] { return nil, errors.New("error installing package " + item) } paths = append(paths, "/nix/store/"+strings.ReplaceAll(i, "#", "-")) @@ -65,35 +65,35 @@ var testInstallables = map[string][]string{ } func TestNotInStoreAllInstalledUsesOneBatch(t *testing.T) { - f := &fakeNix{inStore: map[string]bool{ + fake := &fakeNix{inStore: map[string]bool{ "/nix/store/a-out": true, "/nix/store/a-man": true, "/nix/store/b-out": true, "/nix/store/c-out": true, "/nix/store/resolved": true, }} - q := f.query(testInstallables, map[string][]string{"r": {"/nix/store/resolved"}}, []string{"a", "b", "c"}, nil) - missing, err := q.notInStore(context.Background(), []string{"r", "a", "b", "c"}) + query := fake.query(testInstallables, map[string][]string{"r": {"/nix/store/resolved"}}, []string{"a", "b", "c"}, nil) + missing, err := query.notInStore(context.Background(), []string{"r", "a", "b", "c"}) if err != nil { t.Fatal(err) } if len(missing) != 0 { t.Errorf("got missing %v, want none", missing) } - if len(f.pathInfoArgs) != 1 || len(f.pathInfoArgs[0]) != 4 { - t.Errorf("got path-info calls %v, want one call with all 4 installables", f.pathInfoArgs) + if len(fake.pathInfoArgs) != 1 || len(fake.pathInfoArgs[0]) != 4 { + t.Errorf("got path-info calls %v, want one call with all 4 installables", fake.pathInfoArgs) } - if len(f.singleCalls) != 0 { - t.Errorf("got single calls %v, want none", f.singleCalls) + if len(fake.singleCalls) != 0 { + t.Errorf("got single calls %v, want none", fake.singleCalls) } } func TestNotInStoreMissingPathIsAttributed(t *testing.T) { - f := &fakeNix{inStore: map[string]bool{ + fake := &fakeNix{inStore: map[string]bool{ "/nix/store/a-out": true, // a-man is missing "/nix/store/b-out": true, // resolved r is missing }} - q := f.query(testInstallables, map[string][]string{"r": {"/nix/store/resolved"}}, []string{"a", "b"}, []string{"c"}) - missing, err := q.notInStore(context.Background(), []string{"r", "a", "b", "c"}) + query := fake.query(testInstallables, map[string][]string{"r": {"/nix/store/resolved"}}, []string{"a", "b"}, []string{"c"}) + missing, err := query.notInStore(context.Background(), []string{"r", "a", "b", "c"}) if err != nil { t.Fatal(err) } @@ -101,22 +101,22 @@ func TestNotInStoreMissingPathIsAttributed(t *testing.T) { t.Errorf("got missing %v, want %v", missing, want) } // The single insecure item skips the batch call. - if len(f.pathInfoArgs) != 1 { - t.Errorf("got path-info calls %v, want 1", f.pathInfoArgs) + if len(fake.pathInfoArgs) != 1 { + t.Errorf("got path-info calls %v, want 1", fake.pathInfoArgs) } - if want := []string{"c", "a", "b"}; !slices.Equal(f.singleCalls, want) { - t.Errorf("got single calls %v, want %v", f.singleCalls, want) + if want := []string{"c", "a", "b"}; !slices.Equal(fake.singleCalls, want) { + t.Errorf("got single calls %v, want %v", fake.singleCalls, want) } } func TestNotInStoreBatchErrorIsAttributed(t *testing.T) { - f := &fakeNix{failing: map[string]bool{"b#out": true}} - q := f.query(testInstallables, nil, []string{"a", "b", "c"}, nil) - _, err := q.notInStore(context.Background(), []string{"a", "b", "c"}) + fake := &fakeNix{failing: map[string]bool{"b#out": true}} + query := fake.query(testInstallables, nil, []string{"a", "b", "c"}, nil) + _, err := query.notInStore(context.Background(), []string{"a", "b", "c"}) if err == nil || err.Error() != "error installing package b" { t.Errorf("got error %v, want error attributed to package b", err) } - if want := []string{"a", "b"}; !slices.Equal(f.singleCalls, want) { - t.Errorf("got single calls %v, want %v", f.singleCalls, want) + if want := []string{"a", "b"}; !slices.Equal(fake.singleCalls, want) { + t.Errorf("got single calls %v, want %v", fake.singleCalls, want) } } From d3a6421d7b416b14cd225f3a8cd53e375b3d63b6 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:46:38 -0800 Subject: [PATCH 4/4] style(devpkg): gofumpt -extra storepaths_test.go Co-Authored-By: Claude Opus 5.5 --- internal/devpkg/storepaths_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/devpkg/storepaths_test.go b/internal/devpkg/storepaths_test.go index 9a76d98d4ab..212a0550cc8 100644 --- a/internal/devpkg/storepaths_test.go +++ b/internal/devpkg/storepaths_test.go @@ -20,7 +20,7 @@ type fakeNix struct { singleCalls []string } -func (fake *fakeNix) query(installables map[string][]string, resolved map[string][]string, secure, insecure []string) *storePathQuery[string] { +func (fake *fakeNix) query(installables, resolved map[string][]string, secure, insecure []string) *storePathQuery[string] { return &storePathQuery[string]{ resolved: resolved, secure: secure,