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..d7f06bc408f --- /dev/null +++ b/internal/devpkg/storepaths.go @@ -0,0 +1,226 @@ +// 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() + + query := storePathQuery[*Package]{ + resolved: map[*Package][]string{}, + installables: map[*Package][]string{}, + pathInfo: nix.StorePathsFromInstallables, + inStore: nix.StorePathsAreInStore, + single: func(ctx context.Context, pkg *Package) ([]string, error) { + return pkg.storePathsFromInstallables(ctx) + }, + } + for _, pkg := range pkgs { + paths, err := pkg.GetResolvedStorePaths() + if err != nil { + return nil, err + } + if len(paths) > 0 { + query.resolved[pkg] = paths + continue + } + if featureflag.TidyWarning.Enabled() && pkg.IsDevboxPackage { + // No fast path, we need to query nix. + ux.FHidableWarning(ctx, w, MissingStorePathsWarning, pkg.Raw) + } + installables, err := pkg.Installables() + if err != nil { + return nil, err + } + query.installables[pkg] = installables + if pkg.HasAllowInsecure() { + query.insecure = append(query.insecure, pkg) + } else { + query.secure = append(query.secure, pkg) + } + } + return query.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) +} + +// 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. + allPaths := []string{} + 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 _, batch := range batches { + if batch.ok { + allPaths = append(allPaths, batch.paths...) + continue + } + if err := q.querySingles(ctx, batch.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 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 + } + } + } + + missing := []K{} + for _, item := range items { + paths, ok := q.resolved[item] + if !ok { + paths, ok = itemPaths[item] + } + // 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) + if err != nil { + return err + } + itemPaths[item] = paths + *allPaths = append(*allPaths, paths...) + } + return nil +} + +func allInStore(paths []string, inStore map[string]bool) bool { + for _, path := range paths { + if !inStore[path] { + return false + } + } + return true +} diff --git a/internal/devpkg/storepaths_test.go b/internal/devpkg/storepaths_test.go new file mode 100644 index 00000000000..212a0550cc8 --- /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 (fake *fakeNix) query(installables, 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) { + fake.pathInfoArgs = append(fake.pathInfoArgs, inst) + paths := []string{} + for _, i := range inst { + if fake.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] = fake.inStore[p] + } + return m, nil + }, + single: func(_ context.Context, item string) ([]string, error) { + fake.singleCalls = append(fake.singleCalls, item) + paths := []string{} + for _, i := range installables[item] { + if fake.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) { + 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, + }} + 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(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(fake.singleCalls) != 0 { + t.Errorf("got single calls %v, want none", fake.singleCalls) + } +} + +func TestNotInStoreMissingPathIsAttributed(t *testing.T) { + fake := &fakeNix{inStore: map[string]bool{ + "/nix/store/a-out": true, // a-man is missing + "/nix/store/b-out": true, + // resolved r is missing + }} + 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) + } + 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(fake.pathInfoArgs) != 1 { + t.Errorf("got path-info calls %v, want 1", fake.pathInfoArgs) + } + 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) { + 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(fake.singleCalls, want) { + t.Errorf("got single calls %v, want %v", fake.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") + } +} 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.