-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Split dotnet/runtime-only downlevel LibraryImport support into its own source generator #106436
Conversation
…ribute marshalling model handling
… a separate downlevel generator concept.
…erences to a project with the same simple name as an analyzer in the ref pack
…l downlevel scenarios and run the regular generator for only current-TFM scenarios (like ComInterfaceGenerator).
Tagging subscribers to this area: @dotnet/interop-contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see any issues aside from the JS generator test failures.
...raries/System.Runtime.InteropServices/gen/DownlevelLibraryImportGenerator/LanguageSupport.cs
Outdated
Show resolved
Hide resolved
…'s not considered "unreachable code"
… had in the special COM generator
/ba-g failure is a network timeout that looks generally flaky |
'$(DisableImplicitFrameworkReferences)' == 'true' and | ||
( | ||
'@(Reference->AnyHaveMetadataValue('Identity', 'System.Runtime.InteropServices'))' == 'true' or | ||
'@(ProjectReference->AnyHaveMetadataValue('Identity', '$(CoreLibProject)'))' == 'true' | ||
'@(Reference->AnyHaveMetadataValue('Identity', 'System.Private.CoreLib'))' == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks wrong. System.Private.CoreLib is never a reference as it isn't part of the targeting pack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change requires updating all the solution files as this is adding a new dependency. Can you please do that? The command is .\dotnet.cmd build src\libraries\slngen.proj
.
Please also check if the System.Private.CoreLib slns need to be updated.
Condition="'$(EnableLibraryImportGenerator)' == '' and | ||
( | ||
'$(IsSourceProject)' == 'true' or | ||
'$(IsTestProject)' == 'true' or | ||
'$(IsTestSupportProject)' == 'true' | ||
) and | ||
'$(MSBuildProjectExtension)' == '.csproj' and | ||
!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', '$(NetCoreAppMinimum)'))" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be $(TargetFrameworkIdentifier)' != '.NETCoreApp'
as we shouldn't support targeting an out-of-support .NETCoreApp TFM.
@@ -1,51 +1,56 @@ | |||
<Project> | |||
|
|||
<PropertyGroup> | |||
<!-- Enable LibraryImportGenerator for CoreLib. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this already existed before this change but it feels weird to special case CoreLib in here. Can we move this property into the shared CoreLib project instead?
@ViktorHofer I've addressed your feedback in #106827 |
Instead of shipping unusable downlevel support in LibraryImportGenerator and having to maintain how it works in combination with the non-downlevel support that we actually expect users to use, split the generator into two separate generators:
The Downlevel support only needs to support standard and NETFX as all .NETCoreApp TFMs we target for downlevel support all ship LibraryImportGenerator already (and they build using the version that shipped with the TFM).
This PR also shares the managed->unmanaged stub generation between LibraryImportGenerator and ComInterfaceGenerator to reuse more code cleanly.
The MSBuild changes in Directory.Build.targets help fix a problem where the TFM-included generators would be removed in the generator unit test projects, which also reference the projects directly as regular (non-analyzer) references. This would fail sporadically when working locally before this change, but the changes in generators.targets caused the intermittent failure to become consistent.