Migrate Csc to multithreaded MSBuild execution - #84500
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…for 18.7 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4145242b-ec7c-4ff0-bace-d006bf403feb
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates Roslyn’s MSBuild Csc task and its supporting compiler-server plumbing to work with MSBuild’s multithreaded task execution model by routing environment/path/working-directory decisions through MSBuild’s TaskEnvironment instead of process-global state.
Changes:
- Opts
Cscinto multithreaded execution and adjusts MSBuild task behavior to preferTaskEnvironment(working directory, env vars, path absolutization). - Adds task-aware overloads in shared compiler-server utilities to allow server startup/environment decisions to be based on a caller-provided environment/path snapshot.
- Adds/updates unit tests covering injected environment/path handling and task-directory-relative path resolution.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Compilers/Shared/RuntimeHostInfo.cs | Adds overloads to resolve dotnet path / DOTNET_ROOT using injected env/path accessors. |
| src/Compilers/Shared/CompilerServerLogger.cs | Adds constructor overload to inject env lookup + path absolutization for log-file resolution. |
| src/Compilers/Shared/BuildServerConnection.cs | Adds task-aware server request overloads and environment/process-info helpers for server startup. |
| src/Compilers/Server/VBCSCompilerTests/BuildServerConnectionTests.cs | Adds regression tests validating snapshot-based env/path behavior. |
| src/Compilers/Core/Portable/InternalUtilities/CompilerOptionParseUtilities.cs | Allows feature-flag injection to read ROSLYN_CACHE_PATH via injected env accessor. |
| src/Compilers/Core/MSBuildTaskTests/RuntimeHostInfoTests.cs | Adds test ensuring dotnet discovery works with TaskEnvironment-relative PATH entries. |
| src/Compilers/Core/MSBuildTaskTests/ManagedCompilerGlobalCacheTests.cs | Minor cleanup consistent with env-driven feature-flag behavior. |
| src/Compilers/Core/MSBuildTaskTests/CscTests.cs | Adds tests for task-environment dotnet host path and reference resolution against project directory. |
| src/Compilers/Core/MSBuildTask/Utilities.cs | Adds task-environment-aware GetFullPathNoThrow overload. |
| src/Compilers/Core/MSBuildTask/ManagedToolTask.cs | Uses TaskEnvironment for dotnet discovery and runtime-host related environment behavior. |
| src/Compilers/Core/MSBuildTask/ManagedCompiler.cs | Uses TaskEnvironment for server logger, env probing, path normalization, and server startup. |
| src/Compilers/Core/MSBuildTask/Csc.cs | Marks Csc as [MSBuildMultiThreadableTask]. |
|
Test failures look legitimate. I have not reviewed the code. |
jaredpar
left a comment
There was a problem hiding this comment.
This change is leaving problematic APIs like Environment.GetEnvironmentVariable in place and callable from code. Also the passing around of ProcessStartInfo in our creation APIs seems to break invariants and it's hard to see a justification for why this is happening.
Overall this change seems to be made from manual inspection of our code vs. running the MT analyzer over it. Is that the case? If so why aren't we using the analyzer here?
| try | ||
| { | ||
| var filePath = Path.Combine(item, fileName); | ||
| var filePath = getFullPath(Path.Combine(item, fileName)); |
There was a problem hiding this comment.
This is the wrong fix. If a path in this is not absolute we should just skip it. Yes I realize that the code was broken before but honestly didn't think about that. Don't want to perpetuate the bad behavior.
There was a problem hiding this comment.
Fixed, If path is not absolute we skip it now.
| internal static string GetDotNetPathOrDefault() => | ||
| GetDotNetPathOrDefault(Environment.GetEnvironmentVariable, static path => path); |
There was a problem hiding this comment.
This feels like the wrong fix here. The is hiding us using process wide state behind a pleasant looking API. We should be threading through the Func<string, string?> getEnvironment parameter here to.
There was a problem hiding this comment.
Now only overload with explicit Func<string, string?> getEnvironment parameter is called.
| /// Static class initializer that initializes logging. | ||
| /// </summary> | ||
| public CompilerServerLogger(string identifier, string? loggingFilePath = null) | ||
| : this(identifier, loggingFilePath, Environment.GetEnvironmentVariable, static path => path) |
There was a problem hiding this comment.
Same feedback about using Environment.GetEnvironmentVariable just hiding the problem here. This should be threaded through.
| string pipeName, | ||
| Func<string, string?> getEnvironmentVariable, | ||
| Func<string, string> getFullPath, | ||
| ProcessStartInfo? processStartInfo, |
There was a problem hiding this comment.
This breaks contracts around the API. It means that on Windows and Linux we can get different behaviors. There needs to be a substantial reason for adding this here vs. fixing up the implementation to create the process correctly (on all OS).
There was a problem hiding this comment.
You are right, I will not just pass a snapshot of current environmentVariables from task
| TryCreateServerCore( | ||
| clientDirectory, | ||
| pipeName, | ||
| Environment.GetEnvironmentVariable, |
There was a problem hiding this comment.
Similar feedback about this needing to be removed.
There was a problem hiding this comment.
Fixed, I now pass snapshot from environmentVariables
| internal static bool TryCreateServer( | ||
| string clientDirectory, | ||
| string pipeName, | ||
| ProcessStartInfo processStartInfo, |
There was a problem hiding this comment.
Same feedback about this breaking our guarantees around TryCreateServer.
| // Clear DOTNET_ROOT* variables such as DOTNET_ROOT_X64 before setting our own DOTNET_ROOT. | ||
| foreach (var key in environmentVariables.Keys.ToArray()) | ||
| { | ||
| if (key.StartsWith(RuntimeHostInfo.DotNetRootEnvironmentName, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| environmentVariables[key] = string.Empty; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What is the reason for changing where this logic happens?
There was a problem hiding this comment.
I moved it while extracting the common helper but it was indeed unnecessary. I reverted it
|
@jaredpar |
…host discovery and compiler server logging. Skip relative PATH entries instead of trying to absolutize it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Compilers/Shared/CompilerServerLogger.cs:121
- The PR description says the existing
CompilerServerLogger(string identifier, string? loggingFilePath = null)constructor is preserved, but this file currently only exposes the injected-delegate constructor. Reintroducing the original convenience overload keeps the API stable for non-MSBuild callers and avoids the repeated boilerplate at call sites that don't need task-specific environment/path behavior.
/// <summary>
/// Initializes logging using the supplied environment variable lookup and path absolutization.
/// </summary>
/// <param name="getEnvironmentVariable">Reads the named environment variable.</param>
/// <param name="makeAbsolutePath">
/// Resolves a (possibly relative) path to an absolute one before it is used for file system
/// access.
/// </param>
public CompilerServerLogger(
string identifier,
string? loggingFilePath,
Func<string, string?> getEnvironmentVariable,
Func<string, string> makeAbsolutePath)
{
…d of caller-owned ProcessStartInfo instances. Keep process construction consistent across Windows and Unix
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Compilers/Shared/CompilerServerLogger.cs:121
- The PR description says the existing
CompilerServerLoggerconstructor is kept, but this change replaces it with a delegate-based constructor and forces all call sites to passEnvironment.GetEnvironmentVariable/ identity path mapping. Consider reintroducing the original convenience overload (forwarding to the new constructor) to match the stated compatibility goal and avoid repeating boilerplate across entry points.
/// <summary>
/// Initializes logging using the supplied environment variable lookup and path absolutization.
/// </summary>
/// <param name="getEnvironmentVariable">Reads the named environment variable.</param>
/// <param name="makeAbsolutePath">
/// Resolves a (possibly relative) path to an absolute one before it is used for file system
/// access.
/// </param>
public CompilerServerLogger(
string identifier,
string? loggingFilePath,
Func<string, string?> getEnvironmentVariable,
Func<string, string> makeAbsolutePath)
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Compilers/Shared/CompilerServerLogger.cs:121
- The PR description indicates the existing
CompilerServerLoggerconstructor is kept, but this file now only exposes the new delegate-based constructor. Consider reintroducing the prior overload so existing call sites (and external/internal consumers) can keep using the simple API while new task-aware callers can opt into the injected behaviors.
/// <summary>
/// Initializes logging using the supplied environment variable lookup and path absolutization.
/// </summary>
/// <param name="getEnvironmentVariable">Reads the named environment variable.</param>
/// <param name="makeAbsolutePath">
/// Resolves a (possibly relative) path to an absolute one before it is used for file system
/// access.
/// </param>
public CompilerServerLogger(
string identifier,
string? loggingFilePath,
Func<string, string?> getEnvironmentVariable,
Func<string, string> makeAbsolutePath)
{
src/Compilers/Core/MSBuildTask/ManagedCompiler.cs:1234
CheckAllReferencesExistOnDisknow callsTaskEnvironment.GetAbsolutePath(itemSpec)insideFile.Exists. IfGetAbsolutePath(...).Valuethrows for an invalidItemSpec, this would regress the pre-migration behavior where invalid references were treated as missing (logging MSB3104) rather than throwing. Consider guarding the absolutization and treating failures as missing references.
var itemSpec = reference.ItemSpec;
if (string.IsNullOrEmpty(itemSpec) || !File.Exists(this.TaskEnvironment.GetAbsolutePath(itemSpec)))
{
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Compilers/Shared/CompilerServerLogger.cs:121
- The PR description says the existing
CompilerServerLogger(string identifier, string? loggingFilePath = null)constructor is kept, but this file only has the injected-delegate constructor now. If the intent is to preserve the existing API and keep standalone tool call sites simpler, reintroduce the old overload and forward it to the injected implementation.
/// <summary>
/// Initializes logging using the supplied environment variable lookup and path absolutization.
/// </summary>
/// <param name="getEnvironmentVariable">Reads the named environment variable.</param>
/// <param name="makeAbsolutePath">
/// Resolves a (possibly relative) path to an absolute one before it is used for file system
/// access.
/// </param>
public CompilerServerLogger(
string identifier,
string? loggingFilePath,
Func<string, string?> getEnvironmentVariable,
Func<string, string> makeAbsolutePath)
{
src/Compilers/Shared/BuildServerConnection.cs:538
GetServerEnvironmentVariablesis declared as returning a nullable dictionary, and the call sites still branch onenvironmentVariables != null, but the implementation always constructs and returns a dictionary. Consider either (1) making the method non-nullable and simplifying the null checks, or (2) reintroducing a realnullreturn for the “inherit current process environment” case (if that’s still a supported scenario), to avoid dead code and confusing nullability.
/// <param name="logger">Optional logger for logging environment variable setup</param>
/// <returns>Dictionary of environment variables to set</returns>
internal static Dictionary<string, string>? GetServerEnvironmentVariables(
Func<string, string?> getEnvironmentVariable,
IReadOnlyList<KeyValuePair<string, string?>> currentEnvironment,
ICompilerServerLogger? logger = null)
I think we do not call Environment.GetEnvironmentVariable in the Csc task path? This is true that I started migration through manual code inspection first. The only really unsafe API remains is Path.GetTempPath(). As you discussed with Chet our suggestion in analyzer is not really valid for getTempPath so I need to discuss with my team what we should do in this case. |
What .NET SDK version has the appropriate MSBuild DLLs in it?
Not exactly sure what to do here. We have a good understanding fo what the algorithm is but rather than everyone implement it I think it would be best to have a helper in the MSBuild APIs that we can use instead. |
| buildRequest, | ||
| pipeName, | ||
| GetClientDirectory(), | ||
| Environment.GetEnvironmentVariable, |
There was a problem hiding this comment.
We should add a Func<string, string> parameter here and require callers to pass the appropriate implementation. My mental model is that eventually these uses are going to be flagged by your analyzer. I want us to be in a state where we have the least number of suppressions possible that reduces the risk that we get anything wrong. For cases like this we should either be
- Abstracting out to
Func<string, string>and forcing the caller to thread through the appropraite method. In the case of the tasks they will thread throughTaskEnvironment.GetEnvironmnetVariablefor callers like csc.exe they will thread throughEnvironment.GetEnvironmentVariable. - Add an entry in the msbuild task project file like
<DefineConstant>$(DefineConstant);ROSLYN_MSBUILD_TASKand then wrap this in an#if !ROSLYN_MSBUILD_TASK
| string pipeName, | ||
| string clientDirectory, | ||
| Func<string, string?> getEnvironmentVariable, | ||
| IReadOnlyList<KeyValuePair<string, string?>> environmentVariables, |
There was a problem hiding this comment.
This seems unnecessary. It's the same information provided two ways. Can we just pass a dictionary here that way we get both in one parameter?
| internal static KeyValuePair<string, string?>[] CreateEnvironmentVariableSnapshot( | ||
| IEnumerable<KeyValuePair<string, string>> currentEnvironment) | ||
| { | ||
| var result = new List<KeyValuePair<string, string?>>(); | ||
| foreach (var entry in currentEnvironment) | ||
| { | ||
| result.Add(new KeyValuePair<string, string?>(entry.Key, entry.Value)); | ||
| } | ||
|
|
||
| return result.ToArray(); | ||
| } |
There was a problem hiding this comment.
This seems unnecessary, callers could just use .ToArray()
| internal static KeyValuePair<string, string?>[] CreateEnvironmentVariableSnapshot(System.Collections.IDictionary currentEnvironment) | ||
| { | ||
| var result = new KeyValuePair<string, string?>[currentEnvironment.Count]; | ||
| var index = 0; | ||
| foreach (System.Collections.DictionaryEntry entry in currentEnvironment) | ||
| { | ||
| result[index++] = new KeyValuePair<string, string?>((string)entry.Key, (string?)entry.Value); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Seems unnecessary, callers can just use .OfType<DictionaryEntry>().ToArray(x => KeyValuePair.Create(x.Key, x.Value)
| Func<string, string?> getEnvironmentVariable, | ||
| IReadOnlyList<KeyValuePair<string, string?>> environmentVariablesSnapshot, |
There was a problem hiding this comment.
This is one of my top areas of concern about these changes. I 100% get the necessity of no longer using Environment directly in our tasks now. At the same time it seems like there is no interface / API that is a suitable replacement for it. Instead we're getting a mix of Func<string, string>, IROList, IDictionary, etc ...
I think it would be simpler if we just treaded through ITaskEnvironmentDriver here. It is the base abstraction the new APIs are using. If that is causing challenges beacuse of the multi-use of this code we could create a local one that has the same surface area.
@jaredpar 10.0.400 |
|
Csc will be migrated in this PR -> #84701 |
Fixes dotnet/msbuild#14230
Summary
Migrates the
CscMSBuild task to MSBuild's multithreaded task execution model while preserving existing compiler arguments, server reuse, cache behavior, and process-wide temporary-directory behavior.Changes
Opt
Cscinto multithreaded execution[MSBuildMultiThreadableTask]to the concreteCsctask. The attribute is intentionally not added toManagedCompiler,ManagedToolTask, orVbc.TaskEnvironmentinherited from MSBuild 18.7.1'sToolTask.Resolve task-local state through
TaskEnvironmentTaskEnvironment.ProjectDirectoryfor the working directory sent in compiler-server requests instead of the MSBuild process CWD.LIB, runtime-host variables, tiered-compilation settings,ROSLYN_CACHE_PATH, and command-line logger configuration fromTaskEnvironment.TaskEnvironment.GetAbsolutePath.GetFullPathNoThrowoverload so task absolutization and canonicalization remain inside the existing I/O-related exception filter.Keep compiler process startup task-aware
TaskEnvironment.GetProcessStartInfo()when the task needs to startVBCSCompiler.DOTNET_ROOTcalculation, pipe arguments, and server process construction owned byBuildServerConnection.Make command-line logging task-aware
CompilerServerLoggerso the MSBuild task can read the logging variable fromTaskEnvironmentand resolve relative log paths against the task project directory.csc,vbc, andVBCSCompilerprocess behavior unchanged.Microsoft Reviewers: Open in CodeFlow