React to APICompat changes enabling reverse APICompat for facades

[ericstj]

I made changes to API Compat that enable it for partial facades, this is the result. We'll want to look through these baselines before RTM to understand if anything here needs to be exposed.

For things that need to be exposed that can happen in separate PRs since it may require API review + test additions.

NO MERGE - this is using a private build of API compat.

[ericstj]

@ViktorHofer @safern : tests tab shows no failures, yet the send to helix step is reporting a failure. Might be something amiss in the AZDO test integration.

[ViktorHofer]

Can you point me to the AzDO build? Usually that happens when the work item starts to fail. We mentioned that in the mail that I sent out earlier this week.

[ericstj]

https://dev.azure.com/dnceng/public/_build/results?buildId=177761&view=logs&jobId=29e7c093-c33b-5b8e-a00b-0e954875c839&taskId=e8d9965f-ee57-59e4-3e7a-0c8aaa58218b&lineStart=42&lineEnd=44&colStart=1&colEnd=14

[ViktorHofer]

dotnet/arcade#2659 (comment)

[ahsonkhan]

@GrabYourPitchforks, this should/can be fixed in the ref.

https://github.com/dotnet/coreclr/blob/4ad11ccba4ebb458046891bd07c0c7d4440d7df4/src/System.Private.CoreLib/src/System/Utf8String.Manipulation.cs#L30-L35

[ahsonkhan]

Unless it was meant to be removed as part of dotnet/coreclr#24036. cc @tarekgh

[tarekgh]

@ahsonkhan yes this method is removed. callers can use Utf8String[Index.GetOffset(Length)..] instead.

[ericstj]

It is still present in the implementation, thus the baseline. https://github.com/dotnet/coreclr/blob/c2e71fd5e711ca150e238741015a1a0229606793/src/System.Private.CoreLib/src/System/Utf8String.Manipulation.cs#L31

[ericstj]

Removing NO MERGE. This is now ready to go. FYI: this means we now will run reverse API compat on all implmentation assemblies (including partial facades) so it may mean more things to fix in the coreclr ingestion PRs.

[ericstj]

@ViktorHofer looks like this might be failing due to dotnet/arcade#2402

[ViktorHofer]

Oh right. Let me fix that.

[ViktorHofer]

Fixed, the host was missing. This is a workaround, I would have expected the RemoteExecutor/build file to deal with copying the host to the output dir. Seems like depproj files don't import such.

[ericstj]

Fixed, the host was missing. This is a workaround, I would have expected the build targets file to deal with copying the host to the output dir. Seems like depproj files don't import such.

That's what I thought. It seemed to me like restore didn't even write this to the generated targets file, so it might be an issue with the naming convention in the package. Here's what I see in the assets file:

  "targets": {
    ".NETCoreApp,Version=v3.0": {
      "Microsoft.DotNet.RemoteExecutor/1.0.0-beta.19256.12": {
        "type": "package",
        "dependencies": {
          "xunit.assert": "2.4.1-pre.build.4059"
        },
        "compile": {
          "lib/netcoreapp2.1/Microsoft.DotNet.RemoteExecutor.dll": {}
        },
        "runtime": {
          "lib/netcoreapp2.1/Microsoft.DotNet.RemoteExecutor.dll": {}
        },
        "build": {
          "build/_._": {}
        }
      },

Maybe something is adding ExcludeAssets?

[joperezr]

NIT: looks like you ran a msbuild /bl and checked in the msbuild.ProjectImports.zip by mistake on the AsyncInterfaces src directory 😆

[joperezr]

Left a few minor comments but this LGTM. From the build errors looks like you are still missing a baseline in System.Threading.Tasks.

[ericstj]

build errors looks like you are still missing a baseline in System.Threading.Tasks.

Yeah, this is do to @stephentoub's rename from this morning 51478cf. We can baseline them until we get a build from CoreCLR with them removed.

[tannergooding]

Is CLSCompliantAttribute meaningful? If so, we should just add it...

[ericstj]

It needs to be consistent. I believe the problem is that CoreLib sets CLSCompliant on assembly level then turns if off on individual types. Your reference assembly sets it as false on the assembly so it doesn't need to disable it on individual types.

corefx/src/System.Runtime.Intrinsics/ref/System.Runtime.Intrinsics.csproj

Line 4 in d81fc35

<ClsCompliant>false</ClsCompliant>

[stephentoub]

Yeah, this is do to @stephentoub's rename from this morning

Sorry, did I somehow break something in master?

[ericstj]

@stephentoub nope, just broke this PR between the time I tested locally and when the CI hit it. No worries.

@ViktorHofer your test host change is still broken. The problem is that it's referenced by test projects via the $(RuntimePath) which will have netcoreapp assemblies. These are incorrect when building a netstandard test project. For now I'm going to undo the update to the remote executor host so that you can work on a fix for that.

[ViktorHofer]

These are incorrect when building a netstandard test project.

That surprises me as the Remote Executor lib assembly is binary identical even though it cross-compiles.

[ericstj]

That surprises me as the Remote Executor lib assembly is binary identical even though it cross-compiles.

The references are different. .NETCoreApp is System.Runtime based and the System.Runtime assembly version is higher than the facades included in netstandard projects.

[ericstj]

All tests are failing with:

error: Common Language Runtime detected an invalid program.

This appears to be a result of arcade changes, but I haven't isolated the change. I'm going to dial back this PR to only the API compat update. We'll need to further investigate what's causing this, but we can do so in the Arcade ingestion PR.

[ericstj]

@jkoritzinsky it looks like your change dotnet/coreclr@796776c is causing CoreFx tests to fail when run on nano. It appear that this is due to https://github.com/dotnet/arcade/blob/ce8e852e418ca52d28e562f9697fbb926d0b0bb1/src/Microsoft.DotNet.XUnitConsoleRunner/src/Program.cs#L8.

[ericstj]

Only failure is network test.