Skip to content

Add workaround for SDK root drive casing issue#17002

Closed
ViktorHofer wants to merge 10 commits into
mainfrom
ViktorHofer-patch-2
Closed

Add workaround for SDK root drive casing issue#17002
ViktorHofer wants to merge 10 commits into
mainfrom
ViktorHofer-patch-2

Conversation

@ViktorHofer

Copy link
Copy Markdown
Member

To double check:

Copilot AI review requested due to automatic review settings June 11, 2026 09:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Windows-specific workaround in Arcade SDK MSBuild targets to normalize $(NetCoreSdkRoot) drive-letter casing early in the build, mitigating task-host handshake failures caused by casing-sensitive salt computation.

Changes:

  • Adds InitialTargets="NormalizeNetCoreSdkRootCasing" to ensure normalization runs at the start of each project build.
  • Introduces an inline RoslynCodeTaskFactory task that probes canonical drive-letter casing via LoadLibraryExW + GetModuleFileNameW and rewrites only the drive letter.
  • Adds a NormalizeNetCoreSdkRootCasing target that updates the NetCoreSdkRoot property on Windows.
Show a summary per file
File Description
src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets Adds a new Windows-only target + inline task to normalize NetCoreSdkRoot drive casing and wires it via InitialTargets.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 3

Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 11, 2026 10:02
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ViktorHofer and others added 3 commits June 11, 2026 12:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets
ViktorHofer and others added 2 commits June 12, 2026 09:41
The previous LoadLibraryEx(LOAD_LIBRARY_AS_DATAFILE)+GetModuleFileName(hModule)
technique was a silent no-op: GetModuleFileName returns ERROR_MOD_NOT_FOUND for
an image loaded only as a data file (the SDK's MSBuild.exe/MSBuild.dll are not
otherwise loaded in the VS MSBuild process), so the casing was never fixed and
MSB4216 persisted (verified in build 2998220).

