build-trigger: skip aliased derivations #518
Conversation
WalkthroughAdds alias detection to scheduling by tracking seen derivation paths in SchedulingContext (new public field Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Scheduler as _schedule_ready_jobs
participant Ctx as SchedulingContext\n(seen_drvs)
participant Logger
Caller->>Scheduler: invoke scheduling
loop each ready job (ordered by attr)
Scheduler->>Ctx: is drvPath in seen_drvs?
alt drvPath seen (alias)
Scheduler->>Logger: log "alias: <attr> -> <first_attr>"
Scheduler->>Ctx: increment skipped count
note right of Scheduler: job skipped (no schedule)
else drvPath not seen
Scheduler->>Ctx: seen_drvs[drvPath] = attr
Scheduler->>Scheduler: proceed with existing scheduling checks
alt schedulable
Scheduler->>Logger: log scheduled
else not schedulable
Scheduler->>Logger: log skipped_out_path (existing behavior)
end
end
end
sequenceDiagram
autonumber
participant Caller
participant Waiter as wait_for_finish_deferred
participant Logger
Caller->>Waiter: await build completion
alt build cancelled (FirstError->CancelledError)
Waiter->>Logger: log "Build was cancelled"
Waiter-->>Caller: return CANCELLED
else other error
Waiter-->>Caller: re-raise error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Mic92
force-pushed
the
failed-statuses
branch
from
a3d8469 to
f54d0b4
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: 1
🧹 Nitpick comments (1)
buildbot_nix/buildbot_nix/build_trigger.py (1)
534-534: Verify that sorting by attr is intentional for deterministic alias selection.The sort order determines which attribute becomes the "original" and which becomes the "alias." For example, if
a-fooandz-barpoint to the same derivation,a-foowill be built andz-barwill be skipped as an alias. While this ensures deterministic behavior, it may not match user expectations in all cases.Consider documenting this behavior in the code comment or function docstring to clarify the selection criteria.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
buildbot_nix/buildbot_nix/build_trigger.py(3 hunks)
🔇 Additional comments (2)
buildbot_nix/buildbot_nix/build_trigger.py (2)
98-98: LGTM! Clear field addition with helpful comment.The new
seen_drvsfield is well-typed and documented, making the alias detection mechanism transparent.
730-730: LGTM! Proper initialization.The
seen_drvsdictionary is correctly initialized as empty at the start of the scheduling context.
| if job.drvPath in ctx.seen_drvs: | ||
| original_attr = ctx.seen_drvs[job.drvPath] | ||
| ctx.scheduler_log.addStdout( | ||
| f"\t- {job.attr} (skipped, alias of {original_attr})\n" | ||
| ) | ||
| skipped_jobs.append(job) | ||
| self._skipped_count += 1 | ||
| continue | ||
| ctx.seen_drvs[job.drvPath] = job.attr |
There was a problem hiding this comment.
Potential issue: Aliased jobs don't set required output path properties.
When a job is skipped as an alias, schedule_success is never called (due to continue at line 543), so the properties normally set at lines 216-217 ({attr}-out_path, {attr}-drv_path) are never stored in build_props. This differs from the "already built" skip case (lines 560-565), where schedule_success is called first and sets these properties.
If downstream processes (e.g., gcroot registration) expect these properties for all jobs, this could cause issues.
Consider calling schedule_success for aliased jobs or manually setting the required properties before skipping:
for job in sorted(ctx.schedule_now, key=lambda j: j.attr):
# Check if we've already scheduled a build for this derivation (alias detection)
if job.drvPath in ctx.seen_drvs:
original_attr = ctx.seen_drvs[job.drvPath]
+ # Still need to set output path properties for downstream processes
+ source = "nix-eval-nix"
+ out_path = job.outputs["out"] or None
+ build_props.setProperty(f"{job.attr}-out_path", out_path, source)
+ build_props.setProperty(f"{job.attr}-drv_path", job.drvPath, source)
+ if out_path and job.cacheStatus == CacheStatus.local:
+ build_props.setProperty(f"{job.attr}-skipped_out_path", out_path, source)
ctx.scheduler_log.addStdout(
f"\t- {job.attr} (skipped, alias of {original_attr})\n"
)
skipped_jobs.append(job)
self._skipped_count += 1
continue
ctx.seen_drvs[job.drvPath] = job.attr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if job.drvPath in ctx.seen_drvs: | |
| original_attr = ctx.seen_drvs[job.drvPath] | |
| ctx.scheduler_log.addStdout( | |
| f"\t- {job.attr} (skipped, alias of {original_attr})\n" | |
| ) | |
| skipped_jobs.append(job) | |
| self._skipped_count += 1 | |
| continue | |
| ctx.seen_drvs[job.drvPath] = job.attr | |
| if job.drvPath in ctx.seen_drvs: | |
| original_attr = ctx.seen_drvs[job.drvPath] | |
| # Still need to set output path properties for downstream processes | |
| source = "nix-eval-nix" | |
| out_path = job.outputs["out"] or None | |
| build_props.setProperty(f"{job.attr}-out_path", out_path, source) | |
| build_props.setProperty(f"{job.attr}-drv_path", job.drvPath, source) | |
| if out_path and job.cacheStatus == CacheStatus.local: | |
| build_props.setProperty(f"{job.attr}-skipped_out_path", out_path, source) | |
| ctx.scheduler_log.addStdout( | |
| f"\t- {job.attr} (skipped, alias of {original_attr})\n" | |
| ) | |
| skipped_jobs.append(job) | |
| self._skipped_count += 1 | |
| continue | |
| ctx.seen_drvs[job.drvPath] = job.attr |
Summary by CodeRabbit
New Features
Bug Fixes
Chores