Add missing Microsoft.IO.Redist PackageReference to Microsoft.Build.Framework#13785
Add missing Microsoft.IO.Redist PackageReference to Microsoft.Build.Framework#13785T-Gro wants to merge 4 commits into
Conversation
After dotnet#13364 and dotnet#13370 moved FileSystem code (WindowsFileSystem, ManagedFileSystem, FileUtilities, FrameworkLocationHelper) into Microsoft.Build.Framework, the assembly acquired a runtime dependency on Microsoft.IO.Redist under net472. The compile-time reference was satisfied by the frozen-version hack in Directory.Build.targets, but no PackageReference existed, so the NuGet package never declared the dependency — breaking any out-of-process host that deploys Framework without IO.Redist (e.g. fsc.exe). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds the missing Microsoft.IO.Redist NuGet PackageReference to Microsoft.Build.Framework.csproj. After PRs #13364 and #13370 moved shared file-system code into Framework, the assembly's IL began referencing Microsoft.IO.* types, but the package wasn't declared as a NuGet dependency (only injected via a raw <Reference> in Directory.Build.targets), causing runtime Assembly not found failures in consumers such as fsc.exe.
Changes:
- Add a conditional
PackageReferencetoMicrosoft.IO.RedistinMicrosoft.Build.Framework.csproj, gated onFeatureMSIORedist, matching the pattern already used byBuild.csproj,Tasks.csproj,Utilities.csproj, andMSBuild.csproj.
There was a problem hiding this comment.
✅ Looks Good
Reviewed dimensions: Backwards Compatibility, ChangeWave Discipline, Performance, Test Coverage, Dependency Management, SDK Integration, Cross-Platform Correctness, Code Simplification, Build Infrastructure.
Summary: This is a correct and well-scoped fix. The Microsoft.Build.Framework assembly genuinely uses Microsoft.IO.Redist types on net472 (via #if FEATURE_MSIOREDIST guards in ManagedFileSystem.cs, WindowsFileSystem.cs, FileMatcher.cs, FileUtilities.cs, AbsolutePath.cs), but was relying on the ReplaceCompileReferencesWithOlderMaintenancePackagesVersions hack for compile-time resolution only — meaning the NuGet .nupkg never declared the runtime dependency.
No issues found:
- ✅ Condition (
FeatureMSIORedist) is correct — only true for net472 in non-source-build, matching other projects - ✅ No
PrivateAssets— correct, since this is a runtime dependency that must flow to package consumers (e.g. out-of-proc hosts likefsc.exe) - ✅ Version is centrally managed via
Directory.Packages.props - ✅ No breaking change — this adds a missing dependency declaration, fixing broken deployments
- ✅ Source-build is unaffected (FeatureMSIORedist is false)
- ✅ No ChangeWave needed — this is a packaging fix, not a behavioral change
One minor cosmetic observation left as an inline comment (non-blocking).
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #13785
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #13785
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (on open) for issue #13785 · ● 2M
rainersigwald
left a comment
There was a problem hiding this comment.
I'm not sure we should do this. Can we get fsc on the shared MSBuild environment instead of redistributing one? In general we don't support using our DLLs outside of an MSBuild "installation", VS or .NET SDK.
|
Specifically I'd like to avoid pushing unnecessary dependencies to tasks and API consumers that reference MSBuild but (correctly) don't redistribute it. |
|
This has been spotted by validation testing of VS in a niche scenario. |
|
Actually, I think this important. I ran into a problem in the #13653 that required adding a package reference for Microsoft.IO.Redist to Microsoft.Build.Framework here: efd5fda. Microsoft.Build.Framework already references Microsoft.IO.Redist and uses types in it. However, it doesn't reference Microsoft.IO.Redist directly. Instead, it gets an incorrect reference to the frozen version of Microsoft.IO.Redist through https://github.com/dotnet/msbuild/blob/main/src/Directory.Build.targets. While working #13653, I ran into a problem where the |
|
@rainersigwald Do I get your crux right that this is desirable behavior to fail at runtime if you redistribute framework package. (possibly it could fail with a more diagnosable message). And that the coordinator and fsc case are very niche legitimate cases which must workaround? Or is the redistributing concern central to your opposition? can we close or is there more to discuss? |
Just to add my 2 cents, I think this is broken and the reference should be added. The package reference already exists in essentially every project except for msbuild/src/Build/Microsoft.Build.csproj Line 37 in ccee89d msbuild/src/MSBuild/MSBuild.csproj Line 141 in ccee89d All of those projects reference (FWIW, I would expect that only |
|
I agree that we should move the dependency to Framework, internally to the MSBuild repo. My concern is our old friend the "targeting pack for MSBuild" type of problem -- ideally we wouldn't accidentally lead folks who create tasks or apps that use our API to have dependencies on Microsoft.IO.Redist, impeding the ability to get it from the folder next to the loaded MSBuild. That would be more about the public-facing NuGet dependencies, which are more than they need to be right now since they describe both runtime dependencies and build-time ones. I'm ok with making this change and avoiding the longer-term fix--Dustin's right that since all of our other packages depend on it we're likely not making things worse.
Agreed but we can push to a follow-up PR. |
Microsoft.Build.Framework now owns the direct PackageReference, so all consumers (Build, Utilities, Tasks, MSBuild, and their tests) get Microsoft.IO.Redist transitively through their ProjectReference to Framework. Drop the redundant direct references. Also removes the dead 'PackageReference Update=Microsoft.IO.Redist PrivateAssets=all' line in Utilities.csproj's .NETStandard ItemGroup, since FeatureMSIORedist is only set on .NETFramework. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
I lied, Copilot did this quickly, pushing. |
JanProvaznik
left a comment
There was a problem hiding this comment.
we're likely not making things worse
and fixing Tomas's problem :)
Problem
After #13364 and #13370 moved shared code into
Microsoft.Build.Framework, the assembly acquired a runtime dependency onMicrosoft.IO.Redist(net472) without declaring it as a NuGet package dependency.WindowsFileSystem.cs,ManagedFileSystem.cs,FileUtilities.csMicrosoft.IO.File,Microsoft.IO.Path,Microsoft.IO.DirectoryFrameworkLocationHelper.csGetPathToDotNetFramework) that triggersFileSystems.Default.FileExists→Microsoft.IO.File.Existsnow lives in FrameworkThe compile succeeded because
Directory.Build.targetsinjects a raw<Reference>to the frozen IO.Redist DLL — but a raw Reference does not flow as a NuGet dependency.Identified via fsc.exe (F# compiler) crashing at VS developer prompt with
Assembly not found: Microsoft.IO.Redistafter updating MSBuild packages from 18.2 to 18.4.Fix
Add
<PackageReference Include="Microsoft.IO.Redist" />to Framework.csproj, matching Build.csproj, Tasks.csproj, Utilities.csproj, and MSBuild.csproj.