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

DYN-3457: Dynamo should alert the user if there are conflicts loading packages from different locations #11554

Merged
merged 20 commits into from
Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/DynamoCore/Properties/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions src/DynamoCore/Properties/Resources.en-US.resx
Original file line number Diff line number Diff line change
Expand Up @@ -735,4 +735,13 @@ Parameter name: {0}</value>
<value>The current user, '{0}', is not a maintainer of the package '{1}'.</value>
<comment>{0} = user name (i.e. 'DynamoTeam'), {1} = package name (i.e. 'Clockwork for Dynamo 1.x')</comment>
</data>
<data name="DuplicatedOlderPackage" xml:space="preserve">
<value>An older version of the package called {0} version {2} was found at {1} with version {3}. The older version has been ignored.</value>
</data>
<data name="InvalidPackageVersion" xml:space="preserve">
<value>The version of the package called {0} found at {1} is invalid (version: "{2}"). Ignoring it.</value>
</data>
<data name="DuplicatedNewerPackage" xml:space="preserve">
<value>A newer version of the package called {0} version {2} was found at {1} with version {3}. The newer version has been ignored.</value>
</data>
</root>
9 changes: 9 additions & 0 deletions src/DynamoCore/Properties/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -735,4 +735,13 @@ Parameter name: {0}</value>
<value>The current user, '{0}', is not a maintainer of the package '{1}'.</value>
<comment>{0} = user name (i.e. 'DynamoTeam'), {1} = package name (i.e. 'Clockwork for Dynamo 1.x')</comment>
</data>
<data name="DuplicatedOlderPackage" xml:space="preserve">
<value>An older version of the package called {0} version {2} was found at {1} with version {3}. The older version has been ignored.</value>
</data>
<data name="InvalidPackageVersion" xml:space="preserve">
<value>The version of the package called {0} found at {1} is invalid (version: "{2}"). Ignoring it.</value>
</data>
sm6srw marked this conversation as resolved.
Show resolved Hide resolved
<data name="DuplicatedNewerPackage" xml:space="preserve">
<value>A newer version of the package called {0} version {2} was found at {1} with version {3}. The newer version has been ignored.</value>
</data>
</root>
79 changes: 68 additions & 11 deletions src/DynamoPackages/PackageLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,13 @@ public IEnumerable<IExtension> RequestedExtensions
public IEnumerable<Package> LocalPackages { get { return localPackages; } }

private readonly List<string> packagesDirectories = new List<string>();

