for common use cases, avoid distributing System.Reflection.MetadataLoadContext dlls and deps. #15170
Conversation
UI Smoke TestsTest: success. 2 passed, 0 failed. |
| <dependencies> | ||
| <group targetFramework="netstandard2.0" /> | ||
| <group targetFramework="netstandard2.0"> | ||
| <dependency id="System.Reflection.MetadataLoadContext" version="8.0.0" /> |
There was a problem hiding this comment.
What are the cases where we would need this? I don't suspect net48 targeting apps would need this assembly. Can't we just exclude it all the time?
There was a problem hiding this comment.
As stated in the description, given the current APIs available in DynamoServices, and the use case for ZT authoring - I agree with your suspicion. It's not clear that those APIs will always remain internal though.
I could also see someone using the DynamoServices package to build some kind of integration rather than to author ZT nodes.
Since we do not sufficiently describe the use cases for these packages it's hard to reason about - IF someone somehow found themselves in a position where they were building a net48 app using DynamoServices and invoked these python load apis the binaries would need to be there.
We could certainly simplify it to always exclude if we're okay dealing with that later, or calling it out of the intended use case for this package - which IMO does make more sense to compile against instead of to consume.
aparajit-pratap
left a comment
There was a problem hiding this comment.
One question, then LGTM.
Purpose
This PR makes two changes
System.Reflection.MetadataLoadContext.dllis still brought into the bin folder fromDynamoPackagesand it's still necessary, this binary is not part of the runtime.DynamoServicesnuspec so that by default whennet8project the SRMLC runtime assets are not copied. This makes it so clients won't have these binaries in their bin folders by default.netstd2project the dependencies are copied as it's possible the developer is targeting an environment where these binaries are not included in the runtime. (net48). I think the value of this is limited for reasons I will get into below.The method that references
MetadataLoadContextis currently private and so are all the methods that invoke that method. It's unlikely that anyone will find themselves in a position requiring this type or needing it at runtime via JIT by just referencing theDynamoServicespackage. For the moment I think this change is okay, but it's not clear yet what use cases it really supports.I think the most important use case to make simple and straightforward is the ZT node author writing nodes for Dynamo 3.x. From that perspective, I think this change is good, it notes the dependency, but does not copy the binaries by default making loading of their packages less fraught (IMO anyway).
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Updates
DynamoServicesNuGet Package with references toSystem.Reflection.MetadataLoadContext