search

dotnet / format

Home for the dotnet-format command
MIT License
1.92k stars 174 forks source link

Fix source build poison leak of Microsoft.Bcl.AsyncInterfaces #1975

Closed mthalman closed 10 months ago

mthalman commented 10 months ago

Related to https://github.com/dotnet/source-build/issues/3670

This fixes a source build poison leak of Microsoft.Bcl.AsyncInterfaces that comes from this repo. This means the 7.0.0 version of Microsoft.Bcl.AsyncInterfaces defined in https://github.com/dotnet/source-build-reference-packages (SBRP) is leaking into the SDK that is produced by source build. That is a reference assembly and it shouldn't be leaked into the SDK output.

This is caused by a transitive dependency from Microsoft.CodeAnalysis.Workspaces.MSBuild.4.8.0-3.23510.8 to Microsoft.Bcl.AsyncInterfaces.7.0.0. So when loading Microsoft.CodeAnalysis.Workspaces.MSBuild, it also ends up loading Microsoft.Bcl.AsyncInterfaces from the SBRP nuget path.

To fix this, I've added an explicit reference to Microsoft.Bcl.AsyncInterfaces to ensure it uses the latest version, overriding the older version from the transitive dependency.

JoeRobich commented 10 months ago

Would it make sense to enable transitive version pinning for these types of issues?

ViktorHofer commented 10 months ago

When we enabled transitive pinning in Arcade, we had some fallout, i.e. SBRPs vs N-1 vs N dependencies. Do you have the possibility to test these changes at the consumption point?

mthalman commented 10 months ago

When we enabled transitive pinning in Arcade, we had some fallout, i.e. SBRPs vs N-1 vs N dependencies. Do you have the possibility to test these changes at the consumption point?

Yeah, I'll run a full build on these changes in the VMR.

ViktorHofer commented 10 months ago

One more: https://github.com/dotnet/format/blob/bb8884a00e4ea78a81c1acf0d0544e0711cdf8cf/tests/dotnet-format.UnitTests.csproj#L27-L31C70

It's nice to finally be able to do this clean-up.

ViktorHofer commented 10 months ago

I'm not sure but I hope these changes get forward ported into main.

JoeRobich commented 10 months ago

I'm not sure but I hope these changes get forward ported into main.

Looks like we will need to update the roslyn-tools merge config. https://github.com/dotnet/roslyn-tools/blob/main/src/GitHubCreateMergePRs/config.xml#L96-L104. Opened https://github.com/dotnet/roslyn-tools/pull/1368/files

mthalman commented 10 months ago

VMR pipeline ran clean with these changes. Confirmed that the reference package doesn't show up in the SDK output.

ViktorHofer commented 10 months ago

Unfortunately I don't have merge permissions. Someone else needs to approve and hit the button.

NikolaMilosavljevic commented 10 months ago

VMR pipeline ran clean with these changes. Confirmed that the reference package doesn't show up in the SDK output.

Same SBRP binary was also showing in dotnet-format nupkg. @mthalman I presume that instance was also fixed - SDK gets dotnet-format content via this nupkg.

mthalman commented 10 months ago

VMR pipeline ran clean with these changes. Confirmed that the reference package doesn't show up in the SDK output.

Same SBRP binary was also showing in dotnet-format nupkg. @mthalman I presume that instance was also fixed - SDK gets dotnet-format content via this nupkg.

Right. That's what causes it to flow into the SDK.