// Returns the default package directory where new packages will be installed
// The default package directory is currently the second entry in the list
// The first entry is the standard library
public string DefaultPackagesDirectory
sm6srw marked this conversation as resolved.
Show resolved Hide resolved
{
get { return packagesDirectories[0]; }
get { return packagesDirectories[1]; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to do something about this property but I think that should be part of @mjkkirschner's PR.

Copy link
Member

Choose a reason for hiding this comment

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

@sm6srw what were you thinking 😉 ?

}

private readonly List<string> packagesDirectoriesToVerifyCertificates = new List<string>();
Expand All @@ -91,8 +95,8 @@ public PackageLoader(IEnumerable<string> packagesDirectories)
if (packagesDirectories == null)
throw new ArgumentNullException("packagesDirectories");

this.packagesDirectories.AddRange(packagesDirectories);
this.packagesDirectories.Add(StandardLibraryDirectory);
this.packagesDirectories.AddRange(packagesDirectories);

var error = PathHelper.CreateFolderIfNotExist(DefaultPackagesDirectory);

Expand Down Expand Up @@ -401,13 +405,13 @@ public Package ScanPackageDirectory(string directory, bool checkCertificates)
{
var headerPath = Path.Combine(directory, "pkg.json");

Package discoveredPkg;
Package discoveredPackage;

// get the package name and the installed version
if (PathHelper.IsValidPath(headerPath))
{
discoveredPkg = Package.FromJson(headerPath, AsLogger());
if (discoveredPkg == null)
discoveredPackage = Package.FromJson(headerPath, AsLogger());
if (discoveredPackage == null)
throw new LibraryLoadFailedException(directory, String.Format(Properties.Resources.MalformedHeaderPackage, headerPath));
}
else
Expand All @@ -418,16 +422,50 @@ public Package ScanPackageDirectory(string directory, bool checkCertificates)
// prevent loading unsigned packages if the certificates are required on package dlls
if (checkCertificates)
{
CheckPackageNodeLibraryCertificates(directory, discoveredPkg);
CheckPackageNodeLibraryCertificates(directory, discoveredPackage);
}

// prevent duplicates
if (LocalPackages.All(pkg => pkg.Name != discoveredPkg.Name))
var discoveredVersion = CheckAndGetPackageVersion(discoveredPackage.VersionName, discoveredPackage.Name, discoveredPackage.RootDirectory);

var existingPackage = LocalPackages.FirstOrDefault(package => package.Name == discoveredPackage.Name);
sm6srw marked this conversation as resolved.
Show resolved Hide resolved

// Is this a new package?
if (existingPackage == null)
{
// Yes
Add(discoveredPackage);
return discoveredPackage; // success
}

var existingVersion = CheckAndGetPackageVersion(existingPackage.VersionName, existingPackage.Name, existingPackage.RootDirectory);

// Is this a duplicated package?
if (existingVersion == discoveredVersion)
{
// Duplicated with the same version
throw new LibraryLoadFailedException(directory,
String.Format(Properties.Resources.DulicatedPackage,
discoveredPackage.Name,
discoveredPackage.RootDirectory));
}

// Is the existing version newer?
if (existingVersion > discoveredVersion)
{
Add(discoveredPkg);
return discoveredPkg; // success
// Older version found, show notification
throw new LibraryLoadFailedException(directory, String.Format(Properties.Resources.DuplicatedOlderPackage,
existingPackage.Name,
discoveredPackage.RootDirectory,
existingVersion.ToString(),
discoveredVersion.ToString()));
}
throw new LibraryLoadFailedException(directory, String.Format(Properties.Resources.DulicatedPackage, discoveredPkg.Name, discoveredPkg.RootDirectory));

// Newer version found, show notification.
throw new LibraryLoadFailedException(directory, String.Format(Properties.Resources.DuplicatedNewerPackage,
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't understand this case. Are we rejecting the new version over here and keeping the old one? If so, why?

Copy link
Contributor Author

@sm6srw sm6srw Mar 25, 2021

Choose a reason for hiding this comment

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

Yes, this is the case where we keep the old one and reject the new one. This was the major change I made after the feedback yesterday. Instead of using the new version we just inform the user that we have found a newer version that we will ignore because we have already found another version of the package. That should give them enough information so they can decide what directory order they want.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about raising the existing mark for uninstall dialog here asking the user if they'd like to mark the existing package for uninstall and install the newer version upon restart or to simply ignore the new version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or does that not make sense at this stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should wait with that for now and add that as a separate task if we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's okay, but just to clarify, I don't think this case is at startup, it's at the time you add a package path if I'm not wrong.

Copy link
Contributor Author

@sm6srw sm6srw Mar 25, 2021

Choose a reason for hiding this comment

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

It can happen at startup. Package X version 1.0.0 is installed in standard library. User download version 2.0.0. The user will get the message above every time they start after that as long as the standard library is first in the directory list. That is one of those cases when the user can't uninstall the old package because most of them will not have write access to the standard library. So lets see how this plays out and go from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt there's gonna be a case where we deliver a package in the std lib with a version that's older than that available on the PM, since that's the decision that we're making as a team, but I agree, it's not impossible. It could happen with host integrator packages, etc.

Copy link
Member

@mjkkirschner mjkkirschner Mar 25, 2021

Choose a reason for hiding this comment

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

I can imagine we might create a new version of meshtoolkit say for release to PM to get early feedback before incorporation of the new version into std.lib.

Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

existingPackage.Name,
discoveredPackage.RootDirectory,
existingVersion.ToString(),
discoveredVersion.ToString()));
}
catch (Exception e)
{
Expand All @@ -438,6 +476,25 @@ public Package ScanPackageDirectory(string directory, bool checkCertificates)
return null;
}

/// <summary>
/// Check and get the version from the version string. Throw a library load exception if anything is wrong with the version
/// </summary>
/// <param name="version">the version string</param>
/// <param name="name">name of the package</param>
/// <param name="directory">package directory</param>
/// <returns>Returns a valid Version</returns>
private Version CheckAndGetPackageVersion(string version, string name, string directory)
{
try
{
return VersionUtilities.PartialParse(version);
}
catch (Exception e) when (e is ArgumentException || e is FormatException || e is OverflowException)
{
throw new LibraryLoadFailedException(directory, String.Format(Properties.Resources.InvalidPackageVersion, name, directory, version));
}
}

/// <summary>
/// Check the node libraries defined in the package json file are valid and have a valid certificate
/// </summary>
Expand Down
96 changes: 96 additions & 0 deletions test/Libraries/PackageManagerTests/PackageLoaderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,102 @@ public void HasValidStandardLibraryPath()
Assert.AreEqual(standardDirectory, directory);
}

[Test]
public void PackageLoaderLoadPackageWithBadVersion()
{
// Arrange
var loader = GetPackageLoader();
var badPackageLocation = Path.Combine(PackagesDirectory, @"BadVersion\PackageWithBadVersion");

// Act
var badPackage = loader.ScanPackageDirectory(badPackageLocation);

// Assert
Assert.IsNull(badPackage);
Assert.IsNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Bad package"));
}