Open the SDK root directory with CreateFileW(FILE_FLAG_BACKUP_SEMANTICS) and ask
the OS for its canonical path via GetFinalPathNameByHandleW. This is load- and
bitness-independent. Splice only the canonical drive letter, and only when the
rest of the path is otherwise case-insensitively identical, so symlink/junction
resolution (which GetFinalPathNameByHandle performs but the child's
Environment.ProcessPath does not) cannot desync the two salts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make the temporary #14026 workaround leave visible evidence in CI console
logs (it only fires when the drive casing actually differs, so this is not
noisy on healthy machines).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 12, 2026 09:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets
Comment thread src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.targets
NetCoreSdkRoot carries a trailing directory separator, but
GetFinalPathNameByHandle returns the canonical path without one. The
strict full-path equality guard therefore always failed, making the
drive-casing normalization a silent no-op and leaving MSB4216 (#14026)
unfixed. Compare with trailing separators trimmed from both sides.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@JanProvaznik JanProvaznik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please avoid RoslynCodeTaskFactory. that'll make it difficult to get to the "0 nonmigrated tasks" for -mt testing on arcade-built repos

JanProvaznik pushed a commit that referenced this pull request Jun 24, 2026
…builds on #17002) (#17039)

Co-authored-by: Viktor Hofer <7412651+ViktorHofer@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ViktorHofer

Copy link
Copy Markdown
Member Author

Closing as #17039 got merged.

please avoid RoslynCodeTaskFactory.

@JanProvaznik we do depend on using RoslynCodeTaskFactory for applying workarounds quickly in Arcade. Rainer did the same when we had an msbuild regression around loading System.Text.Json in the past.

@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-2 branch June 29, 2026 08:15
ViktorHofer added a commit to dotnet/msbuild that referenced this pull request Jun 29, 2026
…ing (#14027)

### Context

A .NET Framework MSBuild host (e.g. VS) launching the .NET SDK task host
can fail the handshake with:

```
error MSB4216: Could not run the "..." task because MSBuild could not create or connect
to a task host with runtime "NET" and architecture "x86".
```

The handshake **salt** is a case-sensitive hash of the SDK tools
directory. For the **.NET task host** the two sides derive that
directory string differently:

- **Host** (.NET Framework MSBuild) passes the tools directory derived
from the `$(NetCoreSdkRoot)` property, whose drive-letter casing is
propagated verbatim from the environment (e.g. an Azure DevOps
`D:\a\_work\1\s` sources path) and is never canonicalized.
- **Child** (.NET task host) resolves its own path via
`Environment.ProcessPath` (`GetModuleFileNameW`), which on these agents
reports a **lowercase** drive letter.

When those casings differ (proven from the failing build''s comm traces:
host `D:\...\sdk\11.0.100-preview.5...` vs child
`d:\...\sdk\11.0.100-preview.5...`, salts `-1569896665` vs `-930293759`)
the salts differ and the handshake fails. Because the NET task host uses
a sentinel version (`99.99.99.99`), host and child can be different
MSBuild builds, so both must agree on the salt input.

### Approach (child-side salt widening)

The child validates the salt, so the fix **widens what the child
accepts**: when the received salt does not match, the .NET task host
child also accepts the salt computed from its tools directory with the
**opposite drive-letter casing**. Both spellings denote the same
directory on Windows, so this only **adds** an accepted value.

This is the only approach that is provably **non-regressing**:

- It is **monotonic** — the child accepts a superset of salts, so no
host that connects successfully today can ever be rejected. There is no
salt **value** change, hence no version-skew flag-day.
- The added value denotes the **same** SDK directory, so node-reuse
isolation is preserved (a genuinely different tools directory is still
rejected — covered by a unit test).
- It needs **no Arcade workaround** and **no VS change**: it fixes any
VS-host / updated-SDK-child pairing as soon as the SDK carries this
change.

Scope is limited to the `Runtime="NET"` task host path
(`IsNetTaskHost`); other node types and the CLR2 task host are
untouched.

The only pairing this cannot fix is an **already-shipped old SDK** child
— no host change can alter how an old child hashes its own path.
Customers get the fix by moving to an SDK that contains it.

### Why the previous host-side approach was abandoned

The earlier revision normalized the launch path on the host with
`GetFinalPathNameByHandle`, which **always returns an uppercase drive
letter**. That matched neither direction reliably: it was a no-op for an
already-uppercase host path (exactly the failing build), could not match
a child whose loader-resolved path used a lowercase drive, and risked
regressing configurations whose working casing was lowercase. The same
uppercasing is also why the Arcade workaround (dotnet/arcade#17002)
silently no-op''d.

### Changes Made

- `src/Framework/BackEnd/Handshake.cs`: for the .NET task host,
precompute the salt for the tools directory with the opposite
drive-letter casing (`_netTaskHostSaltDriveCaseVariant`) and expose
`IsNetTaskHostSaltMatch`. Helper `TryGetDriveLetterCaseVariant` returns
false off-Windows and for driveless/UNC paths, so non-Windows and
non-drive cases are unchanged.
- `src/Shared/NodeEndpointOutOfProcBase.cs`: in the child''s
`IsHandshakePartValid`, add a `Salt` case inside the existing `#if NET`
NET-task-host allowance block that accepts the drive-case variant.
- `src/Build.UnitTests/BackEnd/AppHostSupport_Tests.cs`: three
Windows-only tests — accepts the drive-casing variant, rejects an
unrelated SDK directory (isolation preserved), and confirms
non-NET-task-host nodes do not widen acceptance.
- The previous host-side canonicalization in
`NodeProviderOutOfProcTaskHost.cs` and the `GetFinalPathNameByHandle`
entry in `NativeMethods.txt` are **reverted**.

### Testing

- `Handshake_NetTaskHost_AcceptsDriveLetterCasingDifference`,
`Handshake_NetTaskHost_RejectsUnrelatedToolsDirectory`,
`Handshake_NonNetTaskHost_DoesNotWidenSaltAcceptance` — pass (net10.0).
- Existing `Handshake_ExternalPathCanMismatch_DefaultAlwaysMatches` —
still passes (no regression).
- Engine, MSBuild, MSBuildTaskHost and the test project build clean (0
warnings).

### Notes

- See #14026 for the full root-cause analysis and the comm-trace /
binlog evidence.
- No new warnings or errors are introduced, so this is safe under
`/WarnAsError`; no Change Wave is needed (pure compatibility widening).

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Jan Provazník <janprovaznik@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants