Conversation
| /// Returns key value pairs of environment variables in a dictionary | ||
| /// with a case-insensitive key comparer. | ||
| /// </summary> | ||
| internal static Dictionary<string, string> GetEnvironmentVariables() |
There was a problem hiding this comment.
TODO: Before we used functions from CommunicationUtilities that are more sophisticated and may have better perf. To avoid perf. regression we need to figure out a more proper migration of those functions from Shared files.
There was a problem hiding this comment.
Pull request overview
This PR advances “RAR task enlightening” by routing path resolution and environment access through TaskEnvironment (including in the out-of-proc RAR node), and by upgrading several RAR inputs from strings to AbsolutePath to ensure stable, project-relative-to-absolute resolution in multithreaded builds.
Changes:
- RAR and related resolvers now use
TaskEnvironmentfor absolute path conversion and environment variable access. - Out-of-proc RAR node now hydrates a multithreaded
TaskEnvironmentusing the client project directory. - Introduces a new ChangeWave (18.6) and updates task/unit test code to provide
TaskEnvironment.
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/SystemState.cs | Uses TaskEnvironment for state file path handling during precomputed cache load. |
| src/Tasks/StateFileBase.cs | Clarifies expectation that state file path is absolute. |
| src/Tasks/RedistList.cs | Moves redist list paths to AbsolutePath and enables TaskEnvironment-based absolutization. |
| src/Tasks/InstalledSDKResolver.cs | Caches absolute SDK root paths for resolver probing. |
| src/Tasks/GetReferenceAssemblyPaths.cs | Marks task as multi-threadable and passes TaskEnvironment into GAC resolution. |
| src/Tasks/AssemblyDependency/Resolver.cs | Adds TaskEnvironment to resolver base for consistent path/env usage. |
| src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs | Converts several inputs to AbsolutePath, adds MT task interface, and updates RAR execution to pass TaskEnvironment through the pipeline. |
| src/Tasks/AssemblyDependency/ReferenceTable.cs | Propagates TaskEnvironment into resolution pipeline and normalizes resolved paths. |
| src/Tasks/AssemblyDependency/Reference.cs | Adds IsFrameworkFile overload for AbsolutePath[] framework paths. |
| src/Tasks/AssemblyDependency/RawFilenameResolver.cs | Resolves raw filename candidate via TaskEnvironment. |
| src/Tasks/AssemblyDependency/Node/RarNodeExecuteRequest.cs | Captures project directory for out-of-proc node TaskEnvironment creation. |
| src/Tasks/AssemblyDependency/Node/OutOfProcRarNodeEndpoint.cs | Creates a multithreaded TaskEnvironment per request using the client project directory. |
| src/Tasks/AssemblyDependency/HintPathResolver.cs | Resolves hint path using TaskEnvironment and canonicalizes. |
| src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs | Switches env-var access to TaskEnvironment and threads it through GetLocation APIs. |
| src/Tasks/AssemblyDependency/GacResolver.cs | Threads TaskEnvironment into GAC resolver construction. |
| src/Tasks/AssemblyDependency/FrameworkPathResolver.cs | Threads TaskEnvironment into framework path resolver construction. |
| src/Tasks/AssemblyDependency/DirectoryResolver.cs | Caches absolute search path via TaskEnvironment. |
| src/Tasks/AssemblyDependency/CandidateAssemblyFilesResolver.cs | Threads TaskEnvironment into candidate assembly files resolver construction. |
| src/Tasks/AssemblyDependency/AssemblyResolution.cs | Threads TaskEnvironment through resolver compilation APIs. |
| src/Tasks/AssemblyDependency/AssemblyFoldersResolver.cs | Threads TaskEnvironment into AssemblyFolders resolver. |
| src/Tasks/AssemblyDependency/AssemblyFoldersFromConfig/AssemblyFoldersFromConfigResolver.cs | Uses TaskEnvironment for cache escape-hatch env var access and passes it into cache. |
| src/Tasks/AssemblyDependency/AssemblyFoldersFromConfig/AssemblyFoldersFromConfigCache.cs | Uses TaskEnvironment for cache escape-hatch env var access. |
| src/Tasks/AssemblyDependency/AssemblyFoldersExResolver.cs | Uses TaskEnvironment for cache escape-hatch env var access and passes it into cache. |
| src/Tasks.UnitTests/RARPrecomputedCache_Tests.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/HintPathResolver_Tests.cs | Updates resolver test to provide TaskEnvironment. |
| src/Tasks.UnitTests/GetReferencePaths_Tests.cs | Updates task test to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/WinMDTests.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/VerifyTargetFrameworkHigherThanRedist.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/VerifyTargetFrameworkAttribute.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/VerifyIgnoreVersionForFrameworkReference.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/SuggestedRedirects.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/StronglyNamedDependencyAutoUnify.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/StronglyNamedDependencyAppConfig.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/StronglyNamedDependency.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/SpecificVersionPrimary.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceTestFixture.cs | Updates helper to pass TaskEnvironment into GAC resolution. |
| src/Tasks.UnitTests/AssemblyDependency/Perf.cs | Updates perf tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/NonSpecificVersionStrictPrimary.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/Node/RarNodeExecuteRequest_Tests.cs | Updates node request tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/Node/OutOfProcRarNode_Tests.cs | Updates node tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/InstalledSDKResolverFixture.cs | Updates resolver fixture to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/GlobalAssemblyCacheTests.cs | Updates cache tests to pass TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/FilePrimary.cs | Updates RAR tests to provide TaskEnvironment. |
| src/Tasks.UnitTests/AssemblyDependency/AssemblyFoldersFromConfig_Tests.cs | Updates tests to provide TaskEnvironment. |
| src/Framework/MultiThreadedTaskEnvironmentDriver.cs | Moves driver into Framework layer and updates env-var comparer dependency. |
| src/Framework/MultiProcessTaskEnvironmentDriver.cs | Moves driver into Framework layer and routes env-var ops through Framework utilities. |
| src/Framework/FrameworkCommunicationsUtilities.cs | Adds Framework-local env-var utilities needed by moved drivers. |
| src/Framework/ChangeWaves.cs | Adds new wave 18.6 and includes it in AllWaves. |
| src/Build/Microsoft.Build.csproj | Removes compilation of drivers from Build project (now in Framework). |
| src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs | Updates comment to reflect new Framework env-var utilities usage. |
Comments suppressed due to low confidence (1)
src/Tasks/AssemblyDependency/ReferenceTable.cs:496
- When
assemblyFileNameis relative,reference.FullPathis made absolute viaTaskEnvironment, but the subsequent_fileExists,_directoryExists,_getAssemblyName, andResolvedSearchPathlogic still uses the original (potentially relative)assemblyFileName. In multithreaded/out-of-proc scenarios this can probe the wrong directory and fail to resolve assemblies. Use the resolved absolute path (e.g.,reference.FullPath) consistently for file system checks and assembly name probing.
| _resolvedSDKs = resolvedSDKs; | ||
| _resolvedSDKPaths = new Dictionary<string, string>(resolvedSDKs.Count); | ||
|
|
||
| // Cache absolute paths to avoid repeated TaskEnvironment.GetAbsolutePath calls | ||
| foreach (var kvp in resolvedSDKs) | ||
| { | ||
| _resolvedSDKPaths[kvp.Key] = taskEnvironment.GetAbsolutePath(kvp.Value.ItemSpec); | ||
| } |
There was a problem hiding this comment.
_resolvedSDKPaths is created without a case-insensitive comparer. If sdkName differs in casing from the keys stored in _resolvedSDKs (which is OrdinalIgnoreCase), _resolvedSDKs.TryGetValue can succeed while _resolvedSDKPaths.TryGetValue fails, causing the SDK resolver to incorrectly skip resolution. Construct _resolvedSDKPaths with StringComparer.OrdinalIgnoreCase (or reuse resolvedSDKs.Comparer) to match _resolvedSDKs behavior.
src/Tasks/AssemblyDependency/AssemblyFoldersFromConfig/AssemblyFoldersFromConfigResolver.cs
Outdated
Show resolved
Hide resolved
|
|
||
| /// <summary> | ||
| /// Reads the specified file from disk into a StateFileBase derived object. | ||
| /// stateFile should be absolute path to the file on disk. |
There was a problem hiding this comment.
can it be AbsolutePath type?
There was a problem hiding this comment.
I can look into it. In some cases, I decided to not do that because I will need to jump back and forth between the AbsolutePath and string. This one probably should be possible to just switch to AbsolutePath.
There was a problem hiding this comment.
Ok, I did not do that because there are other tasks that use it. However, I can create a copy of this function that would take AbsolutePath and once all other tasks are enlightened, we can remove the old function.
| if (assemblyName != null) | ||
| { | ||
| // {AssemblyFolders} was passed in. | ||
| // {AssemblyFolders} was passed in. They come from registry and are assumed to be absolute paths. |
There was a problem hiding this comment.
isn't that the utility of AbsolutePath? we can just wrap each of these to get this guarantee on the code level
There was a problem hiding this comment.
I guess we can do that to additionally ensure it!
There was a problem hiding this comment.
Ah, looked again and remembered why i did not want to do that. I do not want to cast all the time in the resolver, but I did not want to do this on the source either - as the values are computed once and then stored in the static cache, same situation as in ToolLocationHelper.
| ResolveAssemblyReference t = new ResolveAssemblyReference | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| TaskEnvironment = TaskEnvironmentHelper.CreateForTest(), |
There was a problem hiding this comment.
we were quite lazy about this test desgin in prior prs., should we consider being less lazy and having both taskenvironments in each test? or are we confident they work the same way from unit testing them?
This env is always multiprocess, which is great for catching regression, but not asserting correctness of mt.
There was a problem hiding this comment.
Yes, absolutely, I had exactly same thoughts about it! Yes, we should and I was looking into doing that in a separate PR - for every test that uses TaskEnvironmentHelper.CreateForTest()
There was a problem hiding this comment.
Idea for MT would be to set the mt environment and then move the current directory to something else. In the end of the test, maybe on the dispose to ensure it, we would return it back. And for MP environment it would be same as now.
| ResolveAssemblyReference t = new ResolveAssemblyReference(); | ||
|
|
||
| t.BuildEngine = new MockEngine(_output); | ||
| t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest(); |
There was a problem hiding this comment.
Just a general question - we've got design discussion about a default for the TaskEnvironment. If/when it goes through, can we then remove all these? It's a lot of noise that does basically nothing.
There was a problem hiding this comment.
yes, though it'd be super nice to test with both test environments which we now don't do :(
|
|
||
| // TODO: This needs to be updated when configuration support is added to pass client-specific state | ||
| // For now, using the environment variables from the RAR node process. | ||
| using (var environmentDriver = new MultiThreadedTaskEnvironmentDriver(request.ProjectDirectory)) |
There was a problem hiding this comment.
Are these failures related to the defaults we need or are these a different beast altogether? (seems like this will need a similar stub like the one I created for the SDK)
| else | ||
| { | ||
| _filesInDirectories = new(assemblyFoldersFromConfig.AsParallel() | ||
| .Where(assemblyFolder => FileUtilities.DirectoryExistsNoThrow(assemblyFolder.DirectoryPath)) |
There was a problem hiding this comment.
Do we have any guarantee this path will be properly absolutized?
There was a problem hiding this comment.
According to copilot:
_filesInDirectories cache is broken if paths aren't absolute. Directory.GetFiles(relativeDir) stores relative strings in the HashSet, but Contains(path) receives paths from
ResolveFromDirectory which may be in a different form → silent cache miss.
| } | ||
|
|
||
| // Lets see if the processor architecture matches, note this this method will cache the result when it was first called. | ||
| AssemblyNameExtension foundAssembly = getAssemblyName(candidatePath); |
There was a problem hiding this comment.
do we have any guarantee that this path will be properly absolutized?
There was a problem hiding this comment.
I'm not saying it isn't - didn't look that deep. However if the code changes elsewhere, we have no visibility here.
Fixes #12483
Context
RAR task enlightening. De-facto the only inputs that I saw unenlightened in the logs were
StateFile,AppConfigFileandCandidateAssemblyFiles. However multiple other inputs, likeTargetFrameworkDirectoriesorHintPathof assemblies are not guaranteed to be the full path. I added additional logic that ensures that the full path is used for them too.Changes Made
StateFileandAppConfigFile)Breaking changes under a ChangeWave:
AppConfigFileresulted before in failure while now it is skipped (as if it was null before)StateFile.ItemSpecis used for the path instead ofStateFile.ToString()which could be otherwise defined in a customITaskItemCommits overview:
Testing (in progress)
Checklist: