-
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
Replace reflection only load context with shared metadataLoadContext for publishing packages. Solution 1. #14576
Changes from all commits
f21d3cd
26c6e27
7bc8727
dd080a2
7504a03
7ddb5b3
b76d815
5d528cb
ba7240c
cbca65f
ec946b6
e74a4f4
e498b45
8abc4fe
fbd1d69
4c59043
b22c889
a4ef81c
b10d203
c77be09
8d7ed18
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 |
---|---|---|
|
@@ -743,28 +743,38 @@ private void CheckPackageNodeLibraryCertificates(string packageDirectoryPath, Pa | |
} | ||
|
||
/// <summary> | ||
/// Attempt to load a managed assembly in to ReflectionOnlyLoadFrom context. | ||
/// Attempt to load a managed assembly in to MetaDataLoad context. | ||
/// </summary> | ||
/// <param name="filename">The filename of a DLL</param> | ||
/// <param name="mlc">The MetaDataLoadContext to load the package assemblies into for inspection.</param> | ||
/// <param name="assem">out Assembly - the passed value does not matter and will only be set if loading succeeds</param> | ||
/// <returns>Returns Success if success, NotManagedAssembly if BadImageFormatException, AlreadyLoaded if FileLoadException</returns> | ||
internal static AssemblyLoadingState TryReflectionOnlyLoadFrom(string filename, out Assembly assem) | ||
internal static AssemblyLoadingState TryMetaDataContextLoad(string filename,MetadataLoadContext mlc, out Assembly assem) | ||
{ | ||
try | ||
{ | ||
assem = Assembly.ReflectionOnlyLoadFrom(filename); | ||
return AssemblyLoadingState.Success; | ||
} | ||
catch (BadImageFormatException) | ||
{ | ||
assem = null; | ||
return AssemblyLoadingState.NotManagedAssembly; | ||
} | ||
catch (FileLoadException) | ||
{ | ||
assem = null; | ||
return AssemblyLoadingState.AlreadyLoaded; | ||
} | ||
try | ||
{ | ||
var mlcAssemblies = mlc.GetAssemblies(); | ||
assem = mlc.LoadFromAssemblyPath(filename); | ||
var mlcAssemblies2 = mlc.GetAssemblies(); | ||
//if loading the assembly did not actually add a new assembly to the MLC | ||
//then we've loaded it already, and our current behavior is to | ||
//disable publish when a package contains the same assembly twice. | ||
if (mlcAssemblies2.Count() == mlcAssemblies.Count()) | ||
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. is this the best way to check if the assembly has been loaded successfully ? 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'm not checking if the assembly has been loaded successfully, I am checking if it had been loaded already. |
||
{ | ||
throw new FileLoadException(filename); | ||
} | ||
return AssemblyLoadingState.Success; | ||
} | ||
catch (BadImageFormatException) | ||
{ | ||
assem = null; | ||
return AssemblyLoadingState.NotManagedAssembly; | ||
} | ||
catch (FileLoadException) | ||
{ | ||
assem = null; | ||
return AssemblyLoadingState.AlreadyLoaded; | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Dynamo; | ||
using Dynamo.Graph.Nodes.CustomNodes; | ||
using Dynamo.Graph.Workspaces; | ||
|
@@ -17,7 +18,7 @@ namespace DynamoCoreWpfTests | |
public class PublishPackageViewModelTests: DynamoViewModelUnitTest | ||
{ | ||
|
||
[Test, Category("Failure")] | ||
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. this seems to be passing... this test is unrelated to this task so if it fails on CI I will just revert this. |
||
[Test] | ||
public void AddingDyfRaisesCanExecuteChangeOnDelegateCommand() | ||
{ | ||
|
||
|
@@ -99,7 +100,34 @@ public void CanPublishLateInitializedJsonCustomNode() | |
} | ||
|
||
|
||
[Test, Category("Failure")] | ||
[Test] | ||
public void NewPackageDoesNotThrow_NativeBinaryIsAddedAsAdditionalFile_NotBinary() | ||
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. Does the dependencies field in the pkg.json being empty indicate that the native assembly is not added as a dependent binary but as an additional file? 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. the dependencies field points to other packages |
||
{ | ||
string packagesDirectory = Path.Combine(TestDirectory, "pkgs"); | ||
|
||
var pathManager = new Mock<Dynamo.Interfaces.IPathManager>(); | ||
pathManager.SetupGet(x => x.PackagesDirectories).Returns(() => new List<string> { packagesDirectory }); | ||
|
||
var loader = new PackageLoader(pathManager.Object); | ||
loader.LoadAll(new LoadPackageParams | ||
{ | ||
Preferences = ViewModel.Model.PreferenceSettings | ||
}); | ||
|
||
PublishPackageViewModel vm = null; | ||
var package = loader.LocalPackages.FirstOrDefault(x => x.Name == "package with native assembly"); | ||
Assert.DoesNotThrow(() => | ||
{ | ||
vm = PublishPackageViewModel.FromLocalPackage(ViewModel, package); | ||
}); | ||
|
||
Assert.AreEqual(1, vm.AdditionalFiles.Count); | ||
Assert.AreEqual(0, vm.Assemblies.Count); | ||
|
||
Assert.AreEqual(PackageUploadHandle.State.Ready, vm.UploadState); | ||
} | ||
|
||
[Test] | ||
public void NewPackageVersionUpload_DoesNotThrowExceptionWhenDLLIsLoadedSeveralTimes() | ||
{ | ||
string packagesDirectory = Path.Combine(TestDirectory, "pkgs"); | ||
|
@@ -123,7 +151,7 @@ public void NewPackageVersionUpload_DoesNotThrowExceptionWhenDLLIsLoadedSeveralT | |
Assert.AreEqual(PackageUploadHandle.State.Error, vm.UploadState); | ||
} | ||
|
||
[Test, Category("Failure")] | ||
[Test] | ||
public void NewPackageVersionUpload_CanAddAndRemoveFiles() | ||
{ | ||
string packagesDirectory = Path.Combine(TestDirectory, "pkgs"); | ||
|
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 are we no longer doing 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.
I found that it does not appear to be necessary to add the assemblies to the MLC as we pass the full path to the file we want to load.
Additionally, we only need the dependencies loaded if want to inspect those types - but we don't do much with the assemblies as far as I can tell except for accessing the location and assembly name.
In an MLC the dependencies are not required to load the assembly AFAIK.
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.
I do not see any immediate need to use those types...but as soon as we do ...we will have exceptions thrown. THese assemblies are not exposed publicly right ?
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.
no, and in addition this would have caused the same issue with ReflectionOnlyLoadContext.
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.
actually, the
PublicPackageViewModel.Assemblies
property is public and so is the class. These classes are only used in the UI layer though AFAIK and are not easily accessible from anywhere but our views, ie not from node or extensions without clients hacking around.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.
So, I'm a bit confused after reading this, just trying to understand it better. The docs seem to say that the resolver that is passed to the mlc resolves assemblies that are dependencies of the assembly being loaded (by the mlc). I can understand that this should include runtime assemblies, which most likely the assembly being loaded (for inspection) has a dependency on but won't it also have dependencies most likely on the other helper assemblies that are in the same package folder itself for the package that's being published? Or are you saying that since the assembly being loaded and the other assemblies are in the same location, that isn't required? Maybe I'm missing something.
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.
yes, the binary we are loading very likely will have dependencies on other binaries in the package - but it doesn't matter. It does not matter because the MLC won't fail to load a binary simply because a dependency is not loaded.
It only becomes an issue when you try to inspect a type and that type has a dependency on an assembly the resolver cannot resolve.
But in our case, as far as I can tell, we don't do any inspection, we just look at the assembly name and location... we're just using the MLC to do some sanity checking that the assembly is a real managed assembly.
ReflectionLoadContext also behaved like this, except instead of a pathresolver, you had to implement the
ReflectionOnlyAssemblyResolve
which we did not do.