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 --custom argument for passing additional installer arguments #2832

Merged
merged 6 commits into from
Jan 12, 2023

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Jan 10, 2023


  • I chose the argument based on what made sense to me and what was in the referenced issues. If a different name is desired, I'm open to suggestions.
  • Decided to name the arg type CustomSwitches to avoid future confusion; again, if there's a better name, feel free to suggest.
// Installation failure was expected as the '/?' argument brings up the MSI help menu

PS D:\Git\winget-pkgs> wingetdev install --manifest D:\Git\winget-pkgs\manifests\7\7zip\7zip\22.01 --custom /? --verbose --open-logs
Found 7-Zip [7zip.7zip] Version 22.01
This application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
Downloading https://www.7-zip.org/a/7z2201-x64.msi
  ██████████████████████████████  1.82 MB / 1.82 MB
Successfully verified installer hash
Starting package install...
Installer failed with exit code: 1639
One of the installation parameters is invalid. The package installation log may have additional details.
2023-01-09 22:29:04.465 [CLI ] Installer args: /passive /norestart /log "C:\Users\Trenly\AppData\Local\Packages\WinGetDevCLI_8wekyb3d8bbwe\LocalState\DiagOutputDir\WinGet-7zip.7zip.22.01-2023-01-09-22-29-04.465.log" /?
2023-01-09 22:29:04.466 [CLI ] Starting: 'C:\Users\Trenly\AppData\Local\Temp\WinGet\7zip.7zip.22.01\7z2201-x64.msi' with arguments '/passive /norestart /log "C:\Users\Trenly\AppData\Local\Packages\WinGetDevCLI_8wekyb3d8bbwe\LocalState\DiagOutputDir\WinGet-7zip.7zip.22.01-2023-01-09-22-29-04.465.log" /?'
Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner January 10, 2023 04:36
@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft
Copy link
Contributor

Thanks for the pr. The change looks great. But as this will be a supported feature, for feature parity, we'd better add it to Com api around here with related implementation, also this should be added as contract version 6 since winget v1.4 just got finalized. And this is also a good one to update Powershell module around here with related tests.

Or alternatively, if you feel not comfortable adding above changes at the moment, we can add it behind an experimental feature, and we'll implement the Com api and PS module during winget v1.5 milestone and remove experimental feature later.

@Trenly
Copy link
Contributor Author

Trenly commented Jan 11, 2023

Thanks for the pr. The change looks great. But as this will be a supported feature, for feature parity, we'd better add it to Com api around here with related implementation, also this should be added as contract version 6 since winget v1.4 just got finalized. And this is also a good one to update Powershell module around here with related tests.

Or alternatively, if you feel not comfortable adding above changes at the moment, we can add it behind an experimental feature, and we'll implement the Com api and PS module during winget v1.5 milestone and remove experimental feature later.

Thanks for the feedback! I think I got it implemented, but I couldn't find where the tests were for the PowerShell module. Any guidance you can provide on where I should be adding additional tests would be appreciated

@Trenly
Copy link
Contributor Author

Trenly commented Jan 11, 2023

I also decided to add some whitespace handling just in case. I didn't think it made sense to error on whitespace, but if there's some other way you'd like it handled (or if you'd like the whitespace handling reverted) just let me know

@yao-msft
Copy link
Contributor

Thanks for the pr. The change looks great. But as this will be a supported feature, for feature parity, we'd better add it to Com api around here with related implementation, also this should be added as contract version 6 since winget v1.4 just got finalized. And this is also a good one to update Powershell module around here with related tests.
Or alternatively, if you feel not comfortable adding above changes at the moment, we can add it behind an experimental feature, and we'll implement the Com api and PS module during winget v1.5 milestone and remove experimental feature later.

Thanks for the feedback! I think I got it implemented, but I couldn't find where the tests were for the PowerShell module. Any guidance you can provide on where I should be adding additional tests would be appreciated

Thank you. The Powershell tests are here. But after a closer look, they are just some basic smoke tests. So it's probably fine with the current unit tests only.

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

:shipit:

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft yao-msft merged commit bf9914a into microsoft:master Jan 12, 2023
@Trenly Trenly deleted the AdditionalArgs branch January 12, 2023 04:30
@denelon
Copy link
Contributor

denelon commented Jan 13, 2023

We're running out of letters :)

Naming things is hard.

--augment
--custom
--pass-args
--switches

What happens if we have both "--override" and this new argument?

I had a discussion with the Visual Studio team about this feature a few days ago. It's common for users to want to add switches to their Visual Studio install so they can include workloads. We might also have a situation where the default install from WinGet is "Silent with progress" and the user attempts to add another switch that conflicts with the ones WinGet already has for install mode or scope. I don't know if installers would take the first argument, the last argument, or throw an exception.

@Trenly
Copy link
Contributor Author

Trenly commented Jan 13, 2023

What happens if we have both "--override" and this new argument?

Override takes precedence and only the value for override is used

I had a discussion with the Visual Studio team about this feature a few days ago. It's common for users to want to add switches to their Visual Studio install so they can include workloads. We might also have a situation where the default install from WinGet is "Silent with progress" and the user attempts to add another switch that conflicts with the ones WinGet already has for install mode or scope. I don't know if installers would take the first argument, the last argument, or throw an exception.

Most installers would throw exception (from what I've seen). However, the idea of "adding your own switches" to the default switches winget uses is almost certainly a power user feature, or at least one that most users should understand to check their input before blaming the cli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

possibility to add parameters for installation of package
3 participants