Don't reference the netstandard shim inside the shared framework

[ViktorHofer]

The netstandard.dll shim shouldn't be referenced inside the shared
framework as it's a compat shim that shouldn't be required to compose
the shared framework.

This removes the necessity of a separate RefPath build and improves
incremental build times as only the few OOB projects that require the
live built shim would need to rebuild.

There are a few projects that require the netstandard.dll shim, i.e.
for netcoreapp3.1 or some OOB projects.

Adding the netcoreapp3.1 configuration to ref projects for which the src project already exposes a netcoreapp3.1 configuration. Doing that to guarantee that when P2Ping the src project, the correct matching assembly with the right assembly universe is resolved. More general speaking, making sure that the same assembly universe is resolved between ref and src.

[ghost]

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

• Commit 1: Don't reference ns.dll inside shared framework …

The netstandard.dll shim shouldn't be referenced inside the shared
framework as it's a compat shim that shouldn't be required to compose
the shared framework.

This removes the necessity of a separate RefPath build and improves
incremental build times as only the few OOB projects that require the
shim would need to rebuild.

• Commit 2: Add netcoreapp2.0 config to SystemEvents …

Adding a netcoreapp2.0 configuration to SystemEvents as the source
assembly targets netcoreapp2.0 and this avoid the netstandard shim to
be used. The additional configuration shouldn't pose a problem as with
an eventual convergance of reference and src projects, we will likely
not mix universes between ref and src anymore and build the additional
matching universe assemblies.

• Commit 4: Reference netstandard shim in OOBs that require it …

There are a few OOB projects that require the netstandard.dll shim as
they intentionally reference projects that only expose a netstandard
configuration.

@ericstj can you please help with the ApiCompat errors? Some like the attribute changes where we don't embed an integer but an enum now sound to be less harmful and should probably be suppressed but there are others like System.AppContext not in the ref that I don't understand. Thanks a lot.

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

[ViktorHofer]

@ericstj I found the reason for the APICompat errors. When not referencing netstandard.dll, the path to the refpack isn't added: https://www.diffchecker.com/0r29c7Sa. That's because of this:

runtime/eng/resolveContract.targets

Line 5 in 636f89d

<ContractDependencyPaths Condition="'$(ContractDependencyPaths)' == ''">@(ReferencePath->'%(RelativeDir)'->Distinct())</ContractDependencyPaths>

[ViktorHofer]

I believe there are two options:

• Add the refpack path if $(NetCoreAppCurrent)
• Undo

  runtime/eng/references.targets

  Lines 24 to 29 in 636f89d

  	<!-- Project references shouldn't be copied to the output for non test apps. -->
  	<ItemDefinitionGroup Condition="'$(IsTestProject)' != 'true' and '$(IsTestSupportProject)' != 'true'">
  	<ProjectReference>
  	<Private>false</Private>
  	</ProjectReference>
  	</ItemDefinitionGroup>

  so that references are copied into the respective reference assemblies app dirs. As we are linking and not copying by default anyway, I would actually prefer this over the other approach as it also improves debuggability when using tools like ilspy which dynamically try to resolve references from the app dir.
  That approach wouldn't work as-is, we would need to fix binplacing first so that it ignores the references from the ref and src projects. Also, would this regress perf noticeably? I need to check if the SDK really hardlinks by default. EDIT: Apparently hard linking is not turned on by default: https://github.com/dotnet/msbuild/blob/4adc47707f27d0dce03cee9e4651d599e7ff265f/src/Tasks/Microsoft.Common.CurrentVersion.targets#L4489.

Thoughts?

[ViktorHofer]

Also, would this regress perf noticeably?

I do think this would cause quite a noticeable perf penalty. Just tried this out locally. Even if hard linking would be enabled, collecting the outputs of the P2Ps is unnecessary work. Maybe we could reconsider when refs and srcs are converged but with the huge amount of projects today I wouldn't want to turn that behavior on.

[ericstj]

I do think this would cause quite a noticeable perf penalty.

