Copy install behavior flags on upgrade --all #2794
Conversation
ghost
added
the
Issue-Bug
| { | ||
| // Copy over install behavior flags from the parent context. | ||
| // Arguments not copied: Override, Log | ||
| for (auto flag : { |
There was a problem hiding this comment.
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.
There was a problem hiding 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'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)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Issue:
When we install multiple packages in the same execution (
upgrade --allandimport), 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 --silentand 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