Migrate FormatUrl to multithreaded execution model#13573
Migrate FormatUrl to multithreaded execution model#13573jankratochvilcz wants to merge 8 commits intomainfrom
Conversation
🔍 Skill Validator Results
Summary
Full validator output```text Found 13 skill(s) [assessing-breaking-changes] 📊 assessing-breaking-changes: 992 BPE tokens [chars/4: 1,154] (detailed ✓), 8 sections, 2 code blocks [merge-dependency-updates] 📊 merge-dependency-updates: 2,030 BPE tokens [chars/4: 1,887] (detailed ✓), 16 sections, 7 code blocks [reviewing-msbuild-code] 📊 reviewing-msbuild-code: 166 BPE tokens [chars/4: 216] (compact ✓), 1 sections, 0 code blocks [reviewing-msbuild-code] ⚠ Skill is only 166 BPE tokens (chars/4 estimate: 216) — may be too sparse to provide actionable guidance. [reviewing-msbuild-code] ⚠ No code blocks — agents perform better with concrete snippets and commands. [reviewing-msbuild-code] ⚠ No numbered workflow steps — agents follow sequenced procedures more reliably. [maintaining-binary-log-compatibility] 📊 maintaining-binary-log-compatibility: 1,271 BPE tokens [chars/4: 1,509] (detailed ✓), 12 sections, 2 code blocks [release] 📊 release: 1,901 BPE tokens [chars/4: 1,889] (detailed ✓), 11 sections, 0 code blocks [release] ⚠ No code blocks — agents perform better with concrete snippets and commands. [multithreaded-task-migration] 📊 multithreaded-task-migration: 3,719 BPE tokens [chars/4: 4,048] (standard ~), 30 sections, 9 code blocks [multithreaded-task-migration] ⚠ Skill is 3,719 BPE tokens (chars/4 estimate: 4,048) — approaching "comprehensive" range where gains diminish. [use-bootstrap-msbuild] 📊 use-bootstrap-msbuild: 709 BPE tokens [chars/4: 751] (detailed ✓), 14 sections, 5 code blocks [pipelines-health-check] 📊 pipelines-health-check: 5,025 BPE tokens [chars/4: 5,098] (comprehensive ✗), 40 sections, 10 code blocks [pipelines-health-check] ⚠ Skill is 5,025 BPE tokens (chars/4 estimate: 5,098) — "comprehensive" skills hurt performance by 2.9pp on average. Consider splitting into 2–3 focused skills. [authoring-errors-and-warnings] 📊 authoring-errors-and-warnings: 1,287 BPE tokens [chars/4: 1,393] (detailed ✓), 13 sections, 4 code blocks [changewaves] 📊 changewaves: 901 BPE tokens [chars/4: 1,007] (detailed ✓), 9 sections, 1 code blocks [optimizing-msbuild-performance] 📊 optimizing-msbuild-performance: 1,182 BPE tokens [chars/4: 1,342] (detailed ✓), 10 sections, 1 code blocks [integrating-sdk-and-msbuild] 📊 integrating-sdk-and-msbuild: 1,250 BPE tokens [chars/4: 1,440] (detailed ✓), 14 sections, 2 code blocks [deploy-msbuild-to-vs] 📊 deploy-msbuild-to-vs: 2,312 BPE tokens [chars/4: 2,363] (detailed ✓), 24 sections, 12 code blocks ✅ All checks passed (13 skill(s)) Found 1 agent(s) Validated 1 agent(s)✅ All checks passed (1 agent(s)) |
There was a problem hiding this comment.
Pull request overview
This PR migrates the FormatUrl built-in task to MSBuild’s multithreaded task execution model by injecting TaskEnvironment and anchoring relative-path resolution to the project directory (instead of the process current directory), following the “absolutize-at-boundary” migration pattern.
Changes:
- Mark
FormatUrlas[MSBuildMultiThreadableTask], implementIMultiThreadableTask, and route relative-path resolution throughTaskEnvironment.ProjectDirectory. - Update
PathUtil.Format/PathUtil.Resolveto accept anAbsolutePath baseDirectoryand resolve local relative paths viaAbsolutePath(...).GetCanonicalForm(). - Refactor and extend
FormatUrlunit tests to injectTaskEnvironmentand add a project-directory-relative resolution regression test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Tasks/ManifestUtil/PathUtil.cs |
Threads a base directory through path formatting/resolution and anchors local relative paths to the project directory. |
src/Tasks/FormatUrl.cs |
Migrates FormatUrl to multithreaded task model via attribute + IMultiThreadableTask and uses TaskEnvironment.ProjectDirectory. |
src/Tasks.UnitTests/FormatUrl_Tests.cs |
Updates tests to provide TaskEnvironment and adds coverage for project-directory-relative resolution. |
.github/skills/multithreaded-task-migration/SKILL.md |
Updates migration guidance around TaskEnvironment and test setup patterns. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Review: 24-Dimension Analysis of PR #13573 (Migrate FormatUrl to Multithreaded Execution)
🔴 BLOCKING Issues
Dimension 1: Backwards Compatibility — LGTM
PathUtil is internal static class, so the signature change from Format(string) to Format(string, AbsolutePath) has no public API impact. FormatUrl is the only caller. The ArgumentException on Windows whitespace-only input preserves the historical Path.GetFullPath(" ") behavior. No new warnings are emitted.
Dimension 2: ChangeWave Discipline — LGTM
The behavioral change (resolving against ProjectDirectory instead of Environment.CurrentDirectory) only manifests in multithreaded mode, which is itself an opt-in feature. No ChangeWave needed — this is part of the multithreaded execution model, not a behavior change in the default mode.
Dimension 13: Concurrency & Thread Safety — LGTM
FormatUrl has no shared mutable state. TaskEnvironment.ProjectDirectory is set once by the engine before Execute(). PathUtil methods are stateless. No concurrency concerns.
Dimension 21: Evaluation Model Integrity — N/A (task-only change)
Dimension 24: Security Awareness — LGTM
No security regression. Path resolution is now anchored to ProjectDirectory rather than process cwd, which is actually more secure in multi-tenant scenarios.
🟡 Issues Found (see inline comments)
Dimension 22: Correctness & Edge Cases — ISSUE (BLOCKING)
FormatUrl.TaskEnvironment lacks = TaskEnvironment.Fallback; default initializer. All 19 other migrated tasks have it. Direct instantiation without setting TaskEnvironment → NullReferenceException. See inline comment on FormatUrl.cs:21.
Dimension 5: Error Message Quality — ISSUE (MODERATE)
Hardcoded English string "Path cannot be null or whitespace on Windows." in PathUtil.Resolve. Should be in .resx per MSBuild conventions. Mitigated by the fact that old code also threw a BCL-provided English message. See inline comment on PathUtil.cs:251.
✅ Clean Dimensions
| # | Dimension | Verdict |
|---|---|---|
| 3 | Performance & Allocation | LGTM — AbsolutePath is a readonly struct, no unnecessary allocations. PathUtil is not a hot path. |
| 4 | Test Coverage | LGTM — Null, empty, whitespace (per-platform), UNC, URL, localhost, absolute, relative, and the new project-directory-relative test. Excellent coverage. |
| 6 | Logging & Diagnostics | LGTM — No logging changes needed for this simple task. |
| 7 | String Comparison | LGTM — Existing StringComparison.OrdinalIgnoreCase for localhost comparison is correct (network hostnames). |
| 8 | API Surface | LGTM — FormatUrl.TaskEnvironment is public but it implements the IMultiThreadableTask interface which requires it. PathUtil is internal. No PublicAPI.txt tracking needed (checked — this project doesn't use them). |
| 9 | Target Authoring | N/A — no .targets/.props changes. |
| 10 | Design & Intent | LGTM — "Absolutize-at-boundary" pattern correctly applied; PathUtil.Format/Resolve gain the base directory parameter, which is pattern #2 from the SKILL.md. |
| 11 | Cross-Platform | LGTM — Windows whitespace guard with Unix pass-through preserves historical cross-platform semantics. #if NET / #if !NET guards preserved. |
| 12 | Code Simplification | LGTM — Clean, minimal change. |
| 14 | Naming | LGTM — baseDirectory parameter name is clear. GetTarget() test helper is descriptive. |
| 15 | SDK Integration | N/A |
| 16 | Idiomatic C# | LGTM — file has #nullable disable and the PR correctly does not add nullable annotations. |
| 17 | File I/O & Path Handling | LGTM — Correctly uses AbsolutePath + GetCanonicalForm() instead of raw Path.GetFullPath. |
| 18 | Documentation | LGTM — SKILL.md updates are valuable: Sin 7 (cwd buried in helpers), migration pattern ranking, pure-string utility callouts. Comments in PathUtil explain the whitespace guard rationale well. |
| 19 | Build Infrastructure | LGTM |
| 20 | Scope & PR Discipline | LGTM — Single concern (FormatUrl migration) + related SKILL.md documentation. |
| 23 | Dependency Management | LGTM — No new dependencies. |
Summary
This is a well-structured migration following the established "absolutize-at-boundary" pattern. The SKILL.md documentation additions (Sin 7, migration patterns, utility callouts) are excellent contributions.
One blocking fix required: Add = TaskEnvironment.Fallback default to FormatUrl.TaskEnvironment to match the pattern used by all other migrated tasks and prevent NRE on direct instantiation.
Generated by Expert Code Review (on open) for issue #13573 · ● 13.5M
- Add TaskEnvironment.Fallback default initializer on FormatUrl (matches convention used by 19 other migrated tasks; prevents NRE on direct instantiation) - Move hardcoded English whitespace error to Strings.resx (PathUtil.WhitespacePathNotAllowedOnWindows) - Revert overboard SKILL.md additions (Updating Unit Tests reverted to main; Patterns for Migrating Tasks and Sin 7 sections removed) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good find from our bot :) |
| // a rooted base directory, Path.GetFullPath silently trims the trailing whitespace, masking the | ||
| // error. Preserve the historical Windows contract by explicitly rejecting whitespace-only input. | ||
| // Unix retains the historical accepting behavior because whitespace is a valid filename character there. | ||
| if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path)) |
There was a problem hiding this comment.
We should consider adding this check to the GetCanonicalForm instead - as it seems it may be applicable in other tasks as well.
There was a problem hiding this comment.
supernit
| if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path)) | |
| if (NativeMethodsShared.IsWindows && string.IsNullOrWhiteSpace(path)) |
this edge case is horrible though. on one hand we want getcanonicalform to be as straightforward as possible but this is a general footgun with getabsolutepath().getcanonicalform() nesting 😦
There was a problem hiding this comment.
we already reject absolutization of empty and null
I vote for it to be rejected at construction of AbsolutePath
now in the constructor we have: ArgumentException.ThrowIfNullOrEmpty(path);
proposal:
if (NativeMethodsShared.IsWindows)
{
ArgumentException.ThrowIfNullOrWhitespace(...)
}
else
{
ArgumentException.ThrowIfNullOrEmpty(...)
}There was a problem hiding this comment.
a new nightmare scenario occurred to me:
Windows MSBuild controlling a build that happens on wsl... I think we should somewhere explicitly document this is forbidden (it'd break more stuff than just this). Do we @baronfel @rainersigwald ?
There was a problem hiding this comment.
I don't think we explicitly document it, but you are right that today tons of stuff is broken or doesn't work as intended. Many aspects of our toolchain embed OS-specific paths and do not work when accessed from another platform. My favorite example is project.assets.json embedding paths that don't work across platforms.
There was a problem hiding this comment.
I think it's not so bad as that? You can't directly control/invoke processes on another OS, so cross-build systems like C++-for-linux-from-VS and some of the Xamarin stuff have tasks that result in RPC-over-OS. You can apply normalization in those tasks and catch problems.
There was a problem hiding this comment.
@JanProvaznik thank for the feedback. I tried to add it to GetCanonicalForm since for the AbsolutePath we're skipping the validation block where you proposed we add things (I'm not 100% sure if the skip is only perf-related, but I quickly skimmed the usages and it's hard where we're not skipping it 👀). Please let me know if it works
There was a problem hiding this comment.
(unsure if this PR is the place to have that bigger-picture discussion, @JanProvaznik maybe better to start a separate issue / Teams thread on this?)
There was a problem hiding this comment.
Oh I wrote the constructor but meant the ValidatePath which is called by the constructor.
It would make sense to me here
or does that cause issues?
There was a problem hiding this comment.
Took another stab, please take a look. I also narrowed the condition a bit to only catch a non-empty whitespace string since that was the edge case we're chasing. I also see this won't be full-proof as ideally we'd also add this to ValidatePath. But it's a bit challenging to know what to do here as we're protecting a very theoretical contract and I think this type of issue could have been implicit in many cases.
| // a rooted base directory, Path.GetFullPath silently trims the trailing whitespace, masking the | ||
| // error. Preserve the historical Windows contract by explicitly rejecting whitespace-only input. | ||
| // Unix retains the historical accepting behavior because whitespace is a valid filename character there. | ||
| if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path)) |
There was a problem hiding this comment.
supernit
| if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path)) | |
| if (NativeMethodsShared.IsWindows && string.IsNullOrWhiteSpace(path)) |
this edge case is horrible though. on one hand we want getcanonicalform to be as straightforward as possible but this is a general footgun with getabsolutepath().getcanonicalform() nesting 😦
| // a rooted base directory, Path.GetFullPath silently trims the trailing whitespace, masking the | ||
| // error. Preserve the historical Windows contract by explicitly rejecting whitespace-only input. | ||
| // Unix retains the historical accepting behavior because whitespace is a valid filename character there. | ||
| if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path)) |
There was a problem hiding this comment.
we already reject absolutization of empty and null
I vote for it to be rejected at construction of AbsolutePath
now in the constructor we have: ArgumentException.ThrowIfNullOrEmpty(path);
proposal:
if (NativeMethodsShared.IsWindows)
{
ArgumentException.ThrowIfNullOrWhitespace(...)
}
else
{
ArgumentException.ThrowIfNullOrEmpty(...)
}| // a rooted base directory, Path.GetFullPath silently trims the trailing whitespace, masking the | ||
| // error. Preserve the historical Windows contract by explicitly rejecting whitespace-only input. | ||
| // Unix retains the historical accepting behavior because whitespace is a valid filename character there. | ||
| if (NativeMethodsShared.IsWindows && String.IsNullOrWhiteSpace(path)) |
There was a problem hiding this comment.
a new nightmare scenario occurred to me:
Windows MSBuild controlling a build that happens on wsl... I think we should somewhere explicitly document this is forbidden (it'd break more stuff than just this). Do we @baronfel @rainersigwald ?
### Context @jankratochvilcz was pointing two friction points with pr-reviewer: - Non actionable comments - e.g. #13573 (comment) - The overviw table containing numerous 'LGTM' rows - which reduces 'signal-to-noise' ratio ### Changes Guiding to reduce nonactionable outputs cc @jankratochvilcz
Migrates the
FormatUrltask to MSBuild's multithreaded execution model, applying the "absolutize-at-boundary" pattern previously established for other tasks (see #13267).Fixes #13567.
Non-obvious decisions
Explicit Windows whitespace guard in
PathUtil.Resolve. Pre-migration,Path.GetFullPath(" ")threwArgumentExceptionon Windows. After absolutizing first,Path.Combine(projectDir, " ")produces"projectDir\ "andPath.GetFullPathsilently trims the trailing whitespace and returns the project directory — silently losing the contract. To preserve historical behavior,Resolvenow does an explicitIsNullOrWhiteSpacecheck on Windows and throws with a clearer message ("Path cannot be null or whitespace on Windows."). Whitespace remains valid on Unix, matching the original cross-platform behavior. Captured as "Sin 5a" in the multithreaded-task-migration skill so future migrations catch the same trap.Validation
Microsoft.Build.Tasks.UnitTestsFormatUrl_Testspass on net10.0 and net472 (2 Unix-only tests skip on Windows).Microsoft.Build.Tasks.UnitTestssuite passes on net10.0 (1184/1184).