Skip to content
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

Add prompts when installer aborts terminal or needs install location #1814

Merged
merged 18 commits into from
Aug 12, 2022

Conversation

florelis
Copy link
Member

@florelis florelis commented Dec 18, 2021

Resolves: #713
Resolves: #970

Changes:

  • Added user setting for default install root
  • Added prompts when an installer has InstallerAbortsTerminal (to confirm that we want to install) or InstallLocationRequired (to provide the install location).
  • Refactored the existing package agreements prompt to a common setup also used by the two new prompts. Before installing a package, we walk through all the possible prompts and show them if applicable. When installing multiple packages, we check all the packages that need the prompt and then show the prompt once.

Remaining items:

  • Testing
  • Use install root from settings
  • Update settings doc
  • Move installers that abort the terminal to the end on upgrade all/import
  • Skip prompts when running non-interactively
Microsoft Reviewers: Open in CodeFlow

Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

My comments on the default install root setting also apply to the install location argument on a multipackage install. I'm just not sure it makes sense to have one location apply to multiple packages.

doc/Settings.md Outdated Show resolved Hide resolved
src/AppInstallerCLICore/ExecutionReporter.cpp Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jan 20, 2022
@github-actions

This comment was marked as outdated.

@florelis
Copy link
Member Author

Note that this is still missing tests, validation of the install root and the arg/setting to disable prompts. (Hence still a draft)

I had thought of moving installers that abort the terminal to the end so as to minimize their impact, but that seems tricky with dependencies.

@florelis florelis marked this pull request as ready for review July 28, 2022 19:25
@florelis florelis requested a review from a team as a code owner July 28, 2022 19:25
@florelis
Copy link
Member Author

Reviving this old PR...

Main changes from last time:

  • Rebased on recent main branch
  • Added a setting and a CLI arg to prevent all prompts
  • Added unit tests

@ghost ghost added Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client. labels Aug 5, 2022
doc/Settings.md Show resolved Hide resolved
schemas/JSON/settings/settings.schema.0.2.json Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Argument.cpp Outdated Show resolved Hide resolved
# Conflicts:
#	src/AppInstallerCLICore/ExecutionArgs.h
#	src/AppInstallerCLIE2ETests/Constants.cs
#	src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
#	src/AppInstallerCommonCore/Public/AppInstallerErrors.h
#	src/AppInstallerCommonCore/Public/winget/UserSettings.h
#	src/AppInstallerCommonCore/UserSettings.cpp
# Conflicts:
#	src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
@florelis
Copy link
Member Author

This PR is having issues with the COM InProc E2E tests because I added something that causes us to load the string resources in a place we weren't doing it before, and the tests fail there because they cannot find the resources. I still don't know how to fix it (and it would make this change bigger), so I'm sidestepping it for now by preventing this new use of the ResourceLoader. I'll open an issue for actually fixing it because I imagine it will hit us again later.

@florelis florelis merged commit d9f2688 into microsoft:master Aug 12, 2022
@florelis florelis deleted the prompts branch August 12, 2022 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default install root unexpected behaviour - Installation abandoned
2 participants