DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x #15083

mjkkirschner · 2024-04-03T14:53:13Z

Purpose

Since packages that were published from Dynamo 1.x or 2.x likely target net framework instead of .NET8 - we'll raise a warning if versions of those packages are installed. At load time, we'll just log some information without alerting the user, this will be useful for devs when inspecting logs - it's possible that a package may load successfully but have strange behavior later.

- [ ] One improvement we could make here is to not raise these warnings for packages that do not contain any .Net binaries. (ie those that only contain dyf files) I was not sure about this, what do reviewers think?

This PR also fixes a really annoying bug where Dynamo's custom message box type had a tooltip show up with no content.

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

show warnings when installing packages into Dynamo 3.x that were published from Dynamo < 3.x

Reviewers

github-actions · 2024-04-03T15:09:45Z

UI Smoke Tests

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

pinzart90 · 2024-04-04T02:12:32Z

Is there a good way to check if a package or it’s deps have binaries ?

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

src/DynamoPackages/PackageLoader.cs

src/DynamoCoreWpf/Properties/Resources.en-US.resx

johnpierson · 2024-04-04T02:59:50Z

I think this is a good warning to have. However, I would be curious how this would work with something that may be built for net 4.8, but still works. Extensions technically work this way. (This is how monocle and Rhythm load on the fly). And if that warning shows up in my case, that is kind of lousy because the actual DLLs that I write on runtime are built for the correct frameworks.

mjkkirschner · 2024-04-04T15:15:05Z

Is there a good way to check if a package or it’s deps have binaries ?

kind of - the ContainsBinaries flag on packages is overloaded to mean contains binaries or python nodes. We can check the package once downloaded though.

mjkkirschner · 2024-04-04T15:29:52Z

I think this is a good warning to have. However, I would be curious how this would work with something that may be built for net 4.8, but still works. Extensions technically work this way. (This is how monocle and Rhythm load on the fly). And if that warning shows up in my case, that is kind of lousy because the actual DLLs that I write on runtime are built for the correct frameworks.

Hey @johnpierson yep it's a good point, unfortunately AFAIK we can't truly know if a binary targeting .net48 will work correctly on the .net8.0 runtime without doing a lot of inspection for references and even then we might still get it wrong.

I have two suggestions though:

you can use .netstd2.0 as a target instead of .net framework 4.8 for your extension binaries, these should be compatible everywhere and if we move to a smarter check where we look for the target framework version instead of just the dynamo engine publish version you'll pass that too.
If you publish your package from 3.0 then users in 3.0 won't see these warnings - of course then 2.x users will.

mjkkirschner · 2024-04-10T22:05:52Z

Primer documentation page is up now but I am trying to find an efficient and safe way to add clickable urls to the message box. It seems we have various ways we're currently doing it throughout the repo for other controls.

turns out Roberto solved this a while ago for guided tour functionality and we can just reuse the custom rich text box control he added there.

also fix tooltip bug

pinzart90 · 2024-04-12T00:33:39Z

The only possible improvement I can think of would be for the wording “This package or one… use an older version of dynamo than the one you are using”.
The word “use” could be “was built for” or “targets”. Not sure what would be better

pinzart90

One comment then Lgtm

hwahlstrom · 2024-04-12T12:29:25Z

Suggested rephrase of warning message:

Package may not work in this version of Dynamo

This package or one of its dependencies were created for a previous version of Dynamo. It may not work in this version. Do you want to continue?

Learn more

aparajit-pratap · 2024-04-12T14:15:33Z

src/DynamoCoreWpf/Utilities/MessageBoxUtilities.cs

+            MessageBoxResult IMessageBox.Show(Window owner, string msg, string title, bool showRichTextBox, MessageBoxButton button, MessageBoxImage img)
+            {
+                return DynamoMessageBox.Show(owner,msg, title, showRichTextBox, button, img);
+            }


Why do we need this?

for both the owner and showRichTextBox param.
Since this type and interface are really internal we can probably just delete all these overloads and simplify whenever we want.

