Skip to content

Conversation

@edolstra
Copy link
Collaborator

@edolstra edolstra commented Nov 14, 2025

Motivation

Build-time inputs can already be fetched in parallel, so prefetching them is usually not what you want.

Context

Summary by CodeRabbit

  • New Features

    • The flake prefetch-inputs command now skips inputs marked with buildTime = true.
  • Documentation

    • Updated documentation to clarify that flake prefetch-inputs recursively fetches transitive inputs and explicitly skips build-time inputs.
  • Tests

    • Added test coverage to verify build-time inputs are properly skipped during prefetch operation.

@coderabbitai
Copy link

coderabbitai bot commented Nov 14, 2025

Walkthrough

The changes add support for skipping build-time inputs in the flake prefetch-inputs command. This includes a guard in the implementation that returns early when buildTime is set, updated documentation explaining recursive behavior and build-time input filtering, and test coverage verifying the feature works correctly.

Changes

Cohort / File(s) Summary
Implementation
src/nix/flake-prefetch-inputs.cc
Adds guard in visit function to return early and skip fetchToStore when lockedNode->buildTime is truthy, preventing fetching of build-time inputs
Documentation
src/nix/flake-prefetch-inputs.md
Clarifies recursive nature of operation and adds explanation that build-time inputs (marked with buildTime = true) are skipped
Test Coverage
tests/functional/flakes/build-time-flake-inputs.sh
Adds comprehensive test to verify prefetch-inputs ignores build-time inputs, including dependency flake setup, lockfile generation, cache clearing, and verification

Sequence Diagram

sequenceDiagram
    participant prefetch as prefetch-inputs
    participant visitor as LockedNode Visitor
    participant fetcher as fetchToStore
    
    prefetch->>visitor: visit(lockedNode)
    alt buildTime is set
        visitor->>visitor: early return
        Note over visitor: Skip this input
    else buildTime not set
        visitor->>fetcher: fetchToStore(...)
        fetcher-->>visitor: fetched
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Implementation requires careful verification of the guard logic and understanding of buildTime semantics
  • Test setup involves git repo manipulation and multiple execution steps to verify correct behavior
  • Documentation changes are straightforward but should be validated against implementation

Suggested reviewers

  • cole-h

Poem

🐰 Build-time inputs, we skip with grace,
No fetching fuss in the prefetch race,
A guard stands tall, returns right quick,
When buildTime is true—that's the trick!
Tests verify this dance so bright,
Our flakes now prefetch just right! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: making nix flake prefetch-inputs skip build-time inputs, which aligns with the modifications across all three files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eelcodolstra/fh-994-make-nix-flake-prefetch-inputs-ignore-build-time-inputs

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

@github-actions github-actions bot temporarily deployed to pull request November 14, 2025 11:57 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/nix/flake-prefetch-inputs.cc (1)

47-48: Consider adding debug logging for skipped build-time inputs.

For better observability, consider logging when build-time inputs are skipped. This would help users understand why certain inputs are not being prefetched.

 if (lockedNode->buildTime)
+    logger->cout(lvlDebug, fmt("skipping build-time input '%s'", lockedNode->lockedRef));
     return;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef054dc and dc4f411.

📒 Files selected for processing (3)
  • src/nix/flake-prefetch-inputs.cc (1 hunks)
  • src/nix/flake-prefetch-inputs.md (1 hunks)
  • tests/functional/flakes/build-time-flake-inputs.sh (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/functional/flakes/build-time-flake-inputs.sh (1)
tests/functional/flakes/common.sh (2)
  • createGitRepo (127-136)
  • createSimpleGitFlake (42-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_aarch64-darwin / build
  • GitHub Check: build_x86_64-linux / build
🔇 Additional comments (2)
src/nix/flake-prefetch-inputs.cc (1)

47-48: LGTM! The guard correctly skips build-time inputs.

The early return ensures build-time inputs are not prefetched, which aligns with the PR objective. The placement after the done check and before the fetchToStore call is appropriate.

src/nix/flake-prefetch-inputs.md (1)

15-15: LGTM! Documentation accurately describes the new behavior.

The documentation clearly explains that the operation is recursive and that build-time inputs (with buildTime = true) are skipped. This aligns with the implementation changes.

@edolstra edolstra enabled auto-merge November 15, 2025 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants