Build the shim assemblies as part of libs.sfx #89005

ViktorHofer · 2023-07-17T13:19:37Z

Unblocks Enable Nullable compilation on the Options source gen #88973
Removes long stand prebuilds in sourcebuild (System.Drawing.Common/7.0.0 and System.Data.SqlClient/4.8.5), cc @MichaelSimons @mthalman
Adds APICompat build protection to the .NET Framework and .NET Standard shim assemblies

The shims build was intentionally kept out of the libs.sfx subset as some shims reference out-of-band assemblies that also need to be built.

Since that change was made, the shim assemblies now don't reference all out-of-band assemblies by default anymore and instead use fine grained dependencies. Because of that, the number of out-of-band projects referenced and built as part of the shims build is much smaller (~10 projects).

This fixes issues with ouf-of-band source generators not being able to target the live targeting pack. These source generators need the shims (more precisely the netstandard.dll shims) as some of the referenced Microsoft.CodeAnalysis packages don't provide a .NETCoreApp assembly.

The shims build was intentionally kept out of the libs.sfx subset as some shims reference out-of-band assemblies that also need to be built. Since that change was made, the shim assemblies now don't reference all out-of-band assemblies by default anymore and instead use fine grained dependencies. Because of that, the number of out-of-band projects referenced and built as part of the shims build is much smaller (~10 projects). This fixes issues with ouf-of-band source generators not being able to target the live targeting pack. These source generators need the shims (more precisely the netstandard.dll shims) as some of the referenced Microsoft.CodeAnalysis packages don't provide a .NETCoreApp assembly.

ghost · 2023-07-17T13:19:51Z

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

Issue Details

Unblocks #88973

The shims build was intentionally kept out of the libs.sfx subset as some shims reference out-of-band assemblies that also need to be built.

Since that change was made, the shim assemblies now don't reference all out-of-band assemblies by default anymore and instead use fine grained dependencies. Because of that, the number of out-of-band projects referenced and built as part of the shims build is much smaller (~10 projects).

This fixes issues with ouf-of-band source generators not being able to target the live targeting pack. These source generators need the shims (more precisely the netstandard.dll shims) as some of the referenced Microsoft.CodeAnalysis packages don't provide a .NETCoreApp assembly.

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

ViktorHofer · 2023-07-17T13:25:17Z

cc @MichalStrehovsky as we chatted about this in the past. You can now build libs.sfx and then immediately build tests as the shim assemblies are now included in that subset build. The only downside is that the subset now builds the few out-of-band assemblies (less than 10) that are referenced by the shims.

ericstj

Love to see us adding this infra. Have a few suggestions.

src/libraries/shims/fakes/README.md

src/libraries/shims/fakes/Directory.Build.targets

src/libraries/shims/fakes/Directory.Build.props

src/libraries/shims/fakes/Directory.Build.targets

ericstj

This is good and my feedback is not blocking. Remaining feedback is naming and testing suggestions.

src/libraries/shims/fakes/Directory.Build.targets

src/libraries/shims/fakes/Microsoft.Win32.SystemEvents.csproj

ViktorHofer · 2023-07-18T08:54:39Z

Here are the diffs for the impacted shim assemblies. I found a few issues that I fixed with the latest commit:

Type forwards for nested classes were missing (thanks for the feedback @ericstj)
A few types were located in the wrong assembly destination.

APICompat doesn't seem to protect these type forwards as the left side is missing the destination assemblies. We could protect this path by adding PackageReferences to apicompat.proj for the ~19 missing out-of-band assemblies on the left hand side (baseline).

Assembly	Link
mscorlib	https://www.diffchecker.com/F8KC0BaO/
System.Configuration	https://www.diffchecker.com/DXYwL6A1/
System.Core	https://www.diffchecker.com/LAkoB6ud/
System.Data	https://www.diffchecker.com/w3qtLVAT/
System	https://www.diffchecker.com/FTsmZHKl/
System.Drawing	https://www.diffchecker.com/JJGanOdj/
System.Net	https://www.diffchecker.com/oCadspIw/
System.Runtime.Serialization	https://www.diffchecker.com/5acRzSLc/
System.Security (fixed the assembly reference change)	https://www.diffchecker.com/7fS43tpW/
System.ServiceModel.Web	https://www.diffchecker.com/YoIfxfZv/
System.ServiceProcess	https://www.diffchecker.com/uUe8VKTP/
System.Transactions	https://www.diffchecker.com/77PuXZhY/
WindowsBase	https://www.diffchecker.com/JhpegEzg/

src/libraries/shims/stubs/Directory.Build.targets

ericstj · 2023-07-18T16:00:33Z

src/libraries/apicompat/ApiCompat.proj

+
+    <!-- Restore and reference assemblies required for resolving type forwards on the baseline (left) side. -->
+    <ApiCompatLeftTypeForwardDestinationPackage Include="@(ApiCompatTypeForwardDestinationPackage)"  />
+    <ApiCompatLeftTypeForwardDestinationPackage


It looks like you got this all fixed up now, but one thing you might consider is using a project to do an actual restore of the old Microsoft.Windows.Compatibility package and all assets for a particular target framework. That might avoid juggling all the PackageDownload items.

elinor-fung · 2023-07-18T17:01:00Z

@ViktorHofer the coreclr / mono runtime tests builds appear to be broken with this change. Those legs weren't run in this PR.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=343619&view=logs&jobId=c92f2c34-43c3-5f9c-356c-2e863ce9eb4e

CSC : error CS8785: Generator 'XUnitWrapperGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'NullReferenceException' with message 'Object reference not set to an instance of an object.' [/__w/1/s/src/tests/Interop/COM/ComWrappers/API/ComWrappersTests.csproj] [/__w/1/s/src/tests/build.proj]
/__w/1/s/src/tests/Interop/COM/ComWrappers/API/Program.cs(931,10): error CS0012: The type 'Attribute' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [/__w/1/s/src/tests/Interop/COM/ComWrappers/API/ComWrappersTests.csproj] [/__w/1/s/src/tests/build.proj]
/__w/1/s/src/tests/Interop/COM/ComWrappers/API/Program.cs(931,10): error CS0012: The type 'Enum' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [/__w/1/s/src/tests/Interop/COM/ComWrappers/API/ComWrappersTests.csproj] [/__w/1/s/src/tests/build.proj]
/__w/1/s/src/tests/Interop/COM/ComWrappers/API/Program.cs(931,10): error CS0012: The type 'Type' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [/__w/1/s/src/tests/Interop/COM/ComWrappers/API/ComWrappersTests.csproj] [/__w/1/s/src/tests/build.proj]

Example PRs:
#89091
#88941
#89061

ViktorHofer · 2023-07-18T17:36:12Z

Taking a look

ViktorHofer · 2023-07-18T18:09:11Z

Give me 30min and otherwise let's revert my change. Here's my attempt to fix it: #89115

Follow-up on #89005

* Fix broken runtime tests build Follow-up on #89005 * Update build-test-job.yml * Update sfx-ref.proj

MichalStrehovsky · 2023-07-19T00:25:40Z

cc @MichalStrehovsky as we chatted about this in the past. You can now build libs.sfx and then immediately build tests as the shim assemblies are now included in that subset build. The only downside is that the subset now builds the few out-of-band assemblies (less than 10) that are referenced by the shims.

This is awesome! Thank you! Submitted #89153 to save money on CI.

ghost assigned ViktorHofer Jul 17, 2023

dotnet-issue-labeler bot added the area-Infrastructure-libraries label Jul 17, 2023

ViktorHofer requested review from carlossanlop and a team July 17, 2023 13:19

ViktorHofer requested a review from tarekgh July 17, 2023 13:19

ViktorHofer marked this pull request as draft July 17, 2023 13:39

This was referenced Jul 17, 2023

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

ViktorHofer marked this pull request as ready for review July 17, 2023 19:20

