Skip to content

Move lots of shared code to Microsoft.Build.Framework#13364

Merged
JanProvaznik merged 28 commits intomainfrom
dev/dustinca/isolate-taskhost-followup
Mar 12, 2026
Merged

Move lots of shared code to Microsoft.Build.Framework#13364
JanProvaznik merged 28 commits intomainfrom
dev/dustinca/isolate-taskhost-followup

Conversation

@DustinCampbell
Copy link
Member

This is a big change, but it was created methodically and is highly mechanical. I highly recommend reviewing commit-by-commit.

The pull request is a follow up to #13232 that cleans up shared code and moves some core shared files to Microsoft.Build.Framework. Pruning all shared files in MSBuild is a long-term effort, so this change targets some of the more problematic types that contain state that is intended to be shared.

Here is a summary of the changes:

  • Remove #if'd code paths that are dead following msbuild#13232.
  • Move all IFileSystem types to Framework. Note that this means that it is now impossible for FileSystems.Default to diverge across binaries.
  • Move EscapingUtilities to Framework. This ensures that the static dictionary cache is shared across binaries.
  • Move FileUtilitiesRegex to Framework.
  • Split ExceptionHandling up into members into two categories:
    • Members that validate Exception objects move to Framework.
    • Members for dumping exceptions have moved to DebugUtils, which is still shared.
  • Move the rest of FileUtilities to Framework. This contains a lot of shared state that is now properly shared instead of being created per-binary.
  • Move files that are compiled into Microsoft.Build.Framework from the shared folder into Framework.
  • Rationalize FileUtilities.GetIsFileSystemCaseSensitive() to limit writing a file to the file system.

Now that all .NET 3.5 code has been consolidated in MSBuildTaskHost, the CLR2COMPATIBILITY conditional compiler constant can be removed entirely from the code base.
Now that all .NET 3.5 code has been consolidated in MSBuildTaskHost, the TASKHOST conditional compiler constant can be removed entirely from the code base.
Now that all .NET 3.5 code has been consolidated in MSBuildTaskHost, the NET35 conditionally compiled code can be removed entirely -- except for StringTools, which compiles for .NET 3.5.
Move FileUtilities.IsPathTooLong to FrameworkFileUtilities.IsPathTooLong.
- Move code from src/Shared/FileSystem to Microsoft.Build.Framework/FileSystem
- Move "Shared.PathTooLong" resource string from Strings.Shared.resx to Microsoft.Build.Framework/Resources/SR.resx. (and move translated strings.)
- Remove Shared/FileSystemSources.proj from Microsoft.Build.Engine.OM.UnitTests, Microsoft.Build, Microsoft.Build.Tasks, Microsoft.Build.Utilities, and MSBuild
- Delete Shared/FileSystemSources.proj
- Move src/Shared/EscapingUtilities.cs to src/Framework/EscapingUtilities.cs
- Add project reference from Microsoft.Build.Framework to StringTools.csproj. This dependency is needed by EscapingUtilities.
- Remove all project reference to StringTools.csproj from projects that reference it transitively through Microsoft.Build.Framework.
- Move src/Shared/FileUtilitiesRegex.cs to src/Framework/FileUtilitiesRegex.cs
This change moves all of the implementations of exception dumping members from ExceptionHandling to DebugUtils. Member stubs have been left in ExceptionHandling that delegate to the implementations in DebugUtils. This will be cleaned up in a later commit.

