Auto-derive BootstrapSdkVersion from global.json so it can never lag tools.dotnet#13694
Auto-derive BootstrapSdkVersion from global.json so it can never lag tools.dotnet#13694Copilot wants to merge 13 commits into
Conversation
|
Hello @copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
Agent-Logs-Url: https://github.com/dotnet/msbuild/sessions/8519415e-ce9b-4854-8348-2c215f5d4c4c Co-authored-by: OvesN <150850103+OvesN@users.noreply.github.com>
| <PropertyGroup> | ||
| <BootstrapSdkVersion>10.0.106</BootstrapSdkVersion> | ||
| <!-- Derived from global.json so it can never lag tools.dotnet --> | ||
| <BootstrapSdkVersion>$(DotNetCliVersion)</BootstrapSdkVersion> |
There was a problem hiding this comment.
That's not a standard property. Use NetCoreSdkVersion instead: https://github.com/dotnet/sdk/blob/7bfcbfa069151ac1f7b602fd4cad874f99fb2c1d/src/Layout/redist/targets/GenerateBundledVersions.targets#L654
There was a problem hiding this comment.
What we are missing here is doing some kind of Math.Min('$(NetCoreSdkVersion)', '10.0.106'))
There was a problem hiding this comment.
Yes, I like being able to differ from the initial SDK version. What we should need to guarantee is that we will never construct a bootstrap with a lower .NET runtime version than the SDK delivers (since that version can be embedded into outputs e.g. the apphosts). Unfortunately that's a much harder computation since the runtime can be embedded in different SDKs.
There was a problem hiding this comment.
I fixed it - now we use NetCoreSdkVersion, and we have a new target that checks that we do not use a lower runtime in bootstrap.
| $globalJsonPath = Join-Path $PSScriptRoot "..\global.json" | ||
| $bootstrapSdkVersion = (Get-Content $globalJsonPath -Raw | ConvertFrom-Json).tools.dotnet | ||
| if ([string]::IsNullOrWhiteSpace($bootstrapSdkVersion)) { | ||
| throw "Could not read tools.dotnet from $globalJsonPath." | ||
| } |
There was a problem hiding this comment.
This change seems wrong. It now always uses the SDK version from global.json. It would be better to still allow to hardcode a newer version via the BootstrapSdkVersion property but make sure that is never older than the SDK provided one.
I would use the msbuild -getproperty function here to avoid direct XML parsing and invoke some project that imports the microsoft.net.sdk, i.e. the msbuild.bootstrap.csproj.
This also applies to the bash changes.
There was a problem hiding this comment.
Addressed - we can hardcode a newer version via BootstrapSdkVersionFloor, later we use Max(BootstrapSdkVersionFloor, NETCoreSdkVersion). XML parsing replaced by -getproperty function
| <PropertyGroup> | ||
| <BootstrapSdkVersion>10.0.106</BootstrapSdkVersion> | ||
| <!-- Derived from global.json so it can never lag tools.dotnet --> | ||
| <BootstrapSdkVersion>$(DotNetCliVersion)</BootstrapSdkVersion> |
There was a problem hiding this comment.
Yes, I like being able to differ from the initial SDK version. What we should need to guarantee is that we will never construct a bootstrap with a lower .NET runtime version than the SDK delivers (since that version can be embedded into outputs e.g. the apphosts). Unfortunately that's a much harder computation since the runtime can be embedded in different SDKs.
Reworks the bootstrap SDK derivation based on PR review feedback:
- Restore hardcoded BootstrapSdkVersion floor in eng/Versions.props so
developers can pin a NEWER SDK when needed (per Viktor / Rainer).
- In eng/BootStrapMsBuild.props, override BootstrapSdkVersion to
`\$(NETCoreSdkVersion)` when the SDK actually resolving the project is
newer than the floor. Uses `[MSBuild]::VersionGreaterThan`. The
override runs after Microsoft.NET.Sdk's implicit props import sets
NETCoreSdkVersion. Result: `BootstrapSdkVersion = Max(floor,
NETCoreSdkVersion)` — bootstrap can never be older than what
global.json delivers, but explicit pins of newer SDKs are preserved.
- In cibuild_bootstrapped_msbuild.{ps1,sh}, replace direct global.json
parsing with `dotnet msbuild -getProperty:BootstrapSdkVersion` against
src/MSBuild.Bootstrap/MSBuild.Bootstrap.csproj. This evaluates the SDK
and applies the override, returning the same value the stage1 build
used to lay out `sdk/<version>`. User properties are forwarded so
/p:BootstrapSdkVersion=... overrides remain consistent across stage1
and the version query.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BootstrapSdkVersion selects which SDK is acquired, but the bundled runtime is what actually matters: build outputs (apphosts, runtimeconfig.json) embed the runtime version, so a bootstrap with an older runtime fails host resolution. Because SDK feature bands are serviced independently, a higher SDK version does not guarantee a newer runtime and cannot be predicted cheaply up front. Add VerifyBootstrapRuntimeFloor (AfterTargets=AcquireSdk) which compares the runtime actually laid down under the bootstrap shared/Microsoft.NETCore.App against BundledNETCoreAppPackageVersion (the runtime the resolving SDK delivers) and fails fast if it is older. This closes the SDK-version-vs-runtime-version gap raised in PR review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Covers the bootstrap runtime floor guard in eng/BootStrapMsBuild.targets by importing the real targets file and running the target against synthetic bootstrap shared-runtime layouts: - passes when the laid-down runtime equals/exceeds the floor (incl. numeric-not-lexical comparison and prerelease-suffix insensitivity); - fails with a clear error when the runtime is older than BundledNETCoreAppPackageVersion; - passes when any present runtime satisfies the floor (roll-forward semantics); - passes (no-op) when no bootstrap runtime is present, so the guard never false-positives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review-driven fixes to VerifyBootstrapRuntimeFloor: - Fix a latent crash: when the shared-runtime root exists but contains no version directory, the empty item batch evaluated [MSBuild]::VersionGreaterThanOrEquals on an empty string and threw MSB4184. Guard the comparison with a non-empty identity check (and short-circuit). - Ignore non-version directories under shared/Microsoft.NETCore.App via a regex filter so a stray folder cannot crash the build. - Make the error message accurate (can fail, not will fail; embeds the SDK runtime version) and document two known limitations (stricter-than-roll-forward patch comparison; prerelease-suffix-insensitive comparison shared with the Max override). Tests: strengthen the fail assertion to avoid a 10.0.3 vs 10.0.300 substring trap (use 9.7.2), and add regression tests for unknown floor (no-op), empty-but-present root, only-non-version-directories, and all-runtimes-older. 12/12 pass on net10.0, warning-clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The value in eng/Versions.props is a minimum floor, not the literal bootstrap SDK version, so name it accordingly. eng/BootStrapMsBuild.props now derives the effective BootstrapSdkVersion = Max(BootstrapSdkVersionFloor, NETCoreSdkVersion). Verified: floor<SDK -> SDK wins; floor>SDK -> floor wins; explicit /p:BootstrapSdkVersion still overrides; both props consumers (MSBuild.Bootstrap and Microsoft.Build.UnitTests.Shared) resolve the same value. Also trims now-redundant explanatory comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
Performance review complete for PR #13694. |
|
Design-level notes on PR #13694 🟡 Stale documentation — property rename not reflectedTwo files still reference the old
Both references will mislead the next release engineer (the property no longer exists under that name). They also need a guidance update: under the new model the floor only needs raising when pinning a newer SDK than 🟡 Stale PR descriptionThe description states:
The actual implementation uses Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "dnceng.pkgs.visualstudio.com"
- "pkgs.dev.azure.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security — LGTM
Checked all seven dimensions against the diff:
| Dimension | Finding |
|---|---|
Script injection ($properties unquoted in SH) |
$properties is word-split by bash, but it originates from the Arcade build harness, not external input. No concrete injection path. |
Path traversal (_BootstrapSharedRuntimeRoot) |
Derived from $(ArtifactsBinDir) set by the build system. Used read-only via GetDirectories(); directory names are then regex-filtered (^[0-9]+\.[0-9]+) before any version comparison. No traversal risk. |
SecurityElement.Escape() in test XML |
Escapes XML special chars (&<>"'). Does not escape MSBuild evaluation sequences ($(...)). However, all three escaped values are: a temp filesystem path (from TestEnvironment.Create), a version string from inline test data, and a resolved .targets file path — none will ever contain $(. No concrete failing scenario. |
@properties splat in PS1 |
Passes CI-controlled MSBuild property flags as individual arguments to dotnet msbuild. Trusted CI input; no user-facing surface. |
GetDirectories() exposure |
Scoped to the bootstrap artifacts directory. Read-only listing; no sensitive data exposed. |
| CI infra context | All changed code runs inside the repo's own CI pipeline on controlled agents. Threat model requires prior CI compromise. |
| Secrets/credentials | None handled or exposed anywhere in the diff. |
No actionable security issue found.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
dnceng.pkgs.visualstudio.compkgs.dev.azure.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "dnceng.pkgs.visualstudio.com"
- "pkgs.dev.azure.com"See Network Configuration for more information.
Generated by Expert Code Review (command) for issue #13694 · 2.9K AIC · ⊞ 29.8K ambient context
Comment /review to run again
eng/Versions.props now defines BootstrapSdkVersionFloor (effective bootstrap SDK = Max(floor, global.json tools.dotnet)). Update the release checklist and release skill to the new name and clarify the floor only needs raising to pin an SDK newer than tools.dotnet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔍 Skill Validator Results
Summary
Full validator output```text Found 1 skill(s) [release] 📊 release: 2,904 BPE tokens [chars/4: 2,869] (standard ~), 12 sections, 0 code blocks [release] ⚠ Skill is 2,904 BPE tokens (chars/4 estimate: 2,869) — approaching "comprehensive" range where gains diminish. [release] ⚠ No code blocks — agents perform better with concrete snippets and commands. ✅ All checks passed (1 skill(s)) ``` |
…ivation Imports eng/BootStrapMsBuild.props directly with explicit BootstrapSdkVersionFloor and NETCoreSdkVersion and asserts the derived BootstrapSdkVersion, covering all three branches (SDK wins, floor wins, empty NETCoreSdkVersion -> floor). Previously only the targets guard was tested, leaving the props derivation uncovered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…checklist The bootstrap SDK is Max(BootstrapSdkVersionFloor, tools.dotnet), so a normal tools.dotnet bump suffices; fold the now-conditional floor step into 5.4 (removing the orphaned 5.4b) while keeping guidance to adjust the floor when pinning a newer SDK or when it exceeds tools.dotnet on a branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
Expert-reviewer subagent launched for PR #13694 (24 dimensions, parallel sub-agent batches). The subagent will post its own inline review comments and submit a final review verdict directly to the PR once complete. |
|
@ViktorHofer @rainersigwald |
There was a problem hiding this comment.
Pull request overview
This PR prevents the repo’s bootstrap SDK from drifting behind the SDK pinned in global.json by (1) redefining the version in eng/Versions.props as a floor, (2) deriving the effective BootstrapSdkVersion as the max of that floor and the resolving SDK’s NETCoreSdkVersion, and (3) adding a post-acquire guard that fails early if the bootstrap’s laid-down runtime is older than the runtime bundled with the building SDK.
Changes:
- Rename
BootstrapSdkVersiontoBootstrapSdkVersionFloorand compute the effectiveBootstrapSdkVersionineng/BootStrapMsBuild.props. - Add
VerifyBootstrapRuntimeFloorafter SDK acquisition to detect runtime mismatches and fail fast. - Update stage-2 bootstrap scripts to query the resolved
BootstrapSdkVersionviadotnet msbuild -getProperty:BootstrapSdkVersion, and add unit tests + doc updates.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Build.UnitTests/BootstrapRuntimeFloor_Tests.cs |
Adds unit coverage for the derived bootstrap SDK version and the runtime-floor guard behavior. |
eng/Versions.props |
Renames the pinned bootstrap SDK value to a floor (BootstrapSdkVersionFloor) and clarifies intent via comments. |
eng/BootStrapMsBuild.props |
Derives BootstrapSdkVersion as Max(BootstrapSdkVersionFloor, NETCoreSdkVersion). |
eng/BootStrapMsBuild.targets |
Adds VerifyBootstrapRuntimeFloor to validate the bootstrap runtime laid down on disk meets the building SDK’s runtime floor. |
eng/cibuild_bootstrapped_msbuild.ps1 |
Resolves the effective bootstrap SDK version via dotnet msbuild -getProperty:BootstrapSdkVersion instead of parsing Versions.props. |
eng/cibuild_bootstrapped_msbuild.sh |
Same as PS1: resolve the effective bootstrap SDK version via dotnet msbuild -getProperty:BootstrapSdkVersion. |
documentation/release-checklist.md |
Updates release instructions to reflect the new floor semantics and reduced manual sync work. |
.github/skills/release/SKILL.md |
Updates the release skill doc to reference BootstrapSdkVersionFloor. |
| <_BootstrapRuntimeLeaf Include="@(_BootstrapRuntimeDir->'%(Filename)%(Extension)')" /> | ||
| <_BootstrapRuntimeVersion Include="@(_BootstrapRuntimeLeaf)" | ||
| Condition="$([System.Text.RegularExpressions.Regex]::IsMatch('%(Identity)', '^[0-9]+\.[0-9]+'))" /> | ||
| <_BootstrapRuntimeAtLeastFloor Include="@(_BootstrapRuntimeVersion)" | ||
| Condition="'%(Identity)' != '' and $([MSBuild]::VersionGreaterThanOrEquals('%(Identity)', '$(BundledNETCoreAppPackageVersion)'))" /> |
| resolving this project; the floor wins only when it is higher (pinning a newer SDK). --> | ||
| <PropertyGroup> | ||
| <BootstrapSdkVersion>$(BootstrapSdkVersionFloor)</BootstrapSdkVersion> | ||
| <BootstrapSdkVersion Condition="'$(NETCoreSdkVersion)' != '' and $([MSBuild]::VersionGreaterThan('$(NETCoreSdkVersion)', '$(BootstrapSdkVersion)'))">$(NETCoreSdkVersion)</BootstrapSdkVersion> |
There was a problem hiding this comment.
Do we need this condition, or should we just have the better error when we violate the runtime-version constraint? I lean toward "don't silently fix a misconfiguration" for this.
There was a problem hiding this comment.
@rainersigwald I thought that this condition is the whole point of this PR though. So we do not have a chore of manually bumping BootstapSdkVersion to match global.json.
There was a problem hiding this comment.
Ah, looking back I think that is what @ViktorHofer was suggesting. However, I'm not sure it's the right move. For a long time the bootstrap base and the SDK we used to build were very strongly coupled, and that sucked. We got them decoupled, which is great--but then we uncovered a more subtle coupling in the packaged-runtime-version stuff. I would personally rate things
{current state} < {reintroduce coupling but only in one direction} < {fully decoupled but clear deterministic errors when things aren't satisfied}
However I'd like to get a bit more consensus.
Fixes #13693
eng/Versions.props's hard-coded bootstrap SDK version andglobal.json'stools.dotnetwere maintained independently and could drift. When the bootstrap SDK lagstools.dotnet, its shared runtime ends up older than the building SDK delivers, causing host-resolution failures (You must install or update .NET to run this application).Changes
eng/Versions.props— RenameBootstrapSdkVersion->BootstrapSdkVersionFloor: it is a minimum, not the literal version.eng/BootStrapMsBuild.props— Derive the effectiveBootstrapSdkVersion = Max(BootstrapSdkVersionFloor, $(NETCoreSdkVersion)), so the bootstrap SDK can never lagglobal.json, while a higher floor can still pin a newer SDK.eng/BootStrapMsBuild.targets— AddVerifyBootstrapRuntimeFloor: after the SDK is acquired, fail the build if none of the laid-downMicrosoft.NETCore.Appruntimes is at least$(BundledNETCoreAppPackageVersion)(the runtime the building SDK delivers).eng/cibuild_bootstrapped_msbuild.ps1/.sh— Resolve the version viadotnet msbuild -getProperty:BootstrapSdkVersiononMSBuild.Bootstrap.csprojinstead of parsingVersions.props, so stage 2 locatessdk/<version>/MSBuild.dllusing the effective value. Fail fast if it cannot be read.documentation/release-checklist.mdand.github/skills/release/SKILL.mdfor the rename.Tests
src/Build.UnitTests/BootstrapRuntimeFloor_Tests.cscovers the guard: runtime equal/newer/older than the floor, multiple runtimes, prerelease-suffix handling, non-version directories, and absent/empty runtime root.