Add fake assemblies for out-of-band type forward destinations

cf755a3

ViktorHofer force-pushed the BuildShimsWithSfx branch from 8458a88 to cf755a3 Compare July 17, 2023 19:24

ericstj reviewed Jul 17, 2023

View reviewed changes

PR feedback

a866009

ericstj approved these changes Jul 18, 2023

View reviewed changes

src/libraries/shims/fakes/Directory.Build.targets Outdated Show resolved Hide resolved

src/libraries/shims/fakes/Microsoft.Win32.SystemEvents.csproj Outdated Show resolved Hide resolved

ViktorHofer added 3 commits July 18, 2023 08:35

PR feedback #2

9743fa7

Fix the build

5528eed

Fix issues with nested classes and wrong type destinations

b7ae915

ViktorHofer added 9 commits July 18, 2023 11:16

Merge remote-tracking branch 'origin/main' into BuildShimsWithSfx

bc3ad9d

Merge remote-tracking branch 'origin/main' into BuildShimsWithSfx

7375514

Disable binplacing

656cd2c

Make stubs private

5937d0e

Fix StrongNameKey for Crypto.Xml

598405a

Disable symbols for stubs

f99a994

Add build protection to validate API compatibility of shims

48a1d31

Fix the Unix build

bba9fdf

Fix build

2faf4e9

ViktorHofer merged commit f2fa057 into main Jul 18, 2023
100 of 104 checks passed

ViktorHofer deleted the BuildShimsWithSfx branch July 18, 2023 15:24

ericstj reviewed Jul 18, 2023

View reviewed changes

ViktorHofer added a commit that referenced this pull request Jul 18, 2023

Fix broken runtime tests build

34e5874

Follow-up on #89005

ViktorHofer mentioned this pull request Jul 18, 2023

Fix broken runtime tests build #89115

Merged

ViktorHofer added a commit that referenced this pull request Jul 18, 2023

Fix broken runtime tests build (#89115)

d39c2eb

* Fix broken runtime tests build Follow-up on #89005 * Update build-test-job.yml * Update sfx-ref.proj

MichalStrehovsky mentioned this pull request Jul 19, 2023

Build less of libs for NativeAOT testing #89153

Closed

ghost locked as resolved and limited conversation to collaborators Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build the shim assemblies as part of libs.sfx #89005

Build the shim assemblies as part of libs.sfx #89005

ViktorHofer commented Jul 17, 2023 •

edited

Loading

ghost commented Jul 17, 2023

ViktorHofer commented Jul 17, 2023

ericstj left a comment

ericstj left a comment

ViktorHofer commented Jul 18, 2023 •

edited

Loading

ericstj Jul 18, 2023

elinor-fung commented Jul 18, 2023 •

edited

Loading

ViktorHofer commented Jul 18, 2023

ViktorHofer commented Jul 18, 2023 •

edited

Loading

MichalStrehovsky commented Jul 19, 2023

Build the shim assemblies as part of libs.sfx #89005

Build the shim assemblies as part of libs.sfx #89005

Conversation

ViktorHofer commented Jul 17, 2023 • edited Loading

ghost commented Jul 17, 2023

ViktorHofer commented Jul 17, 2023

ericstj left a comment

Choose a reason for hiding this comment

ericstj left a comment

Choose a reason for hiding this comment

ViktorHofer commented Jul 18, 2023 • edited Loading

ericstj Jul 18, 2023

Choose a reason for hiding this comment

elinor-fung commented Jul 18, 2023 • edited Loading

ViktorHofer commented Jul 18, 2023

ViktorHofer commented Jul 18, 2023 • edited Loading

MichalStrehovsky commented Jul 19, 2023

ViktorHofer commented Jul 17, 2023 •

edited

Loading

ViktorHofer commented Jul 18, 2023 •

edited

Loading

elinor-fung commented Jul 18, 2023 •

edited

Loading

ViktorHofer commented Jul 18, 2023 •

edited

Loading