Add openbsd-x64 CI leg#130761
Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "c6da547f9fefd1d3541ccddd46c1c6ef0bcb24bc",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "d03f6b9564cc85413a7cfaa000faeabb82abea47",
"last_reviewed_commit": "c6da547f9fefd1d3541ccddd46c1c6ef0bcb24bc",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "d03f6b9564cc85413a7cfaa000faeabb82abea47",
"last_recorded_worker_run_id": "29694242585",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "3e7bfec4003db43e4e044d216cbecfb4ef6c534e",
"review_id": 4730632528
},
{
"commit": "c6da547f9fefd1d3541ccddd46c1c6ef0bcb24bc",
"review_id": 4731077949
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. The PR contributes to #124911 (OpenBSD support) by adding an openbsd-x64 cross-build CI leg. The underlying build infrastructure (osGroup openbsd, openbsd-x64 RID, rootfs, init-os-and-arch.sh/init-distro-rid.sh) already recognizes OpenBSD, so wiring up a CI leg to keep the port building is a reasonable next step.
Approach: The pipeline plumbing mirrors the existing FreeBSD cross-build leg almost exactly — a platform-matrix.yml block gated on containsValue(platforms, 'openbsd_x64'), a cross container image in pipeline-with-resources.yml, and adding openbsd_x64 to the existing CoreCLR_Bootstrapped platform list. This is the established pattern for build-only (non-Helix) cross platforms, and crossBuild: true with no helix queue mirrors FreeBSD correctly. One collateral concern: the same commit also removes the sccache pre/post build steps from the whole bootstrapped leg.
Summary: CoreCLR_Bootstrapped leg (commit titled "Try removing sccache"), which also affects the pre-existing freebsd/riscv64/loongarch64 platforms and appears exploratory. A maintainer should confirm whether that removal is intentional and in-scope for this PR. See the inline comment on eng/pipelines/runtime.yml.
Detailed Findings
✅ Pipeline plumbing — Mirrors the FreeBSD leg correctly
The new platform-matrix.yml OpenBSD block is a faithful copy of the FreeBSD block (osGroup/archType/targetRid/platform/container all consistent, crossBuild: true, no helix queue). The pipeline-with-resources.yml container entry follows the same shape as the other cross images, pinned by digest. Adding openbsd_x64 to the CoreCLR_Bootstrapped platform list matches how freebsd_x64 is handled. No issues here.
⚠️ Scope — sccache removal affects more than OpenBSD
The removed preBuildSteps/postBuildSteps sccache templates applied to the entire bootstrapped leg, so this also disables sccache for freebsd_x64, linux_riscv64, and linux_loongarch64. setup-sccache.yml already guards on osGroup (linux/freebsd only), so it would no-op for OpenBSD without removal. The removal therefore looks unrelated to adding OpenBSD and, combined with the "Try removing sccache" commit message, appears exploratory. Flagged inline; a human should confirm intent.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 62.8 AIC · ⌖ 10.6 AIC · ⊞ 10K
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. The PR contributes to #124911 (OpenBSD support) by adding an openbsd-x64 cross-build CI leg. The underlying build infrastructure already recognizes OpenBSD, so wiring up a CI leg is a reasonable next step. The motivation is unchanged from the prior review.
Approach: The core OpenBSD plumbing (a platform-matrix.yml block, a cross container image in pipeline-with-resources.yml, and adding openbsd_x64 to the CoreCLR_Bootstrapped platform list) mirrors the existing FreeBSD cross-build leg and remains correct. Since the prior review, the new commit "Testing size issue" replaces the previously-removed sccache pre/post build steps with temporary disk-space diagnostic scripts (df -h, du snapshots) on the shared bootstrapped leg. This is exploratory CI instrumentation layered on top of the OpenBSD work.
Summary: CoreCLR_Bootstrapped leg (affecting the pre-existing freebsd/riscv64/loongarch64 platforms as well), which appears to be debugging scaffolding for a container disk-space issue rather than part of adding OpenBSD. These scripts, along with the still-removed sccache caching, should be revisited before merge. See the inline comment on eng/pipelines/runtime.yml.
Assessment History
- review 4730632528 reviewed commit
3e7bfecwith verdict⚠️ Needs Human Review. Current verdict:⚠️ Needs Human Review. Assessment changed in emphasis: the prior concern was the removal of sccache from the shared bootstrapped leg; the new commitfd58018("Testing size issue") adds temporary disk-space diagnostic steps in place of the sccache templates, so the exploratory/out-of-scope concern now centers on that instrumentation as well as the lingering sccache removal. Overall motivation, approach, and risk are otherwise unchanged.
Detailed Findings
⚠️ Scope — Temporary disk-space diagnostics added to the shared bootstrapped leg
The new commit adds preBuildSteps/postBuildSteps that dump df -h and du snapshots on the whole CoreCLR_Bootstrapped leg (freebsd_x64, linux_riscv64, linux_loongarch64, openbsd_x64). This is CI debugging scaffolding, out of scope for adding OpenBSD, and should be removed before merge; sccache caching for those platforms should likely be restored. Flagged inline on eng/pipelines/runtime.yml.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 59.6 AIC · ⌖ 14.9 AIC · ⊞ 10K
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. The PR contributes to #124911 (OpenBSD support) by adding an openbsd-x64 cross-build CI leg. The underlying build infrastructure already recognizes OpenBSD, so wiring up a CI leg to keep the port building is a reasonable next step. Motivation is unchanged from prior reviews.
Approach: The PR has been squashed into a single focused commit and the exploratory scaffolding flagged earlier has been removed. The core OpenBSD plumbing still mirrors the FreeBSD cross-build leg: a platform-matrix.yml block gated on containsValue(platforms, 'openbsd_x64') with crossBuild: true and no helix queue, a digest-pinned cross container image in pipeline-with-resources.yml, and openbsd_x64 added to the CoreCLR_Bootstrapped platform list. The incremental changes since the last review add openbsd alongside freebsd in the places that need it — the agentOs: OpenBSD mapping in xplat-job.yml, the public/official Linux build-pool conditions in xplat-setup.yml, and the sccache setup/stats osGroup guards. The previously-removed sccache steps and the temporary df -h/du disk-space diagnostics are gone; sccache is now correctly restored and extended to cover OpenBSD.
Summary: ✅ LGTM. The incremental changes resolve both concerns raised in the prior two reviews (the exploratory sccache removal and the temporary disk-space diagnostics). The PR is now a clean, self-consistent addition of an OpenBSD x64 cross-build leg that faithfully follows the established FreeBSD pattern across all the pipeline templates. No actionable findings.
Assessment History
- review 4730632528 reviewed commit
3e7bfecwith verdict⚠️ Needs Human Review. Current verdict: ✅ LGTM. Assessment changed: that review flagged the bundled removal of sccache from the sharedCoreCLR_Bootstrappedleg. The squashed PR headc6da547restores sccache and instead extends its osGroup guards to includeopenbsd, so the out-of-scope concern no longer applies. - review 4730930456 reviewed commit
59dbe26with verdict⚠️ Needs Human Review. Current verdict: ✅ LGTM. Assessment changed: that review flagged temporarydf -h/dudisk-space diagnostic steps added to the shared bootstrapped leg. Those steps have been removed in the squashed headc6da547, leaving only the in-scope OpenBSD plumbing.
Detailed Findings
No actionable findings. The changes since the previous review are limited to (1) restoring sccache and adding openbsd to its osGroup guards in setup-sccache.yml/sccache-stats.yml, (2) adding the agentOs: OpenBSD mapping in xplat-job.yml, and (3) adding openbsd to the Linux build-pool conditions in xplat-setup.yml. Each mirrors the existing freebsd handling and is consistent with the rest of the OpenBSD leg.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 57.8 AIC · ⌖ 16.3 AIC · ⊞ 10K
|
/ba-g unrelated test failures on other platforms |
Contributes to #124911.