Not only that, there are side-effects to copying. It can cause references to accidentally be consumed based on being next to the assembly rather than forcing folks to make a proper reference in the project file.

We should make API Compat work off of the references passed to the compiler, there should be a way to make that work. If the compiler sees enough to be happy then we ought to be able to resolve enough to compare the ref/src. I haven't yet looked into the failures but I can try to do so today if I have some cycles.

Other than that the changes look good so far (y)

[ViktorHofer]

We should make API Compat work off of the references passed to the compiler, there should be a way to make that work. If the compiler sees enough to be happy then we ought to be able to resolve enough to compare the ref/src. I haven't yet looked into the failures but I can try to do so today if I have some cycles.

I agree and will also take a look later today.

[ericstj]

Had a couple minutes to look at the binlog. All the failures stem from not having a closure of references for reference assemblies. As you mention this is because netstandard.dll is no longer being passed. This was coincidentally giving both the right and left hand side of APIcompat the path to the ref-pack.

The simplest fix might be to add another fallback here to $(MicrosoftNetCoreAppRefPackRefDir) when appropriate:

runtime/eng/resolveContract.targets

Line 5 in 01b7e73

<ContractDependencyPaths Condition="'$(ContractDependencyPaths)' == ''">@(ReferencePath->'%(RelativeDir)'->Distinct())</ContractDependencyPaths>

Alternatively we could implement passing the DependencyPaths metadata:
https://github.com/dotnet/arcade/blob/349d4a363ccb18d94f406f782c13ccf48df30805/src/Microsoft.DotNet.ApiCompat/src/build/Microsoft.DotNet.ApiCompat.targets#L61
By calling a different target here:

runtime/eng/resolveContract.targets

Lines 16 to 18 in 01b7e73

	<ProjectReference Include="$(ContractProject)" ReferenceOutputAssembly="false" OutputItemType="ResolvedMatchingContract" />
	<!-- We aren't referencing the contract, but make sure it's considered as an input to Compile so that if it changes we rebuild and rerun API compat -->
	<ProjectReference Include="$(ContractProject)" ReferenceOutputAssembly="false" OutputItemType="CustomAdditionalCompileInputs" />

And implementing that target in reference assembly projects to return all directories to their @(ReferencePath)s as the DependencyPaths metadata. Here's an example of another project that did that:
https://github.com/dotnet/machinelearning/blob/04dda55ab0902982b16309c8e151f13a53e9366d/tools-local/Microsoft.ML.StableApi/Microsoft.ML.StableApi.csproj#L25-L33

[ViktorHofer]

Had a couple minutes to look at the binlog. All the failures stem from not having a closure of references for reference assemblies. As you mention this is because netstandard.dll is no longer being passed. This was coincidentally giving both the right and left hand side of APIcompat the path to the ref-pack.
The simplest fix might be to add another fallback here to $(MicrosoftNetCoreAppRefPackRefDir) when appropriate:

Thanks for taking a look Eric. I prefer to choose the fallback as I hope that we will invest in converging ref and src projects in the (near) future anyway and this problem should then go away with that.

Alternatively we could implement passing the DependencyPaths metadata:

I was thinking of exactly the same thing. Cool that we are in agreement on an alternative solution :D

[ericstj]

I'm seeing the current failures are happening on older .NETCoreApp versions where you'll have a harder time changing the reference assemblies. You might need to dial back on the goal of reducing netstandard references and instead just make it so netstandard builds in a way that's more naturally part of the product. Technically it's not much different from other System assemblies that have pushed their types down, it just has more references.

[ericstj]

On second thought, we could just keep pushing forward by adding the netstandard reference on the pre-6.0 .NETCore frameworks, that'd satisfy those builds but then let you get netstandard out of the normal closure of the net6.0 assemblies.

[ericstj]

I believe you need a nestandard reference here: https://github.com/ViktorHofer/runtime/blob/45415008ca188bf934adb8f27642ba5f795bcd5f/src/libraries/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj#L59

