Skip to content

Conversation

@edolstra
Copy link
Collaborator

@edolstra edolstra commented Dec 17, 2025

Motivation

This makes fetchTree substitutable, which is good for flake-compat. It also means that Git fetches will be implicitly shallow (since we don't need to get a revCount).

Alternative to NixOS#14634.

Context

Summary by CodeRabbit

  • Breaking Changes

    • Removed the legacy final-tree public API; use the standard tree fetch interface instead.
  • Improvements

    • Finality is now inferred from presence of a NAR hash rather than an explicit flag, simplifying tree resolution.
  • Tests

    • Updated assertions for final behavior and NAR-hash error messages; removed an older test block that verified non-final substitution behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 17, 2025

Walkthrough

Removed the fetchFinalTree public API and the isFinal parameter; final-tree marking is now determined solely by presence of a NAR hash, with corresponding updates to primops, flake evaluation call sites, and tests.

Changes

Cohort / File(s) Summary
Primop: fetchTree final API removal
src/libexpr/primops/fetchTree.cc
Deleted prim_fetchFinalTree and its RegisterPrimOp; removed bool isFinal from FetchTreeParams; replaced explicit final-flag guard with final detection driven by presence of a narHash (sets __final when present) and removed else-final guard logic.
Flake evaluation changes
src/libflake/call-flake.nix, src/libflake/flake.cc, src/libflake/include/nix/flake/flake.hh
Removed local fetchTreeFinal binding and retrieval/use of internal prim_fetchFinalTree; call-flake pathways no longer accept or pass a fetchFinalTree primitive and now rely on builtins.fetchTree; header declaration of prim_fetchFinalTree removed.
Tests: assertions and removals
tests/functional/tarball.sh, tests/nixos/github-flakes.nix, tests/nixos/fetchers-substitute.nix
Added assertion that fetchTree with a narHash is final and does not expose lastModified; adjusted expected error wording for incorrect NAR hash; removed a test block in fetchers-substitute.nix that verified non-substitution behavior of fetchTree (test and exit-code assertion deleted).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

  • Inspect src/libexpr/primops/fetchTree.cc for correctness of the new final-flagging via narHash and any remaining references to isFinal.
  • Verify call sites in src/libflake/flake.cc and src/libflake/call-flake.nix no longer expect or pass the removed primitive.
  • Check header cleanup in src/libflake/include/nix/flake/flake.hh for matching declarations.
  • Confirm tests reflect runtime behavior and error-message wording changes.

Suggested reviewers

  • grahamc
  • cole-h

Poem

🐰 A NAR hash gleamed beneath the log,

"I'll tuck the flag inside my cog."
No extra gate, no dangling string,
The rabbit hops — a cleaner thing. 🥕

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 accurately reflects the main change: removing explicit fetchFinalTree API and making final-flagging implicit based on narHash presence.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eelcodolstra/fh-1214-make-fetchtree-final

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36c91b0 and b378e48.

📒 Files selected for processing (1)
  • tests/nixos/fetchers-substitute.nix (0 hunks)
💤 Files with no reviewable changes (1)
  • tests/nixos/fetchers-substitute.nix
⏰ 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 / manual
  • GitHub Check: build_aarch64-darwin / test

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

@github-actions
Copy link

github-actions bot commented Dec 17, 2025

@github-actions github-actions bot temporarily deployed to pull request December 17, 2025 18:13 Inactive
This makes fetchTree substitutable, which is good for flake-compat. It
also means that Git fetches will be implicitly shallow (since we don't
need to get a revCount).
@edolstra edolstra force-pushed the eelcodolstra/fh-1214-make-fetchtree-final branch from d3e9920 to 36c91b0 Compare December 17, 2025 19:45
@edolstra edolstra closed this Dec 17, 2025
@edolstra edolstra added the flake-regression-test Run the flake regressions test suite on this PR label Dec 17, 2025
@edolstra edolstra reopened this Dec 17, 2025
@github-actions github-actions bot temporarily deployed to pull request December 17, 2025 19:47 Inactive
@github-actions github-actions bot temporarily deployed to pull request December 17, 2025 19:49 Inactive
@edolstra edolstra added this pull request to the merge queue Dec 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2025
@edolstra edolstra enabled auto-merge December 19, 2025 17:22
@github-actions github-actions bot temporarily deployed to pull request December 19, 2025 17:30 Inactive
@edolstra edolstra added this pull request to the merge queue Dec 19, 2025
Merged via the queue into main with commit 8a4f12e Dec 19, 2025
42 checks passed
@edolstra edolstra deleted the eelcodolstra/fh-1214-make-fetchtree-final branch December 19, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flake-regression-test Run the flake regressions test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants