You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
excludes the runtime assets from SRMLC (System.Reflection.MetadataLoadContext) being pulled in via the DynamoServices project -which is a netstd2.0 project. (this is why the binaries were being brought into the bin folder) The
System.Reflection.MetadataLoadContext.dll is still brought into the bin folder from DynamoPackages and it's still necessary, this binary is not part of the runtime.
updates the DynamoServices nuspec so that by default when
consumed in a net8 project the SRMLC runtime assets are not copied. This makes it so clients won't have these binaries in their bin folders by default.
When consumed in a netstd2 project 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 MetadataLoadContext is 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 the DynamoServices package. 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).
The reason will be displayed to describe this comment to others. Learn more.
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.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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