Migrate GetReferenceAssemblyPaths task to TaskEnvironment API#13495
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the built-in GetReferenceAssemblyPaths task to the TaskEnvironment API to support correct path resolution under multithreaded MSBuild execution, and updates internal caching to be thread-safe.
Changes:
- Mark
GetReferenceAssemblyPathsas multithreadable and route relative path resolution throughTaskEnvironment.GetAbsolutePath. - Replace a non-thread-safe static cache (
bool?) with a thread-safeLazy<bool>for the .NET 3.5 SP1 sentinel GAC lookup. - Add unit tests validating relative vs absolute behavior for
RootPathandTargetFrameworkFallbackSearchPaths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Tasks/GetReferenceAssemblyPaths.cs | Adds TaskEnvironment integration, multithreadable task surface, and thread-safe sentinel caching. |
| src/Tasks.UnitTests/GetReferencePaths_Tests.cs | Adds regressions to ensure relative inputs resolved via TaskEnvironment match absolute-input behavior. |
| AbsolutePath? absoluteRootPath = !string.IsNullOrEmpty(RootPath) | ||
| ? TaskEnvironment.GetAbsolutePath(RootPath) | ||
| : new AbsolutePath(RootPath, ignoreRootedCheck: true); | ||
| IList<AbsolutePath> absoluteFallbackSearchPaths = ResolveAbsoluteFallbackSearchPaths(TargetFrameworkFallbackSearchPaths); | ||
|
|
There was a problem hiding this comment.
Resolving RootPath/TargetFrameworkFallbackSearchPaths through TaskEnvironment.GetAbsolutePath(...) changes observable behavior when callers pass relative paths: ReferenceAssemblyPaths will now be absolute, whereas ToolLocationHelper.GetPathToReferenceAssemblies previously returned paths rooted in the original (possibly relative) inputs. For built-in tasks, this kind of behavior change should be gated behind a ChangeWave, or otherwise preserve legacy behavior when relative inputs are provided (while still fixing multithreaded correctness).
| public void TestRelativeRootPathProducesSameResultAsAbsolute() | ||
| { | ||
| using (var env = TestEnvironment.Create()) | ||
| { | ||
| string baseDir = env.DefaultTestDirectory.Path; |
There was a problem hiding this comment.
Test convention: new tests should inject ITestOutputHelper (store as _output) and create the environment via TestEnvironment.Create(_output) so diagnostic output is captured in CI. Also prefer Shouldly assertions over adding new xUnit Assert calls in modified code.
| public void TestRelativeFallbackSearchPathProducesSameResultAsAbsolute() | ||
| { | ||
| using (var env = TestEnvironment.Create()) | ||
| { | ||
| string baseDir = env.DefaultTestDirectory.Path; |
There was a problem hiding this comment.
Test convention: prefer using TestEnvironment env = TestEnvironment.Create(_output); (using-declaration, no braces) to align with repo patterns and ensure output capture. New assertions in this test should also use Shouldly rather than adding more xUnit Assert calls.
Fixes #13494
Context
The
GetReferenceAssemblyPathstask was made thread-safe for multithreaded MSBuild execution.Changes Made
GetReferenceAssemblyPaths.cs
static bool? s_net35SP1SentinelAssemblyFoundwithstatic readonly Lazy<bool>usingLazyThreadSafetyMode.PublicationOnlyto ensure thread-safe initialization while preserving retry-on-failure behavior.Testing
GetReferencePaths_Tests.cs
RootPath, once with a relativeRootPathresolved viaTaskEnvironment) and asserts identical output, validating that theTaskEnvironment.GetAbsolutePath()integration preserves behavior.TargetFrameworkFallbackSearchPaths: verifies a relative fallback path resolved viaTaskEnvironmentproduces the same result as the equivalent absolute path.