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

Get arguments when using archive type package #2660

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Nov 3, 2022

This fixes an issue where default arguments are not being passed to nested installers

Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner November 3, 2022 15:06
@denelon denelon merged commit 171d95b into microsoft:master Nov 3, 2022
@yao-msft
Copy link
Contributor

yao-msft commented Nov 3, 2022

@Trenly Do you have an example where default args are not passed to installer? From my local testing, args were correctly passed to nested installers (without this change).

@Trenly
Copy link
Contributor Author

Trenly commented Nov 3, 2022

@yao-msft - When running this manifest locally
microsoft/winget-pkgs#87332

@yao-msft
Copy link
Contributor

yao-msft commented Nov 3, 2022

@yao-msft - When running this manifest locally microsoft/winget-pkgs#87332

Thanks, let me try a repro.

@Trenly
Copy link
Contributor Author

Trenly commented Nov 3, 2022

@yao-msft - When running this manifest locally microsoft/winget-pkgs#87332

Thanks, let me try a repro.

I should still have the logs around from my local tests if they would be helpful

@yao-msft
Copy link
Contributor

yao-msft commented Nov 3, 2022

Yep, logs should show what args are selected for execution.

@Trenly
Copy link
Contributor Author

Trenly commented Nov 3, 2022

Yep, logs should show what args are selected for execution.

Before PR:
WinGet-2022-11-03-10-03-42.819.log
WinGet-2022-11-03-18-08-32.570.log

Using dev build with PR changes: (Line 61521)
WinGet-2022-11-03-09-58-24.721.log

@yao-msft
Copy link
Contributor

yao-msft commented Nov 3, 2022

I tried the package (without the change) and cannot repro locally.

I'm not sure what leads to empty installer args with the logs you shared Before PR.

But for the log after the pr, I can see the GetInstallerArgs were called twice (Line 61519 amd Line 61520).

@Trenly
Copy link
Contributor Author

Trenly commented Nov 3, 2022

I tried the package (without the change) and cannot repro locally.

I'm not sure what leads to empty installer args with the logs you shared Before PR.

But for the log after the pr, I can see the GetInstallerArgs were called twice (Line 61519 amd Line 61520).

Interesting, because @SpecterShell was experiencing the same issue I was

@yao-msft
Copy link
Contributor

yao-msft commented Nov 3, 2022

Interesting, because @SpecterShell was experiencing the same issue I was

Maybe you are using an old version of winget that does not have this fix? #2413

@yao-msft
Copy link
Contributor

yao-msft commented Nov 3, 2022

The branch is called msiZipBug for #2413, and the description matches exactly what you were experiencing.

@Trenly
Copy link
Contributor Author

Trenly commented Nov 3, 2022

Interesting, because @SpecterShell was experiencing the same issue I was

Maybe you are using an old version of winget that does not have this fix? #2413

Possibly. I thought I tested it with both winget and wingetdev before the change, since I did see that PR earlier. PerhPs I missed somethinf

I agree that it is definitely being called twice though, so this should probably be reverted.

Trenly added a commit to Trenly/winget-cli that referenced this pull request Nov 3, 2022
yao-msft pushed a commit that referenced this pull request Nov 4, 2022
@Trenly Trenly deleted the GetArgsForArchive branch November 4, 2022 02:22
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.

3 participants