Migrate Csc to multithreaded MSBuild execution - #84701
Open
jaredpar wants to merge 19 commits into
Open
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…for 18.7 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4145242b-ec7c-4ff0-bace-d006bf403feb
…host discovery and compiler server logging. Skip relative PATH entries instead of trying to absolutize it.
…d of caller-owned ProcessStartInfo instances. Keep process construction consistent across Windows and Unix
…nikao/migrate-csc
This change uses BannedSymbols to ensure we're not using problematic APIs from the `System.Environment` type
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
jaredpar
commented
Jul 30, 2026
| internal static void PrependFeatureFlagFromEnvironment(List<string> arguments, Action<string>? log = null) | ||
| #if !MICROSOFT_CODEANALYSIS_MSBUILD_TASK | ||
| internal static void PrependFeatureFlagFromEnvironment(List<string> arguments, Action<string>? log = null) => | ||
| PrependFeatureFlagFromEnvironment(arguments, Environment.GetEnvironmentVariable, log); |
Member
Author
There was a problem hiding this comment.
This is an example of a use of Envirnonment.GetEnvironmentVariable that is very hard to spot without compiler enforcement.
OvesN
reviewed
Jul 30, 2026
| var requestId = getRequestId(); | ||
| logger.Log($"Compilation request {requestId}, PathToTool={pathToTool}"); | ||
|
|
||
| string? tempDirectory = Path.GetTempPath(); |
There was a problem hiding this comment.
We can temporally for now add in roslyn some private helper to resolve temp path, using TaskEnvironment to get env variables. Later we will add such helper to the msbuild directly.
Member
Author
There was a problem hiding this comment.
Sounds good. I have an old impl of GetTempPath which should emulate the real implementation we can use temporarily. This PR has a number of fundamentals though we need for the other changes. So plan is to try and get this PRin without more scope creep and then send PRs to fix items like GetTempPath
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes broad infrastructure changes across the build (MSBuild task execution + compiler server environment capture), MSBuild workspace project loading (including file-based apps via RPC), and several related test/benchmark updates to validate multithreaded and daemonized execution scenarios.
Changes:
- Opt
Csc/Vbcinto MSBuild multithreaded task execution and route task-local state throughTaskEnvironment(working directory, env vars, path resolution, server startup). - Add RPC plumbing to support loading “file-based app” projects via MSBuild workspace/build host, including a new
IFileBasedProgramServiceworkspace service and synced FileBasedPrograms source. - Update rename candidate-reason handling and add/adjust LSP/unit/benchmark coverage to exercise the new concurrency model and file-based program flows.
Show a summary per file
| File | Description |
|---|---|
| src/Workspaces/MSBuild/Core/Rpc/RemoteProjectInstance.cs | New RPC wrapper for ProjectInstance operations. |
| src/Workspaces/MSBuild/Core/Rpc/RemoteBuildHost.cs | Adds project+instance loading overloads (virtual project support). |
| src/Workspaces/MSBuild/Core/MSBuild/ProjectFileExtensionRegistry.cs | Recognizes file-based app entrypoints (via service). |
| src/Workspaces/MSBuild/Core/MSBuild/MSBuildWorkspace.cs | Handles file-based apps during ApplyChanges load. |
| src/Workspaces/MSBuild/Core/MSBuild/MSBuildProjectLoader.cs | Wires IFileBasedProgramService into loader/provider path. |
| src/Workspaces/MSBuild/Core/MSBuild/FileBasedProgramsProjectLoader.cs | New helper to build/load file-based app projects via RPC. |
| src/Workspaces/MSBuild/Core/MSBuild/BuildHostProjectFileInfoProvider.cs | Uses file-based loader when applicable; selects host kind. |
| src/Workspaces/MSBuild/Contracts/IProjectInstance.cs | New RPC contract for MSBuild ProjectInstance. |
| src/Workspaces/MSBuild/Contracts/IBuildHost.cs | Extends load APIs for virtual project + project instance. |
| src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstanceReader.cs | Uses physical file path for virtual projects. |
| src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstance.cs | New server-side IProjectInstance implementation. |
| src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectFile.cs | Carries PhysicalFilePath into instance reading. |
| src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/Extensions.cs | Adjusts ref directive references to use physical paths. |
| src/Workspaces/MSBuild/BuildHost/MSBuild/Constants/MetadataNames.cs | Adds metadata name for ref-directive source. |
| src/Workspaces/MSBuild/BuildHost/Build/ProjectBuildManager.cs | Adds global properties + project instance creation support. |
| src/Workspaces/MSBuild/BuildHost/AbstractBuildHost.cs | Plumbs global properties + instance loading through host. |
| src/Workspaces/CSharp/Portable/SyncedSource/README.md | Marks SyncedSource as generated. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hant.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hans.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.tr.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.ru.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.pt-BR.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.pl.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.ko.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.ja.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.it.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.fr.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.es.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.de.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.cs.xlf | Updates translations/strings for FileBasedPrograms. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/Sha256Hasher.cs | Adds synced hashing utility. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/ProjectLocator.cs | Adds synced project discovery utility. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/MSBuildUtilities.cs | Updates shared MSBuild utilities file sharing logic. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/IBuildService.cs | Adds synced build-service abstraction interfaces. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/FileBasedProgramsResources.resx | Adds new resource strings and locked notes. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/ExternalHelpers.cs | Adds synced external helper hooks for netstandard. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/Extensions.cs | Adds polyfill extensions for older targets. |
| src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/.editorconfig | Marks synced sources as generated/analyzer-suppressed. |
| src/Workspaces/CSharp/Portable/SyncedSource/commitid.txt | Records upstream SDK commit for synced sources. |
| src/Workspaces/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Workspaces.csproj | Embeds FileBasedPrograms resources; removes one compile item. |
| src/Workspaces/CSharp/Portable/FileBasedPrograms/FileBasedProgramService.cs | New workspace service implementation over VirtualProjectBuilder. |
| src/Workspaces/CSharp/Portable/FileBasedPrograms/ExternalHelpers.cs | Implements source-package hooks using Roslyn utilities. |
| src/Workspaces/Core/Portable/Rename/SymbolicRenameLocations.ReferenceProcessing.cs | Refines candidate reason for overload-resolution failures. |
| src/Workspaces/Core/Portable/Rename/RenameUtilities.cs | Treats overload-resolution failure similarly to member group. |
| src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj | Links in IBuildService abstraction into Workspaces. |
| src/Workspaces/Core/Portable/FileBasedPrograms/IFileBasedProgramService.cs | New internal service contract for file-based programs. |
| src/Tools/IdeCoreBenchmarks/Program.cs | Ensures BenchmarkDotNet global properties don’t break graphs; returns exit code. |
| src/Tools/IdeCoreBenchmarks/LanguageServer/LanguageServerDaemonBenchmarks.cs | New benchmarks to compare daemon vs standalone servers. |
| src/Tools/IdeCoreBenchmarks/LanguageServer/LanguageServerBenchmarkHost.cs | New benchmark host wrapper around LSP test infra. |
| src/Tools/IdeCoreBenchmarks/IdeCoreBenchmarks.csproj | Adds conditional LSP test project refs; excludes net472 sources. |
| src/Razor/src/Razor/src/Microsoft.VisualStudioCode.RazorExtension/Services/CohostConfigurationChangedService.cs | Adds new VS Code setting ingestion for Razor cohost. |
| src/NuGet/Microsoft.CodeAnalysis.BuildClient.Package/Microsoft.CodeAnalysis.BuildClient.targets | Adds shared internal utilities to build client package. |
| src/LanguageServer/Protocol.TestUtilities/Microsoft.CodeAnalysis.LanguageServer.Protocol.Test.Utilities.csproj | Adds IVT for IdeCoreBenchmarks. |
| src/LanguageServer/Protocol.TestUtilities/LanguageServer/MaterializedLspWorkspace.cs | New helper to materialize test workspaces on disk. |
| src/LanguageServer/Protocol.TestUtilities/LanguageServer/LspWorkspaceContent.cs | Moves namespace + small refactors for reuse. |
| src/LanguageServer/Protocol.TestUtilities/LanguageServer/LspTestWorkspaces.cs | Moves namespace + adds console app workspace factory. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs | Uses updated ProjectFileExtensionRegistry (file-based support). |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/VirtualProjectXmlProvider.cs | Removes dotnet run-api-based virtual project provider. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/IFileBasedProgramServiceExtensions.cs | Adds discovery cache helpers via new service. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsWorkspaceProviderFactory.cs | Stops injecting removed virtual project provider. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs | Switches file-based app loading to FileBasedProgramsProjectLoader. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsEntryPointDiscovery.cs | Routes cache directory logic through new service. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscellaneousFilesProjectProvider.cs | Updates LoadProjectAsync call signature. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/VirtualProjectXmlProviderTests.cs | Removes tests for deleted run-api provider. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerMefHost.cs | Adds composition cache warming helper. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerHostTests.cs | Adds OpenProjectsAsync helper + daemon shutdown wait. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/MiscellaneousFiles/FileBasedProgramsWorkspaceTests.cs | Adds #:ref directive test coverage. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Microsoft.CodeAnalysis.LanguageServer.UnitTests.csproj | Adds package ref + IVT to benchmarks. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Daemon/LanguageServerProjectLoadingTests.cs | Adds daemon/standalone loading coverage. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Utilities/Usings.cs | Adds global using for new test utilities namespace. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Utilities/AbstractLanguageServerClientTests.cs | Reuses MaterializedLspWorkspace helper. |
| src/Features/CSharp/Portable/SyncedSource/commitid.txt | Removes old synced-source commit record. |
| src/Features/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Features.csproj | Removes embedded resources/defines for FileBasedPrograms. |
| src/EditorFeatures/Test2/Rename/RenameEngineTests.CSharpConflicts.vb | Adds rename regression tests for unbound/ambiguous cases. |
| src/EditorFeatures/Test2/Rename/InlineRenameTests.vb | Updates rename flyout test to allow submit with conflicts. |
| src/EditorFeatures/Core/InlineRename/UI/Adornment/RenameFlyoutViewModel.cs | Allows Submit when replacement text is valid (not blocked by error severity). |
| src/Compilers/Shared/RuntimeHostInfo.cs | Adds TaskEnvironment-friendly env/path probing; skips relative PATH segments. |
| src/Compilers/Shared/CompilerServerLogger.cs | Adds env/path delegates for task-aware logging. |
| src/Compilers/Shared/BuildServerConnection.cs | Makes server env/process creation task-aware via env snapshots. |
| src/Compilers/Shared/BuildClient.cs | Passes typed environment map into server build requests. |
| src/Compilers/Server/VBCSCompilerTests/BuildServerConnectionTests.cs | Updates tests for typed env maps. |
| src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs | Updates BoundRefArrayAccess typing and rewriter integration. |
| src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs | Updates BoundRefArrayAccess construction. |
| src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs | Fixes ref-array access emission path for used/unused cases. |
| src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs | Emits address for ref-array access via ref-assignment helper. |
| src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_HasHome.cs | Adds HasHome handling for ref-array access. |
| src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml | Makes BoundRefArrayAccess.Type non-nullable. |
| src/Compilers/Core/Portable/InternalUtilities/PathExtensions.cs | Adds Path.IsPathFullyQualified polyfill for non-NET targets. |
| src/Compilers/Core/Portable/InternalUtilities/EnvironmentExtensions.cs | Adds typed env snapshot + comparer/compare helpers. |
| src/Compilers/Core/Portable/InternalUtilities/Debug.cs | Narrows banned-API suppression scope around env var read. |
| src/Compilers/Core/Portable/InternalUtilities/CompilerOptionParseUtilities.cs | Adds delegate-based env reading (task-aware). |
| src/Compilers/Core/MSBuildTaskTests/VbcTests.cs | Updates runtime host path expectations. |
| src/Compilers/Core/MSBuildTaskTests/TestUtilities/IntegrationTestBase.cs | Uses new CompilerServerLogger constructor overload. |
| src/Compilers/Core/MSBuildTaskTests/RuntimeHostInfoTests.cs | Adds task-environment PATH tests + relative PATH filtering test. |
| src/Compilers/Core/MSBuildTaskTests/ManagedCompilerGlobalCacheTests.cs | Updates env-based feature flag plumbing. |
| src/Compilers/Core/MSBuildTaskTests/CscTests.cs | Adds TaskEnvironment/multithreaded reference resolution tests. |
| src/Compilers/Core/MSBuildTask/Vbc.cs | Marks Vbc as multithreaded task. |
| src/Compilers/Core/MSBuildTask/Utilities.cs | Adds TaskEnvironment-based GetFullPathNoThrow overload. |
| src/Compilers/Core/MSBuildTask/Sdk/Microsoft.Build.Tasks.CodeAnalysis.Sdk.csproj | Adds MT banned symbols + define for MSBuild task builds. |
| src/Compilers/Core/MSBuildTask/MSBuild/Microsoft.Build.Tasks.CodeAnalysis.csproj | Adds MT banned symbols + define for MSBuild task builds. |
| src/Compilers/Core/MSBuildTask/ManagedToolTask.cs | Uses TaskEnvironment for DOTNET_ROOT/LIB/working dir behavior. |
| src/Compilers/Core/MSBuildTask/ManagedCompiler.cs | Uses TaskEnvironment env/path; updates server request + reference checks. |
| src/Compilers/Core/MSBuildTask/Extensions.cs | Adds TaskEnvironment.GetFullPath helper. |
| src/Compilers/Core/MSBuildTask/Directory.Build.props | Includes new Extensions.cs in task builds. |
| src/Compilers/Core/MSBuildTask/Csc.cs | Marks Csc as multithreaded task. |
| src/Compilers/Core/MSBuildTask/BannedSymbols.MSBuildMT.txt | Adds banned APIs list for multithreaded task safety. |
| eng/Version.Details.xml | Bumps arcade/helix/xliff toolset versions. |
| eng/Version.Details.props | Bumps arcade/helix/xliff props versions. |
| eng/validate-benchmarks.ps1 | Adds IdeCoreBenchmarks runner build validation in CI. |
| eng/ensure-sources-synced.cs | Moves synced-source location to Workspaces + includes MSBuildUtilities. |
| eng/common/core-templates/steps/publish-logs.yml | Removes one feed token argument. |
| .vscode/tasks.json | Adds “build Compilers.slnf with analyzers” task. |
| global.json | Moves repo to .NET 11 preview SDK/tooling and updates Arcade/Helix versions. |
Copilot's findings
- Files reviewed: 112/118 changed files
- Comments generated: 3
Comment on lines
+1230
to
1234
| var itemSpec = reference.ItemSpec; | ||
|
|
||
| if (string.IsNullOrEmpty(itemSpec) || !File.Exists(TaskEnvironment.GetFullPath(itemSpec))) | ||
| { | ||
| success = false; |
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/Compilers/Core/MSBuildTask/ManagedCompiler.cs:1234
CheckAllReferencesExistOnDisknow callsTaskEnvironment.GetFullPath(itemSpec)outside of any exception filter. SinceGetFullPathultimately usesPath.GetFullPath, invalid reference strings (e.g. containing illegal path chars) can throw and fail the task, whereas the previousFile.Exists(reference.ItemSpec)behavior reported MSB3104 without throwing.
var itemSpec = reference.ItemSpec;
if (string.IsNullOrEmpty(itemSpec) || !File.Exists(TaskEnvironment.GetFullPath(itemSpec)))
{
success = false;
src/Compilers/Shared/BuildServerConnection.cs:562
GetServerEnvironmentVariablesusesStringComparison.OrdinalIgnoreCasewhen matchingDOTNET_ROOT*keys. On non-Windows platforms environment variable keys are case-sensitive, and this PR already introducedEnvironment.EnvironmentVariableComparisonfor OS-appropriate comparisons. Using the OS-specific comparison here avoids clearing unrelated variables on Unix and keeps the behavior consistent with the comparer used for the dictionary.
// Clear DOTNET_ROOT* variables such as DOTNET_ROOT_X64 by setting them to empty,
// as we want to set our own DOTNET_ROOT and avoid conflicts
if (dotNetRoot != null && key.StartsWith(RuntimeHostInfo.DotNetRootEnvironmentName, StringComparison.OrdinalIgnoreCase))
{
environmentVariables[key] = string.Empty;
src/Compilers/Core/MSBuildTask/Vbc.cs:28
- The PR title/description calls out migrating
Cscto MSBuild multithreaded execution, but this change also optsVbcinto[MSBuildMultiThreadableTask]. If this is intentional, the PR metadata should be updated to mentionVbcas well (or the change split), so reviewers and release notes match the actual behavior change.
[MSBuildMultiThreadableTask]
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
jjonescz
reviewed
Jul 31, 2026
| "type": "shell", | ||
| "args": [ | ||
| "build", | ||
| "-p:RunAnalyzersDuringBuild=true", |
Member
There was a problem hiding this comment.
Should we set /p:RoslynEnforceCodeStyle=true too?
| if (!File.Exists(reference.ItemSpec)) | ||
| var itemSpec = reference.ItemSpec; | ||
|
|
||
| if (string.IsNullOrEmpty(itemSpec) || !File.Exists(TaskEnvironment.GetFullPath(itemSpec))) |
Member
There was a problem hiding this comment.
Should we use GetFullPathNoThrow here?
| using System.Text; | ||
| using Microsoft.Build.Framework; | ||
| using Microsoft.Build.Utilities; | ||
| using Microsoft.CodeAnalysis.CommandLine; |
Member
There was a problem hiding this comment.
Why isn't GetFullPath in this list?
| ? VisualBasicCommandLineParser.Default.Parse(arguments, Directory.GetCurrentDirectory(), sdkDirectory: null, additionalReferenceDirectories: null).ParseOptions.Features | ||
| : CSharpCommandLineParser.Default.Parse(arguments, baseDirectory: Directory.GetCurrentDirectory(), sdkDirectory: null, additionalReferenceDirectories: null).ParseOptions.Features; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Consider reverting this added blank line.
|
|
||
| internal static StringComparison EnvironmentVariableComparison => RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? | ||
| StringComparison.OrdinalIgnoreCase : | ||
| StringComparison.Ordinal; |
Member
There was a problem hiding this comment.
This seems like a breaking change since we were ignoring case on unix before. Should we document it? (I guess I'm fine with also doing that only if someone hits this.)
| } | ||
|
|
||
| [Fact] | ||
| public void DotNetInPath_SkipsRelativePaths() |
Member
There was a problem hiding this comment.
Would this test fail without the added IsPathFullyQualified check? Don't we need to actually have dotnet.exe in the relative folders to fully exercise that?
| && path[2] == Path.DirectorySeparatorChar | ||
| && IsValidDriveChar(path[0]); | ||
|
|
||
| static bool IsValidDriveChar(char value) |
Member
There was a problem hiding this comment.
Suggested change
| static bool IsValidDriveChar(char value) | |
| static bool isValidDriveChar(char value) |
| using Roslyn.Test.Utilities; | ||
| using Xunit; | ||
| using Xunit.Abstractions; | ||
| using System.Collections.Generic; |
Member
There was a problem hiding this comment.
This looks unused, and also unsorted.
| return CommandLineParser.SplitCommandLineIntoArguments(commandLine, removeHashComments: false).Skip(1); | ||
| } | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Consider reverting this added blank line.
|
/azp run |
|
Commenter does not have sufficient privileges for PR 84701 in repo dotnet/roslyn |
Member
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
3 tasks
Contributor
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (5)
src/Compilers/Core/MSBuildTask/ManagedCompiler.cs:1236
- CheckAllReferencesExistOnDisk now calls TaskEnvironment.GetFullPath(itemSpec) inside File.Exists without any exception filter. For invalid ItemSpec values (e.g., containing invalid path characters), TaskEnvironment.GetFullPath/Path.GetFullPath can throw, which changes the pre-migration behavior (File.Exists returned false) and also contradicts the new regression tests expecting MSB3104 to be logged without throwing.
foreach (ITaskItem reference in References)
{
var itemSpec = reference.ItemSpec;
if (string.IsNullOrEmpty(itemSpec) || !File.Exists(TaskEnvironment.GetFullPath(itemSpec)))
{
success = false;
Log.LogErrorWithCodeFromResources("General_ReferenceDoesNotExist", itemSpec);
}
src/Compilers/Core/Portable/InternalUtilities/PathExtensions.cs:55
- The .NET Framework polyfill for Path.IsPathFullyQualified only recognizes Path.DirectorySeparatorChar (\) as a separator on Windows. Windows paths can also legally use Path.AltDirectorySeparatorChar (/), so entries like "C:/Program Files/dotnet" would be treated as not fully qualified and skipped when scanning PATH.
if (path[0] == Path.DirectorySeparatorChar)
{
// Two leading slashes is UNC or device (\\?\); a single slash followed
// by '?' is \??\, which is equivalent to \\?\. '?' is not legal in a
// drive-relative path, so both forms are fully qualified.
return path[1] == '?' || path[1] == Path.DirectorySeparatorChar;
}
// Otherwise the only fully qualified form is drive + colon + separator (C:\).
// The drive letter is validated to match legacy behavior: "=:\" is the
// default data stream of a file named "=", not a rooted path.
return path.Length >= 3
&& path[1] == ':'
&& path[2] == Path.DirectorySeparatorChar
&& IsValidDriveChar(path[0]);
src/Compilers/Shared/BuildServerConnection.cs:191
- The non-MSBuildTask RunServerBuildRequestAsync wrapper eagerly allocates a full environment snapshot (Environment.GetEnvironmentVariablesTyped()) on every compilation request, even when the server is already running (TryCreateServer is never called). This adds avoidable per-build allocations.
#if !MICROSOFT_CODEANALYSIS_MSBUILD_TASK
internal static Task<BuildResponse> RunServerBuildRequestAsync(
BuildRequest buildRequest,
string pipeName,
string clientDirectory,
ICompilerServerLogger logger,
CancellationToken cancellationToken) =>
RunServerBuildRequestAsync(
buildRequest,
pipeName,
clientDirectory,
Environment.GetEnvironmentVariablesTyped(),
logger,
cancellationToken);
src/Compilers/Core/MSBuildTask/Vbc.cs:28
- This PR is titled "Migrate Csc to multithreaded MSBuild execution" but it also opts Vbc into multithreaded task execution via [MSBuildMultiThreadableTask]. If Vbc is intended to be in-scope, the PR title/description should reflect that; otherwise this attribute should be removed to keep the PR focused on Csc only.
[MSBuildMultiThreadableTask]
global.json:9
- global.json updates the repo SDK/toolset to a .NET 11 preview. This is a broad, repo-wide change that appears unrelated to the MSBuild task multithreading work and will complicate review/diagnosis of any regressions. Consider splitting the SDK bump into a separate PR (or documenting why it is required for this change).
"sdk": {
"version": "11.0.100-preview.5.26302.115",
"allowPrerelease": true,
"rollForward": "patch"
},
"tools": {
"dotnet": "11.0.100-preview.5.26302.115",
"vs": {
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
Contributor
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (4)
src/Compilers/Core/MSBuildTask/ManagedCompiler.cs:1234
CheckAllReferencesExistOnDisknow callsTaskEnvironment.GetFullPath(itemSpec)directly. That path canonicalization can throw (e.g., invalid characters on Windows), which would regress the pre-migration behavior where bad/empty reference items are reported as missing (MSB3104) rather than crashing the task. The file already introducedUtilities.GetFullPathNoThrow(..., TaskEnvironment)for this exact scenario; it should be used here too.
var itemSpec = reference.ItemSpec;
if (string.IsNullOrEmpty(itemSpec) || !File.Exists(TaskEnvironment.GetFullPath(itemSpec)))
{
success = false;
src/Compilers/Shared/BuildServerConnection.cs:563
GetServerEnvironmentVariablesclearsDOTNET_ROOT*variants usingStringComparison.OrdinalIgnoreCase. Environment variable names are case-sensitive on non-Windows, and this method already has an OS-specific comparer available (Environment.EnvironmentVariableComparison). Using the OS-specific comparison avoids unintentionally matching/clearing variables on Unix-like systems.
// Clear DOTNET_ROOT* variables such as DOTNET_ROOT_X64 by setting them to empty,
// as we want to set our own DOTNET_ROOT and avoid conflicts
if (dotNetRoot != null && key.StartsWith(RuntimeHostInfo.DotNetRootEnvironmentName, StringComparison.OrdinalIgnoreCase))
{
environmentVariables[key] = string.Empty;
}
src/Compilers/Core/MSBuildTask/Vbc.cs:30
- The PR title/description say this is about migrating
Cscto multithreaded task execution, but this change also optsVbcinto[MSBuildMultiThreadableTask]. If this is intentional, the PR metadata should be updated to reflect that the VB task is also being migrated; otherwise, consider scoping this PR toCsconly.
/// than vbc.exe. The two should be functionally identical, but the compiler server
/// should be significantly faster with larger projects and have a smaller memory
/// footprint.
/// </summary>
[MSBuildMultiThreadableTask]
public class Vbc : ManagedCompiler
{
global.json:9
- Updating
global.jsonfrom a stable SDK (10.0.301) to a .NET 11 preview SDK affects every developer/CI build of the repo, and it’s not obvious from the diff alone that this repo-wide pinning change is required for the MSBuild-task multithreading work. If the preview SDK is required (e.g., for TaskEnvironment APIs), it would be good to either keep the pin as narrow as possible or document the necessity/compat expectations in the PR description to avoid surprising contributors.
"sdk": {
"version": "11.0.100-preview.5.26302.115",
"allowPrerelease": true,
"rollForward": "patch"
},
"tools": {
"dotnet": "11.0.100-preview.5.26302.115",
"vs": {
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
jasonmalinowski
left a comment
jasonmalinowski
left a comment
Member
There was a problem hiding this comment.
Took a peek to see what this looked like and had a few small comments but nothing significant. There's some of the environment variable dictionary munging I didn't look at in detail so somebody else should give this a better review.
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <AdditionalFiles Include="..\BannedSymbols.MSBuildMT.txt" /> |
Member
There was a problem hiding this comment.
Should we add this in at in a Directory.Build.props up a level?
| internal string GetFullPath(string path) | ||
| { | ||
| var fullPath = taskEnvironment.GetAbsolutePath(path).Value; | ||
| return Path.GetFullPath(fullPath); |
Member
There was a problem hiding this comment.
Is GetFullPath just being used for path normalization then?
| return path; | ||
| } | ||
|
|
||
| internal static string GetFullPathNoThrow(string path, TaskEnvironment taskEnvironment) |
Member
There was a problem hiding this comment.
Should this just be an extension along side the method it's calling?
| && IsValidDriveChar(path[0]); | ||
|
|
||
| static bool IsValidDriveChar(char value) | ||
| => (uint)((value | 0x20) - 'a') <= 'z' - 'a'; |
Member
There was a problem hiding this comment.
Points for cleverness here....
| /// <summary> | ||
| /// Gets the environment variables as a strongly-typed dictionary. | ||
| /// </summary> | ||
| internal static Dictionary<string, string> GetEnvironmentVariablesTyped() |
Member
There was a problem hiding this comment.
Suggested change
| internal static Dictionary<string, string> GetEnvironmentVariablesTyped() | |
| internal static Dictionary<string, string> GetEnvironmentVariablesAsDictionary() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an alternate approach to #84500
Microsoft Reviewers: Open in CodeFlow