Note that references to the BUILDINGAPPXTASKS conditional compilation symbol have been removed from ExceptionHandling. BUILDINGAPPXTASKS isn't set anywhere within the msbuild repo.
- Add src/Framework/ExceptionHandling.cs containing FrameworkExceptionHandling static class.
- Move implementations of members from src/Shared/ExceptionHandling.cs to src/Framework/ExceptionHandling.cs and leave stubs behind. The stubs will be cleaned up in a later commit.
- Remove used ExceptionHandling.GetXmlLineAndColumn(Exception) method.
- Add src/Framework/FileUtilities_TemFiles.cs containing a new partial part of the FrameworkFileUtilities static class.
- Move implementations of members from src/Shared/TempFileUtilities.cs to src/Framework/FileUtilities_TemFiles.cs and leave stubs behind. The stubs will be cleaned up in a later commit.
- Remove TempWorkingDirectory nested type from src/Shared/TempFileUtilities.cs and update callers to use the type nested in FrameworkFileUtilities.
- Add "FailedCreatingTempFile" string resource, taking care to copy the translated strings from "Shared.FailedCreatingTempFile"
- Move StreamExtensions from src/Shared/FileUtilities.cs to src/Framework/Polyfills/StreamExtensions.cs
- Add 'NewPath' type alias for Microsoft.IO.Path when compiling for net472. This will facilitate the process of copying members from `src/Shared/FileUtilities.cs` that all use System.IO.Path.
- Move implementations of all members (except ExecutingAssemblyPath) from src/Shared/FileUtilities.cs to src/Framework/FileUtilities.cs and leave stubs behind. The stubs will be cleaned up in a later commit.
- Add "DebugPathTooLong" and "InvalidGetPathOfFileAboveParameter" string resources, taking care to copy the translated strings.
- Add 'Path' and 'NewPath' aliases to avoid conflicts between System.IO.Path and Microsoft.IO.Path when compiling for net472 and netstandard2.0. This ensures that members copied from src/Shared/FileUtilities.cs continues to call into System.IO.Path, which is necessary to throw expected path exceptions when running on .NET Framework.
- Add src/Framework/ItemSpecModifiers.cs containing a new ItemSpecModifiers static class.
- Move implementations of members from src/Shared/Modifiers.cs to src/Framework/ItemSpecModifiers.cs and leave stubs behind. The stubs will be cleaned up in a later commit.
- Add "InvalidFilespecForTransform" string resource, taking care to copy the translated strings from "Shared.InvalidFilespecForTransform"
- Move FileUtilities.ExecutingAssemblyPath property to BuildEnvironemtnHelper. This property depends on the code being compiled into the binary that calls it. So, moving this call to another file allows src/Shared/FileUtilities.cs to be deleted.
- Update all usages of Microsoft.Build.Shared.FileUtilities.ItemSpecModifiers to Microsoft.Build.Framework.ItemSpecModifiers.
- Remove src/Shared/Modifiers.cs.
- Remove src/Shared/FileUtilities.cs and src/Shared/TempFileUtilities.cs from all project files.
- Rename FrameworkFileUtilities to FileUtilities.
- Delete src/Shared/FileUtilities.cs and src/Shared/TempFileUtilities.cs
- Fix all remaining usages of Microsoft.Build.Shared.FileUtilities to point to Microsoft.Build.Framework.FileUtilities.
- Clean up using directives
Update usages of ExceptionHandling members that were delegating to DebugUtills to just use DebugUtils directly.
- Remove src/Shared/ExceptionHandling.cs from all project files.
- Rename FrameworkExceptionHandling to ExceptionHandling.
- Delete src/Shared/ExceptionHandling.cs
- Fix all remaining usages of Microsoft.Build.Shared.ExceptionHandling to point to Microsoft.Build.Framework.ExceptionHandling.
- Clean up using directives
This type was compiled from src/Shared/Constants.cs but it wasn't included in any other projects. This change removes it completely from src/Shared and includes it directly in src/Framework.
This type was compiled from src/Shared/EnvironmentUtilities.cs but it wasn't included in any other projects. This change removes it completely from src/Shared and includes it directly in src/Framework. In addition, it has been moved to the Microsoft.Build.Framework namespace.
These types were compiled from src/Shared/BinaryReaderExtensions.cs and src/Shared/BinaryWriterExtensions.cs but they weren't included in any other projects. This change removes them completely from src/Shared and includes them directly in src/Framework. In addition, they have been moved to the Microsoft.Build.Framework namespace.
This type was compiled from src/Shared/IMSBuildElementLocation.cs but it wasn't included in any other projects. This change removes it completely from src/Shared and includes it directly in src/Framework. It has to remain in the Microsoft.Build.Shared namespace, since it's a public API.
This method creates a temp file to check whether the file system is case sensitive or not. Since the check relies on writing a file to disk, this change caches the result (like the .NET Runtime does) and converts the GetIsFileSystemCaseSensitive() method to a property called, IsFileSystemCaseSensitive.
Copilot AI review requested due to automatic review settings March 10, 2026 23:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@DustinCampbell
Copy link
Member Author

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

This should be accompanied by an "Achievement Unlocked"

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything makes sense :shipit: thanks, looking for second reviewer

@JanProvaznik
Copy link
Member

Note: this makes Framework depend on StringTools, I think that's fine, but flagging for @rainersigwald

@rainersigwald
Copy link
Member

A Framework->StringTools dependency should be fine, but I'm paranoid so I kicked off an experimental insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/716860.

@DustinCampbell
Copy link
Member Author

A Framework->StringTools dependency should be fine, but I'm paranoid so I kicked off an experimental insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/716860.

Yeah, StringTools gets pulled in by the first thing that calls into the EscapingUtilities, so I don't expect this to be a problem either. Stranger things have happened though! 😄

@rainersigwald
Copy link
Member

We had a bad fire drill a while ago where an API NuGet called got a new assembly dependency (that the DLL already had, just not this specific method!) and broke a bunch of folks using the default AzDO configuration. This at least didn't break that thing, since we have a regression test

<PackageDownload Include="NuGet.CommandLine" Version="[$(NuGetCommandLinePackageVersion)]" />

And the exp insertion looks fine. I'm good with this change as soon as we unblock insertions (VS repo self-build test is currently broken by a not-MSBuild change, but that is a required scenario test for us).

@DustinCampbell
Copy link
Member Author

We had a bad fire drill a while ago where an API NuGet called got a new assembly dependency (that the DLL already had, just not this specific method!) and broke a bunch of folks using the default AzDO configuration.

Yeah, that's exactly the sort of "stranger thing" I was thinking of. 😄

@JanProvaznik JanProvaznik merged commit df3c977 into main Mar 12, 2026
10 checks passed
@JanProvaznik JanProvaznik deleted the dev/dustinca/isolate-taskhost-followup branch March 12, 2026 15:41
JanProvaznik added a commit to JanProvaznik/msbuild that referenced this pull request Mar 13, 2026
After FileUtilities moved from Shared to Framework (dotnet#13364), all assemblies
share the same cacheDirectory static. ClearCacheDirectory() in XMake.cs now
deletes the same directory that ProjectImportsCollector uses for its temporary
archive, causing 'Could not find a part of the path' errors during binlog
finalization.

Fix: Store the temporary ProjectImports archive in TempFileDirectory (the
parent) rather than GetCacheDirectory() (the child that gets wiped). Also
make DeleteArchive resilient to the archive already being cleaned up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants