Pkg builtin bug #12308
Pkg builtin bug #12308
Conversation
|
Is this fixing the case which was the whole reason for allowing a built-in package to be unloaded? |
There are still a couple of issues with this scenario (working on it) Without this PR...even if you follow the stepss described in the warning message...you still will not be able to install the desired package |
src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs
Outdated
Show resolved
Hide resolved
|
It looks good but there are 2 failing tests. |
|
Best to disable whitespaces in the PR diff viewer |
| return; | ||
| if (!dependencyVersionHeaders.Any(x => x.name == name)) | ||
| {// Add the main package if it does not exist | ||
| dependencyVersionHeaders.Add(package); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (name.Equals(localPkgWithSameName.Name)) | ||
| {// Main package has a duplicate in local packages | ||
| duplicatePackage = localPkgWithSameName; | ||
| break;// Download will not be able to continue |
There was a problem hiding this comment.
@aparajit-pratap can you confirm that this was an intended change in your previous PR ?
If the main package (the one that the user wants to download) has a conflict with an existing package ...then download will not be possible without the user having to intervene (remove existing or disabling package paths etc)
It seems like this was possible before ... if the existing package had no assemblies loaded (or was not in use)
There was a problem hiding this comment.
Do you mean that before, we allowed multiple packages of different versions but the same name to be installed at the same time if they didn't have assemblies?
There was a problem hiding this comment.
I think your PR allowed it too.
My question is :
Should the user be able to immediately download (without restart) another version of an already installed package (that has no assemblies loaded) ? Uninstalling the existing package should be possible without restart.
My PR disallows this behavior...
There was a problem hiding this comment.
I don't think that we should allow download. If my PR allowed it then it probably was an oversight on my part.
There was a problem hiding this comment.
I do not understand why we should not allow it though...
Just so that we do not complicate the workflow too much ?
There was a problem hiding this comment.
Well, yes. If it was allowed after my PR, it was purely unintentional. As you say, I think it will complicate things just because the user will start seeing apparently duplicate nodes in the library and not know which node comes from which package version. We could discuss this as a team if you prefer. I would also like to confirm what the behavior was before 2.13. If we didn't allow it before, I think we should just keep it that way.
There was a problem hiding this comment.
I misunderstood this scenario. My previous comment assumed that you will not be uninstalling the conflicting package but it looks like you'll first uninstall it and only then install the new one, which I think is okay.
| if (dialogResult == MessageBoxResult.OK) | ||
| { | ||
| // mark for uninstallation | ||
| duplicatePackage.MarkForUninstall(DynamoViewModel.Model.PreferenceSettings); |
There was a problem hiding this comment.
should we be returning with true here ?
There was a problem hiding this comment.
I don't think so as we will not be going ahead with the download.
There was a problem hiding this comment.
What when duplicatePackage has no assemblies loaded. I think this may be a regression ...
There was a problem hiding this comment.
Again, here it could complicate things as well as we could end up with multiple nodes of the same name from different package versions. However, if this is confirmed to be a regression, then I agree, let's not break it - check if it's a non-assembly package, then install it (return true).
src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs
Outdated
Show resolved
Hide resolved
src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs
Outdated
Show resolved
Hide resolved
src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs
Outdated
Show resolved
Hide resolved
| if (dialogResult == MessageBoxResult.OK) | ||
| { | ||
| // mark for uninstallation | ||
| duplicatePackage.MarkForUninstall(DynamoViewModel.Model.PreferenceSettings); |
There was a problem hiding this comment.
I don't think so as we will not be going ahead with the download.
| if (name.Equals(localPkgWithSameName.Name)) | ||
| {// Main package has a duplicate in local packages | ||
| duplicatePackage = localPkgWithSameName; | ||
| break;// Download will not be able to continue |
There was a problem hiding this comment.
Do you mean that before, we allowed multiple packages of different versions but the same name to be installed at the same time if they didn't have assemblies?
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs
Outdated
Show resolved
Hide resolved
| { | ||
| dialogResult = MessageBoxService.Show(String.Format(Resources.MessageAlreadyInstallDynamo, | ||
| DynamoViewModel.BrandingResourceProvider.ProductName, dupPkg, packageToDownload), | ||
| Resources.CannotDownloadPackageMessageBoxTitle, |
There was a problem hiding this comment.
Does CannotDownloadPackageMessageBoxTitle title still make sense now as the package can now be installed right?
There was a problem hiding this comment.
replaced with pre-existing PackagesInUseConflictMessageBoxTitle
Package has conflicts with one or more packages in use.
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
Co-Authored-By: pinzart <[email protected]>
* filter only loaded blt in packages * Update PackageManagerUITests.cs * Update PackageManagerClientViewModel.cs
* filter only loaded blt in packages * Update PackageManagerUITests.cs * Update PackageManagerClientViewModel.cs
* Pkg builtin bug (#12308) * filter only loaded blt in packages * Update PackageManagerUITests.cs * Update PackageManagerClientViewModel.cs * remove bad merge Co-authored-by: pinzart <[email protected]>
Allow users to download a non-built in package if the built-in package is not Loaded (Unloaded or Error).
Release Notes
Allow users to download a non-built in package if the built-in package is not Loaded (Unloaded or Error).
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of