Migrate 5 MSBuild tasks to IMultiThreadableTask (Group 2: Bundle, Deps, Config & Hosts)#52936
Migrate 5 MSBuild tasks to IMultiThreadableTask (Group 2: Bundle, Deps, Config & Hosts)#52936SimaTian wants to merge 15 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates a set of SDK MSBuild tasks to participate in MSBuild’s multithreaded task execution model by implementing IMultiThreadableTask, wiring in TaskEnvironment, and replacing relative-path I/O with TaskEnvironment.GetAbsolutePath(...). It also adds unit-test coverage and introduces .NET Framework-gated polyfills for the new MSBuild APIs.
Changes:
- Migrate 4 tasks to
IMultiThreadableTask+TaskEnvironmentand absolutize relevant paths (GenerateBundle,GenerateDepsFile,WriteAppConfigWithSupportedRuntime,ResolveAppHosts). - Mark
ShowPreviewMessageas multithreadable via[MSBuildMultiThreadableTask]. - Add unit tests + a
TaskEnvironmentHelperfor constructingTaskEnvironmentinstances in tests, plus.NET Framework-gated polyfill implementations.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/GenerateBundle.cs | Implements IMultiThreadableTask and absolutizes OutputDir/bundle inputs via TaskEnvironment. |
| src/Tasks/Microsoft.NET.Build.Tasks/GenerateDepsFile.cs | Implements IMultiThreadableTask and absolutizes DepsFilePath before writing. |
| src/Tasks/Microsoft.NET.Build.Tasks/WriteAppConfigWithSupportedRuntime.cs | Implements IMultiThreadableTask and absolutizes input/output app.config paths. |
| src/Tasks/Microsoft.NET.Build.Tasks/ResolveAppHosts.cs | Implements IMultiThreadableTask and absolutizes runtime graph + targeting pack root usage. |
| src/Tasks/Microsoft.NET.Build.Tasks/ShowPreviewMessage.cs | Adds [MSBuildMultiThreadableTask] attribute. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateBundleMultiThreading.cs | Adds tests asserting interface/attribute and a smoke path-resolution scenario. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateDepsFileMultiThreading.cs | Adds interface/attribute/property presence tests. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAWriteAppConfigWithSupportedRuntimeMultiThreading.cs | Adds tests validating TaskEnvironment-based input/output path resolution. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAResolveAppHostsMultiThreading.cs | Adds test validating TargetingPackRoot resolution via TaskEnvironment. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAShowPreviewMessageMultiThreading.cs | Adds test validating [MSBuildMultiThreadableTask] presence. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/TaskEnvironmentHelper.cs | Adds helper for constructing TaskEnvironment via reflection/DispatchProxy. |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/TaskEnvironmentHelperTests.cs | Adds smoke tests for helper behavior and path resolution. |
| src/Tasks/Common/IMultiThreadableTask.cs | Adds .NET Framework-gated polyfill for IMultiThreadableTask. |
| src/Tasks/Common/TaskEnvironment.cs | Adds .NET Framework-gated polyfill for TaskEnvironment. |
| src/Tasks/Common/AbsolutePath.cs | Adds .NET Framework-gated polyfill for AbsolutePath. |
| src/Tasks/Common/ITaskEnvironmentDriver.cs | Adds .NET Framework-gated polyfill for the internal driver interface. |
| src/Tasks/Common/ProcessTaskEnvironmentDriver.cs | Adds .NET Framework-gated polyfill driver implementation. |
6e816e5 to
5069549
Compare
SimaTian
left a comment
There was a problem hiding this comment.
Addressed all review comments:
-
TaskEnvironment internal -> public: Fixed in the polyfills PR (#52909). \TaskEnvironment\ is now \public sealed class.
-
AbsolutePath internal -> public: Fixed in the polyfills PR (#52909). \AbsolutePath\ is now \public readonly struct.
-
GenerateBundle test assertion: Added \NotBeOfType\ assertion to verify that OutputDir is absolutized via TaskEnvironment rather than used as a relative path.
Branch rebased on updated polyfills and force-pushed.
Deep Audit: Forbidden API Usage in Helper Functions (Group 2)Traced all helper functions and referenced libraries called by the 5 Group 2 tasks for hidden forbidden API usage that bypasses the 🔴 CRITICAL:
|
| Task | Status | Issues |
|---|---|---|
| GenerateBundle | ✅ Clean | None |
| GenerateDepsFile | 🔴 Issues | FrameworkReferenceResolver uses Environment.GetEnvironmentVariable; AssetsFilePath/RuntimeGraphPath not absolutized; ResolvedFile ctor has Path.GetFullPath (not hit in this path) |
| WriteAppConfig | ✅ Clean | None |
| ResolveAppHosts | ✅ Clean | None |
| ShowPreviewMessage | ✅ Clean | None |
Recommended actions:
- GenerateDepsFile: Absolutize
AssetsFilePathandRuntimeGraphPathbefore passing to helpers - FrameworkReferenceResolver: Needs
TaskEnvironmentparameter (cross-cutting — affects multiple tasks) - ResolvedFile 4-arg ctor: Replace
Path.GetFullPathwith caller-side absolutization (latent, not active in this path)
Fixes applied (248cb91)Resolved the forbidden API issues identified in the deep audit: 1.
|
391a30e to
56f7474
Compare
SimaTian
left a comment
There was a problem hiding this comment.
As good as I can force the copilot to make it.
There is one open question - what do we do about the Runtime dependency which accesses Environment Variable directly? Is it safe enough to risk it or do we need to clone the method?
8a28fb0 to
fe0a2f0
Compare
…for merge-group-2 - Guard NotBeOfType<NullReferenceException> with null check (FluentAssertions fails on null subject even when no exception was thrown) - Add TaskEnvironmentDefaults.cs for NETFRAMEWORK lazy-init fallback - Apply lazy-init pattern to WriteAppConfigWithSupportedRuntime, GenerateDepsFile, ResolveAppHosts, GenerateBundle Co-authored-by: Copilot <[email protected]>
The combining constructor AbsolutePath(string, AbsolutePath) used Path.Combine without Path.GetFullPath, leaving '..' segments unresolved. This caused output paths like 'dir\..\ClassLib\...' instead of 'ClassLib\...', breaking string-based path comparisons in downstream MSBuild targets and tests. Co-authored-by: Copilot <[email protected]>
- Revert AbsolutePath.cs: remove Path.GetFullPath wrapping (matching real MSBuild) - Add UseShellExecute=false in ProcessTaskEnvironmentDriver.GetProcessStartInfo Co-authored-by: Copilot <[email protected]>
Production changes: - GenerateBundle: OutputDir null/empty guard (RC1), absolutize item.ItemSpec - GenerateDepsFile: absolutize ProjectPath (RC3), AssetsFilePath, RuntimeGraphPath - FrameworkReferenceResolver: static->instance with Func<string,string> delegate (C1) - ResolveAppHosts: field pattern for RuntimeGraphPath + TargetingPackRoot - WriteAppConfig: absolutize OutputAppConfigFile + appConfigItem paths - ShowPreviewMessage: attribute-only (no interface/property) Test improvements: - Dedicated test files per task (replacing monolithic GivenTasksUseAbsolutePaths) - [Collection(CWD-Dependent)] for CWD-mutating tests - result.Should().BeTrue() instead of if(result) guards (F3) - AssetsFilePath absolutization test with minimal project.assets.json (F1) - TargetingPackRoot + empty TargetingPackRoot tests (F2) - Removed old GivenAWriteAppConfigWithSupportedRuntimeMultiThreading.cs Co-authored-by: Copilot <[email protected]>
…ectory Empty OutputDir is handled by falling back to TaskEnvironment.ProjectDirectory (matching the old behavior of using Environment.CurrentDirectory). The test should not assert that an exception is thrown during path resolution. Co-authored-by: Copilot <[email protected]>
Replace DotNetReferenceAssembliesPathResolver.Resolve() with
_getEnvironmentVariable("DOTNET_REFERENCE_ASSEMBLIES_PATH"). The static
library method used process-global Environment.GetEnvironmentVariable,
bypassing the injected delegate entirely. This defeats the refactoring
that was specifically introduced for multithreaded MSBuild task support.
Add GivenAFrameworkReferenceResolver tests verifying the delegate is used.
Co-authored-by: Copilot <[email protected]>
… restore - GenerateDepsFile: use IsNullOrEmpty guard (not just null) for AssetsFilePath and RuntimeGraphPath before absolutizing - ResolveAppHosts: guard null/empty RuntimeGraphPath before absolutizing - GivenTasksUseAbsolutePaths: add TaskEnvironment + RuntimeGraphPath to ResolveAppHosts_NoFileIO test - GivenAGenerateDepsFileMultiThreading: restore CWD to Path.GetTempPath() instead of savedCwd to avoid race with parallel test cleanup Co-authored-by: Copilot <[email protected]>
On macOS, GetCurrentDirectory() throws FileNotFoundException when CWD was deleted by a parallel test. Remove all savedCwd variables from GivenAGenerateDepsFileMultiThreading (already using Path.GetTempPath() for restore). Add try-catch fallback in TaskTestEnvironment for both save and restore operations. Co-authored-by: Copilot <[email protected]>
AR-May
left a comment
There was a problem hiding this comment.
There are couple of problematic places in the tasks still.
| } | ||
|
|
||
| private void WriteDepsFile(string depsFilePath) | ||
| private void WriteDepsFile(string depsFilePath, string absoluteProjectPath, string absoluteAssetsFilePath, string absoluteRuntimeGraphPath) |
There was a problem hiding this comment.
Although this seems correct, it’s not very clean. There’s no reason to compute the absolute paths outside this function.
| string absoluteProjectPath = TaskEnvironment.GetAbsolutePath(ProjectPath); | ||
| string absoluteAssetsFilePath = !string.IsNullOrEmpty(AssetsFilePath) ? (string)TaskEnvironment.GetAbsolutePath(AssetsFilePath) : null; | ||
| string absoluteRuntimeGraphPath = !string.IsNullOrEmpty(RuntimeGraphPath) ? (string)TaskEnvironment.GetAbsolutePath(RuntimeGraphPath) : null; | ||
| WriteDepsFile(absoluteDepsFilePath, absoluteProjectPath, absoluteAssetsFilePath, absoluteRuntimeGraphPath); |
There was a problem hiding this comment.
Passing the absoluteDepsFilePath here loses the initial data about the potentially relative path. This results in FilesWritten being absolute path instead of potentially relative. This is a behavioral change, as this is the task output. To avoid it, prefer makingAbsolutePath objects and not casting them back to string. Rather make functions take AbsolutePath objects. This way you can access the initial potentially relative path via OriginalValue field and make correct output items. Also use the original value in logging.
I instead suggest backing the property by the AbsolutePath object and using it in the code. Like it is done here.
|
|
||
| protected override void ExecuteCore() | ||
| { | ||
| _absoluteRuntimeGraphPath = !string.IsNullOrEmpty(RuntimeGraphPath) |
There was a problem hiding this comment.
Consider backing the property with AbsolutePath objects.
There was a problem hiding this comment.
problem: Absolute path goes to this metadata and changes the output of the task from relative to absolute paths, this a behavioral change. See #52936 (comment). We need to use the absolute path only in file io, not allow it to leak to outputs when it was not there.
…puts - GenerateDepsFile: move absolutization from ExecuteCore into WriteDepsFile - ResolveAppHosts: use AbsolutePath? fields, preserve original TargetingPackRoot for SetMetadata output (fixes behavioral change where metadata became absolute) - Update tests: verify output metadata preserves relative path form Addresses @AR-May review comments on PR dotnet#52936. Co-authored-by: Copilot <[email protected]>
Same pattern as prior DepsFile fix: wrap Directory.SetCurrentDirectory(savedCwd) in try-catch with Path.GetTempPath() fallback. On macOS, parallel tests can delete the captured CWD directory, causing DirectoryNotFoundException. Co-authored-by: Copilot <[email protected]>
Implements IMultiThreadableTask to enable safe multi-threaded execution: - Added [MSBuildMultiThreadableTask] attribute - Added TaskEnvironment property with NETFRAMEWORK polyfill - Absolutize ProjectPath at task level before passing to DependencyContextBuilder - Moved AssetsFilePath and RuntimeGraphPath absolutization into WriteDepsFile - Use AbsolutePath.OriginalValue for FilesWritten output to preserve relativity Addresses all 3 AR-May findings from PR dotnet#52936: 1. ProjectPath absolutization: Now absolutized at ExecuteCore level via TaskEnvironment.GetAbsolutePath, passes .Value (absolute) to DependencyContextBuilder 2. Cleanliness: AssetsFilePath and RuntimeGraphPath absolutization moved into WriteDepsFile where consumed 3. Output fidelity: FilesWritten uses AbsolutePath.OriginalValue to preserve user-supplied relative paths Added comprehensive behavioral tests in GivenAGenerateDepsFileMultiThreading.cs: - OutputRelativityIsPreserved: Verifies FilesWritten preserves relative paths - ProjectPathIsAbsolutized: Ensures ProjectPath is absolutized before use - DecoyCwdDoesNotAffectPathResolution: Validates CWD independence - CrossThreadEnvVarIsolation: Tests environment variable isolation - MultiProcessParity: Confirms identical output with/without TaskEnvironment - ConcurrentExecutionProducesCorrectResults: Parallel execution test Co-authored-by: Copilot <[email protected]>
|
Closing this PR — the 5 tasks have been split into individual PRs for independent review and merge:
Each split addresses the AR-May review findings from this merge group (ProjectPath absolutization, output-path relativity via |
Implements IMultiThreadableTask to enable safe multi-threaded execution: - Added [MSBuildMultiThreadableTask] attribute - Added TaskEnvironment property with NETFRAMEWORK polyfill - Absolutize ProjectPath at task level before passing to DependencyContextBuilder - Moved AssetsFilePath and RuntimeGraphPath absolutization into WriteDepsFile - Use AbsolutePath.OriginalValue for FilesWritten output to preserve relativity Addresses all 3 AR-May findings from PR dotnet#52936: 1. ProjectPath absolutization: Now absolutized at ExecuteCore level via TaskEnvironment.GetAbsolutePath, passes .Value (absolute) to DependencyContextBuilder 2. Cleanliness: AssetsFilePath and RuntimeGraphPath absolutization moved into WriteDepsFile where consumed 3. Output fidelity: FilesWritten uses AbsolutePath.OriginalValue to preserve user-supplied relative paths Added comprehensive behavioral tests in GivenAGenerateDepsFileMultiThreading.cs: - OutputRelativityIsPreserved: Verifies FilesWritten preserves relative paths - ProjectPathIsAbsolutized: Ensures ProjectPath is absolutized before use - DecoyCwdDoesNotAffectPathResolution: Validates CWD independence - CrossThreadEnvVarIsolation: Tests environment variable isolation - MultiProcessParity: Confirms identical output with/without TaskEnvironment - ConcurrentExecutionProducesCorrectResults: Parallel execution test Co-authored-by: Copilot <[email protected]>
Implements IMultiThreadableTask to enable safe multi-threaded execution: - Added [MSBuildMultiThreadableTask] attribute - Added TaskEnvironment property with NETFRAMEWORK polyfill - Absolutize ProjectPath at task level before passing to DependencyContextBuilder - Moved AssetsFilePath and RuntimeGraphPath absolutization into WriteDepsFile - Use AbsolutePath.OriginalValue for FilesWritten output to preserve relativity Addresses all 3 AR-May findings from PR dotnet#52936: 1. ProjectPath absolutization: Now absolutized at ExecuteCore level via TaskEnvironment.GetAbsolutePath, passes .Value (absolute) to DependencyContextBuilder 2. Cleanliness: AssetsFilePath and RuntimeGraphPath absolutization moved into WriteDepsFile where consumed 3. Output fidelity: FilesWritten uses AbsolutePath.OriginalValue to preserve user-supplied relative paths Added comprehensive behavioral tests in GivenAGenerateDepsFileMultiThreading.cs: - OutputRelativityIsPreserved: Verifies FilesWritten preserves relative paths - ProjectPathIsAbsolutized: Ensures ProjectPath is absolutized before use - DecoyCwdDoesNotAffectPathResolution: Validates CWD independence - CrossThreadEnvVarIsolation: Tests environment variable isolation - MultiProcessParity: Confirms identical output with/without TaskEnvironment - ConcurrentExecutionProducesCorrectResults: Parallel execution test Co-authored-by: Copilot <[email protected]>
Migrate 5 MSBuild tasks to IMultiThreadableTask (Group 2: Bundle, Deps, Config & Hosts)
Context
This PR migrates 5 MSBuild tasks to support multithreaded execution by implementing
IMultiThreadableTask.Migrated Tasks
OutputDirandFilesToBundle[].ItemSpecviaTaskEnvironment.GetAbsolutePath()DepsFilePathbeforeFile.Create()OutputAppConfigFile.ItemSpecand input app.config pathRuntimeGraphPathandTargetingPackRootChanges Per Task
Each task receives:
[MSBuildMultiThreadableTask]attributeIMultiThreadableTaskinterface implementationTaskEnvironmentpropertyTaskEnvironment.GetAbsolutePath())New Test Files
GivenAGenerateBundleMultiThreading.csGivenAGenerateDepsFileMultiThreading.csGivenAWriteAppConfigWithSupportedRuntimeMultiThreading.csGivenAResolveAppHostsMultiThreading.csGivenAShowPreviewMessageMultiThreading.csTesting