ugh well, just to correct this a bit -
the interface is internal, but the MessageBoxService and these Show methods implementing that interface are public - though the whole thing is in DynamoCoreWPF.dll, so I think it requires just a bit more thinking before we refactor this.

…ge from 2.x in Dynamo 3.x (DynamoDS#15083) tests pass - merging. https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1423/ (filterFrozentest) is marked failure on master.

* test * DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x (#15083) tests pass - merging. https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1423/ (filterFrozentest) is marked failure on master.

mjkkirschner added 2 commits April 2, 2024 13:30

first commit

e63b2f2

add test and resx as well as logs on load

9770969

mjkkirschner requested review from sm6srw and pinzart90 April 3, 2024 14:54

mjkkirschner changed the title ~~DYN-6719 - warnings and logs when installing and loading a package from 2.x in Dynamo 3.x~~ DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x Apr 3, 2024

add to unshipped

7cec3db

aparajit-pratap reviewed Apr 4, 2024

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 4, 2024

View reviewed changes

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 4, 2024

View reviewed changes

src/DynamoPackages/PackageLoader.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Apr 4, 2024

View reviewed changes

src/DynamoCoreWpf/Properties/Resources.en-US.resx Show resolved Hide resolved

mjkkirschner added 2 commits April 11, 2024 19:06

add link and review comments

197f50e

also fix tooltip bug

api

c21211d

pinzart90 approved these changes Apr 12, 2024

View reviewed changes

aparajit-pratap reviewed Apr 12, 2024

View reviewed changes

aparajit-pratap approved these changes Apr 12, 2024

View reviewed changes

mjkkirschner added 3 commits April 12, 2024 13:34

review comments

ff82238

fix some tests

86247bf

fix more tests

678e3be

mjkkirschner merged commit 5cab0ad into DynamoDS:master Apr 15, 2024
22 of 23 checks passed

mjkkirschner deleted the dyn-6719 branch April 15, 2024 18:19

mjkkirschner mentioned this pull request Apr 15, 2024

cherry pick DYN-6719 #15133

Merged

mjkkirschner mentioned this pull request Apr 17, 2024

fix typos in package warnings #15150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x #15083

DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x #15083

mjkkirschner commented Apr 3, 2024 •

edited

Loading

github-actions bot commented Apr 3, 2024 •

edited

Loading

pinzart90 commented Apr 4, 2024

johnpierson commented Apr 4, 2024

mjkkirschner commented Apr 4, 2024

mjkkirschner commented Apr 4, 2024 •

edited

Loading

mjkkirschner commented Apr 10, 2024 •

edited

Loading

pinzart90 commented Apr 12, 2024

pinzart90 left a comment

hwahlstrom commented Apr 12, 2024

aparajit-pratap Apr 12, 2024

mjkkirschner Apr 12, 2024

mjkkirschner Apr 12, 2024 •

edited

Loading

DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x #15083

DYN-6719 - show warning when installing, and log when loading a package from 2.x in Dynamo 3.x #15083

Conversation

mjkkirschner commented Apr 3, 2024 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

github-actions bot commented Apr 3, 2024 • edited Loading

UI Smoke Tests

pinzart90 commented Apr 4, 2024

johnpierson commented Apr 4, 2024

mjkkirschner commented Apr 4, 2024

mjkkirschner commented Apr 4, 2024 • edited Loading

mjkkirschner commented Apr 10, 2024 • edited Loading

pinzart90 commented Apr 12, 2024

pinzart90 left a comment

Choose a reason for hiding this comment

hwahlstrom commented Apr 12, 2024

aparajit-pratap Apr 12, 2024

Choose a reason for hiding this comment

mjkkirschner Apr 12, 2024

Choose a reason for hiding this comment

mjkkirschner Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

mjkkirschner commented Apr 3, 2024 •

edited

Loading

github-actions bot commented Apr 3, 2024 •

edited

Loading

mjkkirschner commented Apr 4, 2024 •

edited

Loading

mjkkirschner commented Apr 10, 2024 •

edited

Loading

mjkkirschner Apr 12, 2024 •

edited

Loading