-
Notifications
You must be signed in to change notification settings - Fork 636
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
when loading python engines dynamically fail fast #14941
Conversation
UI Smoke TestsTest: success. 2 passed, 0 failed. |
use mlc
@@ -157,7 +157,7 @@ | |||
<PackageReference Include="SharpDX.Direct3D9" Version="4.2.0" /> | |||
<PackageReference Include="SharpDX.DXGI" Version="4.2.0" /> | |||
<PackageReference Include="SharpDX.Mathematics" Version="4.2.0" /> | |||
<PackageReference Include="System.Reflection.MetadataLoadContext" Version="6.0.0" /> | |||
<PackageReference Include="System.Reflection.MetadataLoadContext" Version="8.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why bump up the version ?
ANy issues compatibility issues with hosts ? Need to update licensing ?
Will it cause issues if we cherry pick this in 2.19.5 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- why use an old version?
- I don't think so, I can look at Revit, but I think aligning on the .net8 version makes the most sense and avoids conflicts with other packages, like meta data load context net8 and system collections immutable.
- I don't think we should cherry pick this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinzart90 WRT to licensing were your steps for regenerating the license sheets documented somewhere in the release process wiki? - I can't find them atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WRT Revit - they have removed their use of MLC - only RTF is using it currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the license, I was thinking we shoul just update the about box stuff. The dependency reports can be postponed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Should we specify this new requirement to the documentation somewhere ? |
Hmm - I think anyone writing a PythonEngine would likely look at the base class they have to inherit from - so I will add a comment to that base class summary. |
This reverts commit d01a151.
Purpose
When attempting to load python engines dynamically - attempt to load top level dependencies of the assembly containing the
PythonEngine
so we fail fast in a controlled way (inside a try catch)I verified this avoids the crash in DynamoRevit in 2.19.x so if we do a release there we can back port this change.
In addition these crashes no longer occur with the new packages (in old Dynamo versions) because .NetFramework4.8 cannot load the assembly load context types - so it fails even earlier when the extension
Ready
call is made so in either case partially working Python engines are no longer added to theAvailableEngines
collectionAlso updates MLC and System.Collections.Immutable packages to net8 versions.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of