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

Address loading of packages with inter-dependencies #9736

Merged
merged 24 commits into from
Jun 14, 2019

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented May 24, 2019

Purpose

This PR addresses the following issues (JIRA Link: https://jira.autodesk.com/browse/DYN-1863):

  1. If there's a package A that depends on package B, and A does not come bundled with B, package B needs to be installed (from the PM) or loaded into Dynamo (imported in the VM) before A for A to be loaded successfully. If not, only B is loaded.
  • If B is already installed but not imported before A, resolve the dependencies from B that are required while importing A by searching for them in the package paths.

  • If package B is not already installed there is not much that can be done unless the user installs B manually. Once B is loaded/installed, we need to restart Dynamo in order to reload A.

  1. If package A depends on B and also contains B's assembly, then A is imported successfully. In this case B will not be installed (nor imported). If B is installed from the PM separately, it will fail as the VM will throw a class redefinition error.
  • Still making sure that B is installed properly.

  • Now if B is labelled as "node library" in pkg.json manifest file it will load along with A.

  1. Package load failures should now show up in notifications manager as shown:

image

screenshot2

Performance profiling results for loading 6 packages totaling 15,000 nodes during Dynamo startup (savings of ~10sec out of overall time of ~66sec):
Before:
image

After:
image

Declarations

Check these if you believe they are true

  • The code base 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.

Reviewers

@mjkkirschner

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@aparajit-pratap aparajit-pratap changed the title [WIP] refactor package loading [WIP] Address loading of packages with inter-dependencies Jun 4, 2019
@aparajit-pratap aparajit-pratap changed the title [WIP] Address loading of packages with inter-dependencies Address loading of packages with inter-dependencies Jun 13, 2019
@aparajit-pratap aparajit-pratap added the PTAL Please Take A Look 👀 label Jun 13, 2019
var entries = CurrentDynamoModel.SearchModel.SearchEntries.ToList();
Assert.IsTrue(entries.Any(x => x.FullName == "AnotherPackage.AnotherPackage.AnotherPackage.HelloAnotherWorld"));
Assert.IsTrue(entries.Any(x => x.FullName == "DependentPackage.DependentPackage.DependentPackage.HelloWorld"));
Assert.IsTrue(entries.Any(x => x.FullName == "Package.Package.Package.Hello"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following code shows the methods defined in the 3 test packages. DependentPackage has a dependency on Package and AnotherPackage has a dependency on the first two:
image

@@ -47,7 +47,7 @@ public void SetsErrorState()
//open a dyf file and modify it
string packagedirectory = Path.Combine(TestDirectory, "pkgs");
var packages = Directory.EnumerateDirectories(packagedirectory);
var first = Path.GetFullPath(packages.First());
var first = Path.GetFullPath(packages.FirstOrDefault(x => Path.GetFileName(x).Equals("Custom Rounding")));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests specifically check for the "Custom Rounding" test package that earlier happened to be the first package in the list. This order changed in this PR after I added new test packages, which is why I'm specifically searching for the "Custom Rounding" package from now on.

@mjkkirschner
Copy link
Member

LGTM!

@mjkkirschner mjkkirschner added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Jun 14, 2019
@aparajit-pratap aparajit-pratap merged commit 5bafc55 into DynamoDS:master Jun 14, 2019
@aparajit-pratap aparajit-pratap deleted the packageLoad branch June 14, 2019 15:50
mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
* refactor package loading

* change method name

* refactor import packages during install

* add import library step into VM after ImportLibrary is called

* fixes in library services resolve assembly

* try sending failure messages to notification view extension

* surface package load exception to notifications view extension

* clear notifications from logger after adding them to notifications extension

* surface more library load failed exceptions to notification view extension

* address review comments

* revert unchanged file

* obsolete public API's no longer used and to be removed in 3.0

* remove ReadyParams property

* cleanup

* correct for importing library from assembly info in old xml formatted DYN files

* more cleanup

* refactor LibraryServices methods

* code cleanup, documentation

* add reasons for obsoleting methods

* add test for loading interdependent packages

* cleanup

* update failing tests from self-serve

* revert unchanged file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants