DYN-3457: Dynamo should alert the user if there are conflicts loading packages from different locations #11554
Conversation
There was a problem hiding this comment.
I think the intention of this task was to make Dynamo scan all package locations to see if there are going to be potential package conflicts before loading any single one. With these changes, it looks like Dynamo is going to continue loading them one by one, then when it encounters a package conflict with a second location, take appropriate action but by then it's too late - at least one package would already have been loaded.
@aparajit-pratap The intention of this code is to always load the newest version of a package when there are version conflicts. The existing implementation is already doing a pre-scan of all packages but is only removing duplicated packages based on the package name. I extend that logic to also include the version. The actual loading happens later based on the cleaned up LocalPackages property. Lets talk at standup. |
@sm6srw if that's the case, then it's good but I'd like to test your PR once to convince myself that it works especially with the std library. |
Sure! Go ahead. |
sm6srw
changed the title
|
@sm6srw it doesn't look like on first glance that the tests you have added are testing the package conflicts? Am I missing it? Also does it make sense to add a failing test for the workflow when a new package dir is added? |
The last two tests are testing package conflicts (The involved packages have the same name but different versions, I am using the description field as a way to separate the packages in addition to the version number and that is probably what's confusing you. I will try to make that more obvious in the test). Y |
|
Redesigned version where the first found package always wins but with enhanced notifications. PTAL! |
| public string DefaultPackagesDirectory | ||
| { | ||
| get { return packagesDirectories[0]; } | ||
| get { return packagesDirectories[1]; } |
There was a problem hiding this comment.
We need to do something about this property but I think that should be part of @mjkkirschner's PR.
There was a problem hiding this comment.
@sm6srw what were you thinking 😉 ?
| 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, |
There was a problem hiding this comment.
I still don't understand this case. Are we rejecting the new version over here and keeping the old one? If so, why?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Or does that not make sense at this stage?
There was a problem hiding this comment.
I think we should wait with that for now and add that as a separate task if we need it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Purpose
This pull request does:
LibraryLoadFailedExceptionfor packages with invalid versions.Declarations
Check these if you believe they are true
*.resxfiles