Check in shim typeforwards and remove the dependency on the underlying targeting packs #79147
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsFixes #65997
|
Fixes dotnet#65997 GenFacade is a tool that inspects a contract assembly's public API surface safe area, compares it with a passed in assembly closure and emits a C# source file with type forwards in it so that the satisfied API can be forwarded from the contract to the "implementation". GenFacades is used to construct the .NETFramework and the .NETStandard shim assemblies which live under src/libraries/shims/. The generated source file isn't checked into the tree, it's placed into the project's intermediate output path. IMO it would help if we check these files into the tree so that any changes that impact shims - intentional or unintentional - are trackable. As an example, I just recently refactored how the shims are structured under src/libraries/shims and unintentionally removed a type forward from an assembly. I only noticed this regression by pure luck later. The priority of this issue just rose because source build plans to remove their .NET Framework reference assembly packages during the .NET 8 release. That means that we can't automatically generate the type forwards during source-build. I will work on removing the dependency and check the type forwards in instead.
ViktorHofer
force-pushed
the
ShimsCheckin
branch
from
926da54
to
c9d95df
Compare
Also remove the System.Xml special runtime project which isn't necessary anymore as the compiler now allows type forwards to Obsolete types with error=true.
ViktorHofer
changed the title
This reverts commit 2f5d410.
| <GenFacadesForceZeroVersionSeeds Condition="'$(MSBuildProjectName)' != 'netstandard'">true</GenFacadesForceZeroVersionSeeds> | ||
| <!-- Ensure the .NET Framework shims reference the lowest possible version of dependencies since | ||
| those do not all ship as part of the framework and we don't want to force apps to reference the | ||
| latest packages. netstandard.dll doesn't need to do this since it has no dangling dependencies. --> |
There was a problem hiding this comment.
Why do we not want the latest package to he referenced?
There was a problem hiding this comment.
This is already the same in main. Or are you asking about why we are doing this generally? IMO the comment captures that nicely. We make sure that shims don't depend on the very latest packages and instead work with any version. When you look at any of the shims in ilspy, i.e. mscorlib you will see that all dependency versions in its metadata are 0.0.0.0.
| <!-- Opt out of some features which are on by default. --> | ||
| <EnableLibraryImportGenerator>false</EnableLibraryImportGenerator> | ||
| <ApiCompatValidateAssemblies>false</ApiCompatValidateAssemblies> | ||
| <ILLinkTrimAssembly>false</ILLinkTrimAssembly> | ||
| <AddOSPlatformAttributes>false</AddOSPlatformAttributes> | ||
| <!-- Allow shim ref projects to reference source projects which is required for referencing shared | ||
| framework dependencies. --> | ||
| <SkipValidateReferenceAssemblyProjectReferences Condition="'$(IsReferenceAssemblyProject)' == 'true'">true</SkipValidateReferenceAssemblyProjectReferences> |
There was a problem hiding this comment.
I assume this is related to what you told me yesterday that refs don't necessarily expose all the public APIs that exist in src.
There was a problem hiding this comment.
This was also already in main just in a different location:
runtime/src/libraries/shims/ref/Directory.Build.props
Lines 5 to 6 in 39c02d9
Usually our reference source project must not have a TFM with a $(TargetOS) in it. In this case we override that validation as the shim refernce source projects are a bit special. That could be cleaned-up in the future but is unrelated to this PR.
|
|
||
| <PropertyGroup> | ||
| <AssemblyVersion>4.0.0.0</AssemblyVersion> | ||
| <StrongNameKeyId>ECMA</StrongNameKeyId> |
There was a problem hiding this comment.
I see we use Microsoft, MicrosoftShared or ECMA values. What is the purpose of using difference values? How do you decide which one to use?
There was a problem hiding this comment.
These assemblies exist in the .NET Framework runtime on Windows machines and need to have the same identity. The strong name key is part of the assembly's identity, same as the version. We just pick whatever strong name key was chosen for the assembly in the .NET Framework runtime. Note that this also already existed in main, I didn't change any of these values, just moved them. Git diff unfortunately doesn't help here.
ghost
locked as resolved and limited conversation to collaborators
Fixes #65997
This also changes the folder structure of shims to make them fit into the existing libraries layout (projectname/ref, projectname/src). That makes sharing msbuild data and C# files easier and enables the use of default compile items.
The shim projects now also define their dependencies explicitly instead of just receiving all references.