-
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
Allow pkgs to partially load if some of the assemblies are good #12763
Changes from 6 commits
63aebaa
b96b8bf
176dc9c
58d2115
e1ac6d1
699c060
289edd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,9 +246,11 @@ private void TryLoadPackageIntoLibrary(Package package) | |
if (package.LoadState.State == PackageLoadState.StateTypes.Unloaded) return; | ||
|
||
List<Assembly> loadedNodeLibs = new List<Assembly>(); | ||
List<Assembly> failedNodeLibs = new List<Assembly>(); | ||
try | ||
{ | ||
// load node libraries | ||
List<Assembly> blockedAssemblies = new List<Assembly>(); | ||
// Try to load node libraries from all assemblies | ||
foreach (var assem in package.EnumerateAndLoadAssembliesInBinDirectory()) | ||
{ | ||
if (assem.IsNodeLibrary) | ||
|
@@ -260,11 +262,38 @@ private void TryLoadPackageIntoLibrary(Package package) | |
} | ||
catch (LibraryLoadFailedException ex) | ||
{ | ||
// Managed exception | ||
// We can still try to load other parts of the package | ||
Log(ex.GetType() + ": " + ex.Message); | ||
} | ||
catch (DynamoServices.AssemblyBlockedException) | ||
{ | ||
blockedAssemblies.Add(assem.Assembly); | ||
} | ||
catch (Exception) | ||
{ | ||
failedNodeLibs.Add(assem.Assembly); | ||
} | ||
} | ||
} | ||
|
||
if (loadedNodeLibs.Count > 0) | ||
{ | ||
// Try to load any valid node libraries regardless package state | ||
PackagesLoaded?.Invoke(loadedNodeLibs); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you change the sequence of loading this by moving it up here from line 290? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so that I do not have to add it in 2 places (when it succeeds and in the catch blocks) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid code duplication...I can put it here (line 283) or in a finally block (but I would have to tweak the loadedNodeLibs to be declared before the try catch) |
||
} | ||
|
||
if (blockedAssemblies.Count > 0) | ||
{ | ||
throw new Exception("The following assemblies are blocked : " + string.Join(", ", blockedAssemblies.Select(x => Path.GetFileName(x.Location)))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct. Thanks |
||
} | ||
|
||
// Generic fatal error | ||
if (failedNodeLibs.Count > 0) | ||
{ | ||
throw new Exception("Failed to load the following assemblies : " + string.Join(", ", failedNodeLibs.Select(x => Path.GetFileName(x.Location)))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well..it could be..but I don't think it matters...since we do not catch that in any special way in this try catch |
||
} | ||
|
||
// load custom nodes | ||
var packageInfo = new Graph.Workspaces.PackageInfo(package.Name, new Version(package.VersionName)); | ||
var customNodes = OnRequestLoadCustomNodeDirectory(package.CustomNodeDirectory, packageInfo); | ||
|
@@ -287,7 +316,6 @@ private void TryLoadPackageIntoLibrary(Package package) | |
|
||
package.SetAsLoaded(); | ||
PackgeLoaded?.Invoke(package); | ||
PackagesLoaded?.Invoke(loadedNodeLibs); | ||
|
||
PythonServices.PythonEngineManager.Instance. | ||
LoadPythonEngine(package.LoadedAssemblies.Select(x => x.Assembly)); | ||
|
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.
any other type of Exception would disrupt the package loading operation.
Exceptions are managed in the same way inside
LibraryServices.LoadNodeLibrary
, i.e LibraryLoadFailedException usually means you can continue, anything else is fatal to the loading op