[ViktorHofer]

WOHOO this is finally ready. This change will make incremental builds in dotnet/runtime much faster.

@safern @Anipik can you please review/approve as @ericstj is out for the next few days?

The windows arm64 failure is because of a corrupt machine. Notified core-eng in the First Responders Teams channel: https://teams.microsoft.com/l/message/19:[email protected]/1623319812422?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=4d73664c-9f2f-450d-82a5-c2f02756606d&parentMessageId=1623319812422&teamName=.NET%20Core%20Eng%20Services%20Partners&channelName=First%20Responders&createdTime=1623319812422

[eerhardt]

I thought we were trying to avoid netstandard on .NET Core. Doesn't this go against the goal of this PR?

[eerhardt]

Repeated in other .csprojs.

[ViktorHofer]

The PR only targets getting rid of the netstandard reference inside the shared framework and for some OOBs (which are in the middle of the graph) for NetCoreAppCurrent. There are still 50 libraries that don't have a .NETCoreApp configuration and all of those target .NETStandard. For referencing those we need the netstandard.dll shim as that's the piece that makes referencing a .NETStandard lib from .NETCoreApp work.

My master plan is that still during the .NET 6 timeframe we add a NetCoreAppCurrent configuration to every "applicable" library and with that avoid the necessity of the shim for NetCoreAppCurrent inside our build entirely.

When we then version the repository to .NET 7, we will have a net6.0 and a .net7.0 configuration in all our "applicable" packages and by that then be able to remove the netstandard.dll shim for .NETCoreApp entirely.

Getting rid of the netstandard.dll shim for netcoreapp3.1 would be unwise at this point as we just need to wait until we version the repo and get to net6.0 if my master plan succeeds ;)

[eerhardt]

Here's a difference from the two projects before this - targetting netcoreapp3.1, but referencing System.Runtime here, vs netstandard above.

[ViktorHofer]

This project doesn't reference any .NETStandard projects (via P2P) therefore the shim isn't required.

[eerhardt]

Do we have this written down? It is going to be near impossible for someone to discern the rules of when to reference what.

[ViktorHofer]

This was already the case before this change for reference projects. Before this PR the netstandard shim was automatically referenced by any .NETCoreApp source project. Reference always had to add the netstandard reference manually when they were referencing a netstandard project.

I do have a presentation planned (target audience Libraries Team) where I will go over the current state of libraries and their packages. Will make sure that the right documentation is in place by then.

[eerhardt]

why doesn't this have $(NetCoreAppCurrent)? We seem to do that elsewhere

[ViktorHofer]

To avoid the netstandard.dll shim when P2Pin the library, a .NETCoreApp configuration must be present. As this is just a PNSE, it doesn't matter if it's netcorepap3.1 or $(NetCoreAppCurrent).

[eerhardt]

You don't follow that same policy on

src/libraries/System.Security.Principal.Windows/src/System.Security.Principal.Windows.csproj

There you added both $(NetCoreAppCurrent) and netcoreapp3.1.

[ViktorHofer]

True. I think for Principal.Windows I chose to add $(NetCoreAppCurrent) configuration to the source so that in case another rid-less $(NetCoreAppCurrent) project references Principal.Windows, it will get the $(NetCoreAppCurrent) reference assembly. Without adding the configuration, the netcoreapp3.1 asset would be chosen.

It shouldn't really matter though as the API set is identical but I felt like it's better to guarantee that $(NetCoreAppCurrent) reference assemblies are always chosen if possible over others.

[eerhardt]

Are we sure dropping netstandard2.1 here is correct? Looks like @maryamariyan added it during getting this project moved over to this repo. 2220437

[ViktorHofer]

The netstandard2.1 reference isn't needed as netstandard2.0 fully satisfies the contract. It's not entirely clear to me why that configuration was added when bringing over the project but it doesn't serve any purpose, especially now that we are adding a .NETCoreApp configuration.

[ViktorHofer]

Failure is #54033