Removing Reference assets from packages which contain runtime-specific dependencies when targeting Desktop.

[joperezr]

Fixes #32457

These changes will remove the reference assets from the packages which have runtime-specific dependencies which are causing RAR to not see a conflict, and won't generate binding redirects to address the conflict. As part of this, we will also add validation that will ensure future packages won't hit the same problem either (that will come in a buildtools PR). Marking it as No Merge since we have to follow the shiproom process to get this in.

cc: @ericstj @natemcmaster

[ericstj]

Did you consider adding the target to detect these?

We may want to do an assembly diff between the changes that this causes so that we can understand how this might impact consumers of these libraries.

[joperezr]

Did you consider adding the target to detect these?

I did but didn't want to add that on corefx directly, which is why in the description I mentioned that this will be added in buildtools. I'll add it there now in a PR and then update this one to ingest those new changes.

We may want to do an assembly diff between the changes that this causes

Agreed, I'll do that today for those 9 packages and ensure everything is ok.

[joperezr]

@ericstj I did a full Api diff comparing the old reports with the new ones after my change, and I didn't notice any possible problems except for one small issue with System.Security.Cryptography.Cng. The problem was for .NET Framework 4.6.1, since there are some types that were on the contract but are not present in the implementation assembly. I chatted with @bartonjs about this, and it looks like this is a known issue since the missing types where simply not present in .NET 4.6.1, so current users targetting that framework and using the package would be able to compile against the types, but get runtime errors for missing types. ApiCompat doesn't flag this because we have a baseline file for this known issue:

corefx/src/System.Security.Cryptography.Cng/src/ApiCompatBaseline.net461.txt

Lines 1 to 10 in a51d39a

	# The .NET 4.6.3 targeting pack (1.0.1) doesn't have the new members added for ECDsa import/export yet.
	MembersMustExist : Member 'Microsoft.Win32.SafeHandles.SafeNCryptHandle..ctor(System.IntPtr, System.Runtime.InteropServices.SafeHandle)' does not exist in the implementation but it does exist in the contract.
	MembersMustExist : Member 'Microsoft.Win32.SafeHandles.SafeNCryptKeyHandle..ctor(System.IntPtr, System.Runtime.InteropServices.SafeHandle)' does not exist in the implementation but it does exist in the contract.
	TypesMustExist : Type 'System.Security.Cryptography.AesCng' does not exist in the implementation but it does exist in the contract.
	MembersMustExist : Member 'System.Security.Cryptography.CngAlgorithm.ECDiffieHellman.get()' does not exist in the implementation but it does exist in the contract.
	MembersMustExist : Member 'System.Security.Cryptography.CngAlgorithm.ECDsa.get()' does not exist in the implementation but it does exist in the contract.
	MembersMustExist : Member 'System.Security.Cryptography.CngKeyBlobFormat.EccFullPrivateBlob.get()' does not exist in the implementation but it does exist in the contract.
	MembersMustExist : Member 'System.Security.Cryptography.CngKeyBlobFormat.EccFullPublicBlob.get()' does not exist in the implementation but it does exist in the contract.
	TypesMustExist : Type 'System.Security.Cryptography.DSACng' does not exist in the implementation but it does exist in the contract.
	TypesMustExist : Type 'System.Security.Cryptography.TripleDESCng' does not exist in the implementation but it does exist in the contract.

So what we've done with this change is to move a runtime error into a compile time error, which in my opinion is much better. Other than that, I didn't notice any problems this change would have API-wise.

[ericstj]

Interesting, so we had a net4x reference with type forwards for the existing types and type-defs for the missing ones, eh? That's rather odd but I see how that would have happened. I guess there could be someone who wrote light-up code that needed those type-defs to compile but guarded it with a runtime check. We could always add it back if folks reported such an issue. @bartonjs what do you think?

[bartonjs]

I discussed it with @joperezr this morning (er, yesterday, since I didn't hit Comment), and my thoughts are that we should just rip the band-aid (make it a compile error).

• Abandon this project: Callers get a TypeNotFoundException. They could be doing lightup to try and do a fallback, but seems unlikely.
• Make PNSE-throwing wrappers: Doesn't change anyone who wasn't calling it. Breaks lightup callers looking for TypeNotFoundException, continues to power anyone with open exception drains for lightup.
• Make an explicit thing that throws TypeNotFoundException: I feel like the runtime team will give us the evil eye.
• Use ILMerge (or something) to write type forwards we know won't resolve: Keeps the TypeNotFoundException, don't know what it does to compiling. Or VS.
• Leave the types out: Compile break in lightup code. Guess they have to switch to reflection?

Leaving the types out is the easiest, and the easiest to describe.

[ericstj]

Leave the types out: Compile break in lightup code. Guess they have to switch to reflection?

Or target netstandard 😛

I agree with you that its unlikely for folks to break, especially given that they would have needed a runtime facade with the typeforwards for that lightup to work. I'm ok with this.

[joperezr]

@dotnet-bot test OSX x64 Debug Build please

[joshfree]

@joperezr can this be merged now?

[joperezr]

Release/2.1 branch is currently locked which is why I have the NO MERGE label on this. Once the branch is unlocked we can merge this in.