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

Check in shim typeforwards and remove the dependency on the underlying targeting packs #79147

Merged
merged 6 commits into from
Mar 28, 2023

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Dec 2, 2022

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.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

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.

@ghost
Copy link

ghost commented Dec 2, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #65997

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

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.
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

I'm supportive of checking in the forwards, but I think we need to think about the workflow of this and expectations for how developers interact with these sources. Consider adding a readme describing the interaction.

src/libraries/shims/Directory.Build.props Outdated Show resolved Hide resolved
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 ViktorHofer changed the title Make .NET Framework and .NET Standard facade changes trackable Check in shim typeforwards and remove the dependency on the underlying targeting packs Mar 27, 2023
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

I mostly have questions to learn about this.

Did the GenFacade tool generate both the cs files and csproj files under shims?

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. -->
Copy link
Member

Choose a reason for hiding this comment

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

Why do we not want the latest package to he referenced?

Copy link
Member Author

Choose a reason for hiding this comment

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

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>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was also already in main just in a different location:

<!-- Allow shim ref projects to reference source projects. -->
<SkipValidateReferenceAssemblyProjectReferences>true</SkipValidateReferenceAssemblyProjectReferences>

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>
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Assuming the CI does not contain related failures, LGTM.

@ViktorHofer ViktorHofer merged commit c0cca33 into dotnet:main Mar 28, 2023
@ViktorHofer ViktorHofer deleted the ShimsCheckin branch March 28, 2023 15:32
@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make .NET Framework and .NET Standard facade changes trackable
3 participants