Include LibraryImportGenerator in WindowsDesktop transport package

[ViktorHofer]

Include the LibraryImportGenerator in order for WinForms's System.Drawing.Common library to have access to the source generator which depends on it for its net6.0 build.

Unblocks dotnet/winforms#8633

On the consuming side, the following msbuild code adds the source generator to the compiler:

  <ItemGroup Condition="'$(TargetFramework)' == 'net6.0'">
    <PackageDownload Include="Microsoft.Internal.Runtime.WindowsDesktop.Transport" Version="[$(MicrosoftInternalRuntimeWindowsDesktopTransportPackageVersion)]" />
    <Analyzer Include="$(NuGetPackageRoot)microsoft.internal.runtime.windowsdesktop.transport\$(MicrosoftInternalRuntimeWindowsDesktopTransportPackageVersion)\tools\Microsoft.Interop.LibraryImportGenerator.dll" />
    <Analyzer Include="$(NuGetPackageRoot)microsoft.internal.runtime.windowsdesktop.transport\$(MicrosoftInternalRuntimeWindowsDesktopTransportPackageVersion)\tools\Microsoft.Interop.SourceGeneration.dll" />
  </ItemGroup>

[ghost]

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

Issue Details

---

Include the LibraryImportGenerator in order for WinForms's System.Drawing.Common library to have access to the source generator which depends on it for its net6.0 build.

Unblocks dotnet/winforms#8633

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	area-Infrastructure-libraries
Milestone:	-

[stephentoub]

which depends on it for its net6.0 build

Doesn't the LibraryImport generator generate code that relies on types which only ship in .NET 7+? I didn't think it was written to accomodate downlevel targets; is it?

[ViktorHofer]

Inside dotnet/runtime, netstandard2.0, net462 and net6.0 TFMs already consume the LibraryImportGenerator. Those targets don't have the required API available. Still, it works via API polyfill:

runtime/eng/generators.targets

Lines 37 to 42 in 65f5ad0

	<ItemGroup Condition="'@(EnabledGenerators)' != '' and
	@(EnabledGenerators->AnyHaveMetadataValue('Identity', 'LibraryImportGenerator')) and
	!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net7.0'))">
	<Compile Include="$(CoreLibSharedDir)System\Runtime\InteropServices\LibraryImportAttribute.cs" />
	<Compile Include="$(CoreLibSharedDir)System\Runtime\InteropServices\StringMarshalling.cs" />
	</ItemGroup>

We would continue to provide the polyfill code inside the winforms repository.

[stephentoub]

Inside dotnet/runtime, netstandard2.0, net462 and net6.0 TFMs already consume the LibraryImportGenerator. Those targets don't have the required API available. Still, it works via API polyfill:

The generator emits calls to methods like MemoryMarshal.CreateSpan that don't exist in netstandard2.0 and .NET Framework 4.6.2 and that can't be polyfilled. How do those work?

It looks like for System.Drawing.Common in particular the netstandard2.0 build is an auto-generated PlatformNotSupported implementation, and the .NET Framework 4.6.2 build is just a type-forwarding facade. I guess we're banking on all of the APIs the generator generates code to use happened to have been shipped in .NET 6 as well?

[ViktorHofer]

From what I see, the LibraryImportGenerator dynamically figures out the TFM and adapts the emitted code based on that:

runtime/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/AnalyzerConfigOptionsExtensions.cs

Lines 40 to 47 in 65f5ad0

	".NETStandard" => TargetFramework.Standard,
	".NETCoreApp" when version is not null && version < FirstNonCoreVersion => TargetFramework.Core,
	".NETCoreApp" => TargetFramework.Net,
	// If the TFM is not specified, we'll infer it from this assembly.
	// Since we only ship this assembly as part of the Microsoft.NETCore.App TFM,
	// the down-level support only matters for the repo where this project is built.
	// In all other cases, we will only be used from the TFM with the matching version as our assembly.
	null => TargetFramework.Net,

I'm not sure if this code path is only exercised in an analyzer or also in the source generator though. @elinor-fung, @jkoritzinsky can you please answer Stephen's above question?