[Test]
public void PackageLoaderLoadMultiplePackagesWithBadVersion()
{
// Arrange
var loader = GetPackageLoader();
var goodPackageLocation = Path.Combine(PackagesDirectory, @"BadVersion\PackageWithGoodVersion");
var badPackageLocation = Path.Combine(PackagesDirectory, @"BadVersion\PackageWithBadVersion");

// Act
var goodPackage = loader.ScanPackageDirectory(goodPackageLocation);
var badPackage = loader.ScanPackageDirectory(badPackageLocation);

// Assert
Assert.IsNotNull(goodPackage);
Assert.IsNull(badPackage);
Assert.IsNotNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Good package"));
Assert.IsNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Bad package"));
}

[Test]
public void PackageLoaderLoadMultiplePackagesWithBadVersionReversed()
{
// Arrange
var loader = GetPackageLoader();
var goodPackageLocation = Path.Combine(PackagesDirectory, @"BadVersion\PackageWithGoodVersion");
var badPackageLocation = Path.Combine(PackagesDirectory, @"BadVersion\PackageWithBadVersion");

// Act
var badPackage = loader.ScanPackageDirectory(badPackageLocation);
var goodPackage = loader.ScanPackageDirectory(goodPackageLocation);

// Assert
Assert.IsNotNull(goodPackage);
Assert.IsNull(badPackage);
Assert.IsNotNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Good package"));
Assert.IsNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Bad package"));
}


[Test]
public void PackageLoaderLoadNewPackage()
{
// Arrange
var loader = GetPackageLoader();
var oldPackageLocation = Path.Combine(PackagesDirectory, @"Version\PackageWithOldVersion");
var newPackageLocation = Path.Combine(PackagesDirectory, @"Version\PackageWithNewVersion");

// Act
var oldPackage = loader.ScanPackageDirectory(oldPackageLocation);
var newPackage = loader.ScanPackageDirectory(newPackageLocation);

// Assert
Assert.IsNotNull(oldPackage);
Assert.IsNull(newPackage);
Assert.AreEqual("Package", oldPackage.Name);
Assert.AreEqual("1.0.0", oldPackage.VersionName);
Assert.IsNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"New package"));
Assert.IsNotNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Old package"));
}

[Test]
public void PackageLoaderLoadOldPackage()
{
// Arrange
var loader = GetPackageLoader();
var oldPackageLocation = Path.Combine(PackagesDirectory, @"Version\PackageWithOldVersion");
var newPackageLocation = Path.Combine(PackagesDirectory, @"Version\PackageWithNewVersion");

// Act
var newPackage = loader.ScanPackageDirectory(newPackageLocation);
var oldPackage = loader.ScanPackageDirectory(oldPackageLocation);

// Assert
Assert.IsNull(oldPackage);
Assert.IsNotNull(newPackage);
Assert.AreEqual("Package", newPackage.Name);
Assert.AreEqual("2.0.0", newPackage.VersionName);
Assert.IsNotNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"New package"));
Assert.IsNull(loader.LocalPackages.FirstOrDefault(package => package.Description == @"Old package"));
}

[Test]
public void IsUnderPackageControlIsCorrectForValidFunctionDefinition()
{
Expand Down
Binary file not shown.
1 change: 1 addition & 0 deletions test/pkgs/BadVersion/PackageWithBadVersion/pkg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"license":"","file_hash":null,"name":"Package","version":"","description":"Bad package","group":"","keywords":null,"dependencies":[],"contents":"","engine_version":"2.1.0.7840","engine":"dynamo","engine_metadata":"","site_url":"","repository_url":"","contains_binaries":true,"node_libraries":["Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"]}
sm6srw marked this conversation as resolved.
Show resolved Hide resolved
Binary file not shown.
1 change: 1 addition & 0 deletions test/pkgs/BadVersion/PackageWithGoodVersion/pkg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"license":"","file_hash":null,"name":"Package","version":"1.0.0","description":"Good package","group":"","keywords":null,"dependencies":[],"contents":"","engine_version":"2.1.0.7840","engine":"dynamo","engine_metadata":"","site_url":"","repository_url":"","contains_binaries":true,"node_libraries":["Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"]}
Binary file not shown.
1 change: 1 addition & 0 deletions test/pkgs/Version/PackageWithNewVersion/pkg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"license":"","file_hash":null,"name":"Package","version":"2.0.0","description":"New package","group":"","keywords":null,"dependencies":[],"contents":"","engine_version":"2.1.0.7840","engine":"dynamo","engine_metadata":"","site_url":"","repository_url":"","contains_binaries":true,"node_libraries":["Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"]}
Binary file not shown.
1 change: 1 addition & 0 deletions test/pkgs/Version/PackageWithOldVersion/pkg.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"license":"","file_hash":null,"name":"Package","version":"1.0.0","description":"Old package","group":"","keywords":null,"dependencies":[],"contents":"","engine_version":"2.1.0.7840","engine":"dynamo","engine_metadata":"","site_url":"","repository_url":"","contains_binaries":true,"node_libraries":["Package, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"]}