Don't ignore substitution failures for BasicDerivation inputSrcs#520
Conversation
📝 WalkthroughWalkthroughAdds a ChangespathRequired flag propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)src/libstore/build/derivation-building-goal.cc┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.43][ERROR]: unable to find a config; path src/libstore/build/derivation-goal.cc┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.53][ERROR]: unable to find a config; path src/libstore/build/entry-points.cc┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.47][ERROR]: unable to find a config; path
Comment |
edolstra
force-pushed
the
eelcodolstra/nix-437
branch
from
ee24e7d to
8fa6857
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libstore/build/worker.cc`:
- Around line 109-112: The makePathSubstitutionGoal method caches goals in
substitutionGoals using only the path as the key, but ignores the
substitutionRequired parameter. When a cached goal is reused for the same path
with a different substitutionRequired value, the cached goal's original
requirement is preserved instead of honoring the new requirement. Fix this by
modifying the cache key to include the substitutionRequired parameter when
looking up or storing goals in substitutionGoals, or by validating that the
cached goal's substitutionRequired matches the new request and creating a new
goal if they differ.
In `@src/libstore/include/nix/store/build/worker.hh`:
- Around line 238-241: The function signature has a parameter order mismatch
where bool substitutionRequired precedes RepairFlag repair, causing three call
sites in worker.cc, entry-points.cc, and derivation-goal.cc to pass RepairFlag
values (like Repair or NoRepair) that implicitly convert to bool and bind to the
wrong parameter. Reorder the parameters in the function signature so that
RepairFlag repair comes before bool substitutionRequired, changing the signature
from (storePath, substitutionRequired, repair, ca) to (storePath, repair,
substitutionRequired, ca) or similar order that places the enum before the bool
to prevent implicit conversion and ensure the positional arguments at all three
call sites bind to the correct parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5401703-1577-49ca-a5c5-c86be20e8a49
📒 Files selected for processing (7)
src/libstore/build/derivation-building-goal.ccsrc/libstore/build/derivation-goal.ccsrc/libstore/build/entry-points.ccsrc/libstore/build/substitution-goal.ccsrc/libstore/build/worker.ccsrc/libstore/include/nix/store/build/substitution-goal.hhsrc/libstore/include/nix/store/build/worker.hh
You now get:
error: path '/nix/store/ksv7jr3gl2damnbnn6rcigrzh8wql0rc-missing-dependency' is required, but there is no substituter that can build it
error: Cannot build '/nix/store/vis7l3rjqbb75kimqds2cqbm4wkf3qa2-test.drv'.
Reason: 1 dependency failed.
Output paths:
/nix/store/y600378sad3qj81vn46g1lavwp7scgzl-test
edolstra
force-pushed
the
eelcodolstra/nix-437
branch
from
8fa6857 to
8ca8648
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/libstore/build/worker.cc (1)
109-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCache reuse still drops
pathRequiredsemantics for repeated requests.On Line 112,
substitutionGoals[path]reuses an existing goal regardless of the newpathRequiredargument. That keeps behavior order-dependent: a prior non-required goal can cause later required callers to inheritecNoSubstituterssemantics (or the reverse).Suggested fix
std::shared_ptr<PathSubstitutionGoal> Worker::makePathSubstitutionGoal( const StorePath & path, bool pathRequired, RepairFlag repair, std::optional<ContentAddress> ca) { - return initGoalIfNeeded(substitutionGoals[path], path, *this, pathRequired, repair, ca); + auto & slot = substitutionGoals[path]; + if (auto goal = slot.lock()) { + goal->pathRequired = goal->pathRequired || pathRequired; + return goal; + } + return initGoalIfNeeded(slot, path, *this, pathRequired, repair, ca); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libstore/build/worker.cc` around lines 109 - 112, The makePathSubstitutionGoal method reuses a cached goal from substitutionGoals[path] without verifying that the pathRequired semantics match the current request, causing order-dependent behavior. When retrieving an existing goal via initGoalIfNeeded, check if the cached goal's pathRequired value differs from the incoming pathRequired argument. If they differ, either create a new goal instead of reusing the cached one or update the existing goal to reflect the new pathRequired requirement. This ensures that repeated requests with different pathRequired values are handled consistently rather than inheriting stale semantics from the first caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/libstore/build/worker.cc`:
- Around line 109-112: The makePathSubstitutionGoal method reuses a cached goal
from substitutionGoals[path] without verifying that the pathRequired semantics
match the current request, causing order-dependent behavior. When retrieving an
existing goal via initGoalIfNeeded, check if the cached goal's pathRequired
value differs from the incoming pathRequired argument. If they differ, either
create a new goal instead of reusing the cached one or update the existing goal
to reflect the new pathRequired requirement. This ensures that repeated requests
with different pathRequired values are handled consistently rather than
inheriting stale semantics from the first caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31313c12-0db0-4dcd-8fc5-55a5987b0a73
📒 Files selected for processing (7)
src/libstore/build/derivation-building-goal.ccsrc/libstore/build/derivation-goal.ccsrc/libstore/build/entry-points.ccsrc/libstore/build/substitution-goal.ccsrc/libstore/build/worker.ccsrc/libstore/include/nix/store/build/substitution-goal.hhsrc/libstore/include/nix/store/build/worker.hh
🚧 Files skipped from review as they are similar to previous changes (4)
- src/libstore/build/derivation-building-goal.cc
- src/libstore/include/nix/store/build/worker.hh
- src/libstore/include/nix/store/build/substitution-goal.hh
- src/libstore/build/substitution-goal.cc
| StorePath storePath; | ||
|
|
||
| /** | ||
| * Whether, if there are not substituters, to return ecNoSubstituters or ecFailed. |
There was a problem hiding this comment.
nit:
| * Whether, if there are not substituters, to return ecNoSubstituters or ecFailed. | |
| * Whether, if there are no substituters that have this store path, to return ecNoSubstituters (which means......................) or ecFailed (which means.............). |
follow-on from my previous comment (would be great to describe when ecNoSubs vs ecFailed will be returned and what that actually means/should mean...
There was a problem hiding this comment.
This is documented in PathSubstitutionGoal:
/* Hack: don't indicate failure if there were no substituters.
In that case the calling derivation should just do a
build. */
co_return doneFailure(
substituterFailed || pathRequired ? ecFailed : ecNoSubstituters,
There was a problem hiding this comment.
I definitely saw that comment, but somehow blanked it from my memory within 1 minute of reading it.... Thanks...
Then, nit:
| * Whether, if there are not substituters, to return ecNoSubstituters or ecFailed. | |
| * Whether, if there are no substituters, to return ecNoSubstituters or ecFailed. |
2 participants
Motivation
Previously, substitution failures for the
inputSrcsofBasicDerivationwere silently ignored, leading to inscrutable " 1 dependency failed" errors.You now get:
Context
Summary by CodeRabbit