It looks like for System.Drawing.Common in particular the netstandard2.0 build is an auto-generated PlatformNotSupported implementation, and the .NET Framework 4.6.2 build is just a type-forwarding facade. I guess we're banking on all of the APIs the generator generates code to use happened to have been shipped in .NET 6 as well?

The LibraryImportGenerator runs for all source projects in dotnet/runtime, not just System.Drawing.Common. While Drawing doesn't depend on it for anything than .NETCoreApp, I'm sure there are other libraries that depend on it for non .NETCoreApp TFMs.

[jkotas]

LibraryImportGenerator ships and versions as part of the NetCoreApp. We do not support mixing and matching LibraryImportGenerator and NetCoreApp versions.

Can WinForms follow the supported pattern and always use the LibraryImportGenerator from the matching NetCoreApp version, like any other project out there?

[ViktorHofer]

LibraryImportGenerator ships and versions as part of the NetCoreApp. We do not support mixing and matching LibraryImportGenerator and NetCoreApp versions.

While our product doesn't support this, we in runtime feed the same source generator build output to the compiler in all of our project' inner builds (all TFMs). We do have explicit infrastructure in both the LibraryImportGenerator project and its corresponding msbuild code to make that integration work. IIRC, the team even considered shipping the generator out-of-band as a nuget package.

Can WinForms follow the supported pattern and always use the LibraryImportGenerator from the matching NetCoreApp version, like any other project out there?

System.Drawing.Common supports net6.0 and unfortunately the LibraryImportGenerator API and generator aren't part of .NET 6 ('s targeting pack).

[jkotas]

The alternative would be to ifdef the PInvoke definitions, like what all other multi-targeting projects outside dotnet/runtime repo have to today. For example; https://github.com/dotnet/aspnetcore/blob/main/src/DataProtection/Cryptography.Internal/src/SafeHandles/SafeLibraryHandle.cs#L131-L137

If we want to take this route, the comments like these should be updated:

• runtime/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/AnalyzerConfigOptionsExtensions.cs

  Line 45 in 65f5ad0

  // the down-level support only matters for the repo where this project is built.

runtime/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/AnalyzerConfigOptionsExtensions.cs

Lines 29 to 30 in 65f5ad0

	// - In the dotnet/runtime repository.
	// - In a .NET SDK for the same TFM that matches the version of this assembly.

Also, we will need to be careful about this one-off case when evolving the PInvoke source generator. It is likely that we are going to see breaks in WinForms repo every once in a while.

[jkoritzinsky]

The downlevel support in LibraryImportGenerator is meant to simplify the dotnet/runtime build. It is explicitly a non-supported scenario outside the runtime. If we start shipping it, even just to other repos, then we need to consider that fact when evolving the generator. For example, we can remove fallback marshaller implementations when dotnet/runtime stops building for .NET 6 in a few years, but if we start shipping that downlevel support, it becomes a true breaking change to do that.

[jkoritzinsky]

If System.Drawing.Common is moving out of the repo, it should move away from using LibraryImport downlevel and follow Jan's suggestion of ifdefs.

[ViktorHofer]

The alternative would be to ifdef the PInvoke definitions, like what all other multi-targeting projects outside dotnet/runtime repo have to today.

Thanks for providing some examples.

The downlevel support in LibraryImportGenerator is meant to simplify the dotnet/runtime build. It is explicitly a non-supported scenario outside the runtime. If we start shipping it, even just to other repos, then we need to consider that fact when evolving the generator.

It was unclear to me that we draw the line between repositories and not between shipping vs unshipping. In this example, the assets would have been part of the transport package and been used during the build but never exposed to customers.

For example, we can remove fallback marshaller implementations when dotnet/runtime stops building for .NET 6 in a few years, but if we start shipping that downlevel support, it becomes a true breaking change to do that.

I expect that winforms follow the established package support policy and remove Drawing's net6.0 tfm when .NET 6 reaches EOL, the same time that we in dotnet/runtime remove our net6.0 tfms. I will discuss with @JeremyKuhne in our next sync meeting.

Thanks for the input. I'm now closing the PR and will follow the recommended approach.