System.Drawing.Common is missing a netcoreapp rid-less asset

[ericstj]

I'm not sure how we aren't catching this in package testing but I noticed this today.

We have a netcoreapp3.0 ref, but no rid-less netcoreapp3.0 lib. We have rid-specific libs, but if someone were to use this package on a platform that didn't map to unix|win they'd get the netstandard2.0 lib. Moreover this lib has a lower version than the netcoreapp3.0 ref, so they would see a file load exception. We should fix this by adding a netcoreapp config to match the ref.

Maybe we should see why package testing missed it too /cc @joperezr @safern

[safern]

The netstandard2.0 lib asset just throws. Since S.D.Common is very rid specific, should we make the same for netcoreapp3.0?

[ericstj]

Yeah, a not supported assembly is fine. It just needs to have an assembly version that matches the ref.

[joperezr]

I'll take a look later today into why this wasn't catched in our package testing, as I agree that this should have been caught.

[joperezr]

We have a netcoreapp3.0 ref, but no rid-less netcoreapp3.0 lib. We have rid-specific libs, but if someone were to use this package on a platform that didn't map to unix|win they'd get the netstandard2.0 lib.

I took a look, the reason why we are not hitting any issues in package testing is because we don't test any framework|RID configuration where the above is true.

Netcoreapp 3.0 tests the following RIDS:

    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=ubuntu.14.04-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=linuxmint.17-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=centos.7-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=rhel.7.2-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=debian.8-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=win7-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=win7-x86
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=ubuntu.16.04-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=fedora.24-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=osx.10.12-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=rhel.7-x64
    Testing System.Drawing.Common TFM=netcoreapp3.0 RID=opensuse.42.1-x64

All of which get either the win-rid specific asset or the unix-rid specific asset and so the AssemblyVersions match as expected.

[ericstj]

We don't test with no RID? We used to. We should fix that.

[ericstj]

I was also expecting package validation to catch this, not just package testing. The package report should show the mismatch.

[joperezr]

We do test with no RID, but in that case I believe we don't consider runtime files, same goes for the package report, the report doesn't emit runtimeAssets when there is no RID:

    ".NETCoreApp,Version=v3.0": {
      "Framework": ".NETCoreApp,Version=v3.0",
      "CompileAssets": [
        {
          "LocalPath": "F:\\git\\runtime\\artifacts\\bin\\System.Drawing.Common\\ref\\netcoreapp3.0-Debug\\System.Drawing.Common.dll",
          "PackagePath": "ref/netcoreapp3.0/System.Drawing.Common.dll",
          "SourceProject": {
            "Project": "F:\\git\\runtime\\src\\libraries\\System.Drawing.Common\\ref\\System.Drawing.Common.csproj",
            "AdditionalProperties": "TargetFramework=netcoreapp3.0;"
          },
          "TargetFramework": ".NETCoreApp,Version=v3.0",
          "Version": "5.0.0.0"
        }
      ]
    },

[ericstj]

I see, that likely predates our pattern of shipping not-supported assemblies. I could have sworn we had validation that forced this :(

[joperezr]

For reference, here is the piece if code that is in question:

runtime/src/libraries/pkg/test/packageTest.targets

Lines 63 to 77 in a43d6c4

	<Target Name="VerifyRuntimeClosure"
	DependsOnTargets="ResolveReferences"
	Condition="'$(ShouldVerifyClosure)' == 'true' and '$(RuntimeIdentifier)' != ''">
	<ItemGroup>
	<_runClosureFileNames Include="@(ReferenceCopyLocalPaths->'%(FileName)')">
	<Original>%(Identity)</Original>
	</_runClosureFileNames>
	<_runClosureFileNamesFiltered Include="@(_runClosureFileNames)" Exclude="@(ExcludeFromClosure);@(ExcludeFromClosure->'%(Identity).ni')"/>
	<_runClosureFileFiltered Include="@(_runClosureFileNamesFiltered->'%(Original)')"/>
	</ItemGroup>
	<VerifyClosure Condition="'@(_runClosureFileFiltered)' != ''"
	Sources="@(_runClosureFileFiltered)"
	IgnoredReferences="@(IgnoredReference)" />
	</Target>

If you see, VerifyRuntimeClosure will only run if RuntimeIdentifier is set, so on the one that doesn't have a RID doesn't show.

[danmoseley]

@ericstj do we need to add validation here and run it for 5.0 do you think?

[safern]

FWIW, we added this asset on: #40814