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

for common use cases, avoid distributing System.Reflection.MetadataLoadContext dlls and deps. #15170

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Apr 22, 2024

Purpose

This PR makes two changes

  1. 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.
  2. 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).

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Updates DynamoServices NuGet Package with references to System.Reflection.MetadataLoadContext

Copy link

github-actions bot commented Apr 22, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@@ -15,7 +15,13 @@
It also contains useful static events and event data for monitoring workspace and execution changes.</summary>
<copyright>Copyright Autodesk 2023</copyright>
<dependencies>
<group targetFramework="netstandard2.0" />
<group targetFramework="netstandard2.0">
<dependency id="System.Reflection.MetadataLoadContext" version="8.0.0" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

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.

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, then LGTM.

@mjkkirschner mjkkirschner merged commit 210ed3b into DynamoDS:master Apr 24, 2024
22 of 23 checks passed
@mjkkirschner mjkkirschner deleted the dynservicesmlc branch April 24, 2024 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants