Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SB ref assembly issue for System.ComponentModel.Composition #4675

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

mthalman
Copy link
Member

This fixes an issue with System.ComponentModel.Composition, as described in dotnet/source-build#3599, that is showing up as a reference assembly in the source-built SDK.

The reason a reference assembly exists in this case is because vstest has a reference to the 7.0.0 version of System.ComponentModel.Composition. When building with source-build, it loads that reference from SBRP (which only contains reference assemblies) in order to fulfill compile time references. The problem is that the assembly is also getting included in the output. This should have been detected by poison leak detection but that doesn't yet handle reference assemblies.

It's not known whether the existence of this ref assembly causes a functional issue. But it is known that the source-built 7.0 SDK doesn't define this as a ref assembly but rather as an implementation assembly. So to maintain parity with 7.0 and avoid potential risk, it's best to ensure this is represented as an implementation assembly in the output.

This removes the special configuration that was done for source-build so that it simply reuses the existing SystemComponentModelCompositionVersion property. This property will get overridden by source-build as a result of the inclusion of System.ComponentModel.Composition to Version.Details.xml.

@mthalman
Copy link
Member Author

cc @MichaelSimons for review

@Evangelink
Copy link
Member

Thanks for the PR @mthalman, would you mind fixing the SB error:

5 new pre-builts discovered! Detailed usage report can be found at /__w/1/s/artifacts/source-build/self/prebuilt-report/baseline-comparison.xml.
See https://aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them.
Package IDs are:
Microsoft.NETCore.Platforms.2.0.0
System.ComponentModel.Composition.4.5.0
System.Security.AccessControl.4.5.0
System.Security.Permissions.4.5.0
System.Security.Principal.Windows.4.5.0

@mthalman
Copy link
Member Author

mthalman commented Aug 28, 2023

@Evangelink - Is there any reason that SystemComponentModelCompositionVersion has to be set to such an old version: 4.5.0? Can we upgrade that to 7.0.0?

@Evangelink
Copy link
Member

TP is used in many different scenarios/contexts and so any package update is always a concern as it could be breaking users. In this particular context, I have no idea what's the impact of changing the composition dll...

@mthalman
Copy link
Member Author

Looks like the repo has a dependency on netcoreapp3.1 TFM which System.ComponentModel.Composition.7.0.0 does not support. So we can't upgrade the default version to 7.0.0. That means I'll need to add System.ComponentModel.Composition.4.5.0 to SBRP to resolve the prebuilts.

@mthalman mthalman requested a review from MichaelSimons August 30, 2023 19:20
@mthalman
Copy link
Member Author

@Evangelink - Builds are passing now.

@Evangelink Evangelink merged commit c2d75c9 into microsoft:main Aug 31, 2023
@mthalman mthalman deleted the sb3599 branch August 31, 2023 12:52
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.

3 participants