-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Print message when install technology is different in upgrade scenarios #1649
Conversation
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.
@@ -1236,4 +1236,10 @@ Please specify one of them using the `--source` option to proceed.</value> | |||
<data name="CountOutOfBoundsError" xml:space="preserve"> | |||
<value>The requested number of results must be between 1 and 1000.</value> | |||
</data> | |||
</root> | |||
<data name="UpgradeDifferentInstallTechnologyInNewerVersions" xml:space="preserve"> | |||
<value>At least a newer package was found but the install technology is different from the current version installed. Please uninstall the package and install a newer version.</value> |
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.
<value>At least a newer package was found but the install technology is different from the current version installed. Please uninstall the package and install a newer version.</value> | |
<value>A newer version was found, but the install technology is different from the current version installed. Please uninstall the package and install the newer version.</value> | |
``` #Resolved |
<data name="UpgradeDifferentInstallTechnologyInNewerVersions" xml:space="preserve"> | ||
<value>At least a newer package was found but the install technology is different from the current version installed. Please uninstall the package and install a newer version.</value> | ||
</data> | ||
<data name="UpgradeDifferentInstallTechnology" xml:space="preserve"> |
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.
It is not clear to me when this would be used over the other string.
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.
UpgradeDifferentInstallTechnologyInNewerVersions is for winget upgrade <id>
and we just look at all versions
UpgradeDifferentInstallTechnology is for specific version winget upgrade <id> -v 2.0
it is different codepaths and i thought it was weird showing "At least a newer package was found..." when a version is specified.
struct InstallerAndInapplicability | ||
{ | ||
std::optional<Manifest::ManifestInstaller> installer; | ||
InapplicabilityFlags inapplicabilityFlags; |
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.
My intention was that there would be a reason for every installer that was not applicable. #Resolved
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.
And ORing all of the reasons together makes for a very strange experience, breaking most of our ability to reason about what could be done to change things.
{ | ||
AICLI_LOG(CLI, Info, << "Installer " << installer << " not applicable: " << filter->ExplainInapplicable(installer)); | ||
return false; | ||
return inapplicability; |
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.
We should run all of them and OR the results together. That way we can confidently say "This is not applicable for this specific reason" rather than telling them then finding out there are also others. #Resolved
@@ -65,8 +67,16 @@ namespace AppInstaller::CLI::Workflow | |||
{ | |||
if (m_reportUpdateNotFound) | |||
{ | |||
context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; | |||
if (inapplicabilityVersions == InapplicabilityFlags::InstalledType) |
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.
This should be "If there is at least one installer whose only reason is InstalledType
". That way we know that in fact the only thing blocking us is the installed version. #Resolved
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentactivatable Globals mytool URIs UtilsTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the [email protected]:msftrubengu/winget-cli.git repository
|
{ | ||
std::optional<Manifest::ManifestInstaller> installer; | ||
InapplicabilityFlags inapplicabilityFlags; | ||
std::vector<InapplicabilityFlags> inapplicabilitiesInstaller; |
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.
It probably doesn't matter right now, but it seems odd to not know which installer each goes with.
It is possible that the winget shows an available update via
winget upgrade
but doingwinget upgrade <id>
results in "No applicable update found." The details of why already shown in the logs.Based on #1191, one of the common patterns we've seen is when the installed technology of the installed version is different to the update version. This change bubbles up the error to make it more visible to users.
The current recommendation is to uninstall and install the new version. In the future, #1640 will provide a way to do it automatically if desired.
Microsoft Reviewers: Open in CodeFlow