Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we stop using reference assemblies on desktop? #23309

Closed
ericstj opened this issue Aug 23, 2017 · 6 comments
Closed

Should we stop using reference assemblies on desktop? #23309

ericstj opened this issue Aug 23, 2017 · 6 comments
Labels
area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Aug 23, 2017

See issues dotnet/sdk#1522 and https://github.com/dotnet/corefx/issues/23229.

We're seeing multiple issues where tools in the .NET framework build pipeline attempt to load reference assemblies in an execution context (Assembly.Load, LoadFrom, LoadFile). This fails for assemblies that have the ReferenceAssemblyAttribute.

Build tools should never attempt to load references for execution: they may be running on totally different frameworks / platforms / versions. The problem is that not all tools (especially those for desktop) follow this practice. Largely because before System.Reflection.Metadata we didn't provide a safe way for build tools to do the right thing. They had to use the COM metadata APIs, CCI, LMR, or Cecil.

Given that landscape and our desire to work in as many places as possible, should we remove the ReferenceAssemblyAttribute from any assembly which would apply to desktop?

/cc @nguerrera @weshaggard @jkotas

@jkotas
Copy link
Member

jkotas commented Aug 23, 2017

Attempt to execute code in reference assembly typically fails with cryptic exceptions that our team was asked to debug. The attribute and loader check was introduced to replace the cryptic exceptions with something self-diagnosable to reduce our support burden. If we remove the attribute on reference assemblies, it will open flood gates for these type of support issues again.

@nguerrera
Copy link
Contributor

nguerrera commented Aug 23, 2017 via email

@ericstj
Copy link
Member Author

ericstj commented Aug 23, 2017

We've been using this bit for a long time. Help me understand the change that is making it problematic now.

Typically what happens is that we also have an assembly in the GAC, so even when someone loads a reference assembly from the targeting pack folks get the impl from the GAC.

We haven't been using it on desktop in a while. In the past we typically shipped our runtime facades for desktop, which didn't have the bit. Some refactoring in the corefx repo in 2.0 made it easy and cleaner to build ref facades so we did. Also the majority of our packages which did have some out-of-band implementation for desktop didn't use reference assemblies until 2.0. Also, when using our packages from a desktop project that uses packages.config it ignores the ref and uses the lib anyways.

Is it scoped to a particular set of references that are added for netfx -> netstandard?

No, but those are part of it and likely the most common culprit right now since they get added regardless of packages.config vs packagereference.

I think what we do is remove the bit from anything that builds for desktop and is a pure-facade. That'll fix most of the cases folks are hitting today. It sounds like we don't want to strip the bit completely, so perhaps we stop shipping reference assemblies for the OOB packages (like S.R.M and S.C.I) and build them to use for API compat but only include the libs in the package.

@weshaggard
Copy link
Member

I think what we do is remove the bit from anything that builds for desktop and is a pure-facade.

I think that is the right level of scoping to remove the bit. Those contain no type-definitions and thus can be loaded for runtime use without any real trouble. We should avoid removing the bit from any reference assembly that has any type definitions, but no code, in it.

@ericstj
Copy link
Member Author

ericstj commented Sep 6, 2017

dotnet/buildtools#1663 removes the bit from full facades.

@weshaggard
Copy link
Member

@ericstj I believe the work for this has been completed so closing.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants