Skip to content

Comments

Copy install behavior flags on upgrade --all#2794

Merged
florelis merged 5 commits intomicrosoft:masterfrom
florelis:forwardUpgradeFlags
Jan 10, 2023
Merged

Copy install behavior flags on upgrade --all#2794
florelis merged 5 commits intomicrosoft:masterfrom
florelis:forwardUpgradeFlags

Conversation

@florelis
Copy link
Member

@florelis florelis commented Dec 22, 2022

Issue:
When we install multiple packages in the same execution (upgrade --all and import), we create a sub-context to install each individual package but this sub-context does not respect the parent's install behavior flags, like --silent.

Change:
Added a step to the creation of a sub-context to copy the relevant flags from the parent context, so that the same behavior is used for all packages. Added a test to verify the new behavior.


This came from a user running winget upgrade --all --silent and expecting all the upgrades to be done silently. We need to either respect those flags (this PR) or change the argument validation to forbid that combination.

I figure the most contentious issue with this PR will be which args we should forward. I added basically everything that made sense to me, but we can trim the list as much as needed.

Fixes #2008 and #2793

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner December 22, 2022 00:26
@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Dec 22, 2022
{
// Copy over install behavior flags from the parent context.
// Arguments not copied: Override, Log
for (auto flag : {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to copy over some other behavior args like DisableInteractivity, Force, AcceptPackageAgreements, etc? I feel this needs a more thorough revist.

I also feel this selective copying is hard to maintain but I cannot find a better way at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

DisableInteractivity and AcceptPackageAgreements don't need to be copied over because those are handled at the top level. Unless we have nested contexts...

I've been thinking we can add some tags/flags about what an argument is for. Like, IsInstallBehavior, IsGlobalBehavior, IsQueryArgument. Then we could do this copying based on that, and we could also use it for argument validation. For example, Manifest cannot be used with query/source arguments, and installer specific args (like override) cannot be used when dealing with multiple packages (update --all)

Copy link
Contributor

@yao-msft yao-msft Jan 10, 2023

Choose a reason for hiding this comment

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

I just went over all the args, I suggest to copy over Interactive, Silent, Locale, InstallScope, InstallArchitecture, HashOverride, IgnoreLocalArchiveMalwareScan, NoUpgrade, Force

Regarding InstallLocation, I saw in PromptForMultiplePackages we set them all to same value already, but I feel it will be more likely to cause conflicts for packages if installerss write to the InstallLocation root. Perhaps we should add a todo somewhere to revisit this. And this copy is probably not needed (also for dependencies, user may not want to impact dependency package installlocation based on main package InstallLocation).

Copy link
Contributor

Choose a reason for hiding this comment

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

@florelis / @yao-msft; Regarding this comment -

DisableInteractivity and AcceptPackageAgreements don't need to be copied over because those are handled at the top level. Unless we have nested contexts...

I just stumbled across this issue:

Perhaps the "top-level" flags do need to be copied over?

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

Please fix the tests before merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue-Bug It either shouldn't be doing this or needs an investigation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

winget (v1.2) upgrade --all --silent only installs the first package silently

3 participants