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

Revert start /wait for msiexec and change nulls to ? in UTF-8 mode. #412

Merged
merged 2 commits into from
Mar 14, 2022

Conversation

BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Mar 3, 2022

microsoft/vcpkg#23367 was a failure because we still end up trying to download portable git; however as some combination of changes in attempt to debug we drop all msiexec output on the floor now.

This reverts change to add start /wait, replaces nulls with ?s.

@Neumann-A
Copy link
Contributor

as far as I can tell it worked before #394 was merge right? So why not revert that change?

@BillyONeal
Copy link
Member Author

as far as I can tell it worked before #394 was merge right? So why not revert that change?

I don't know which of the 2 changes were the cause of the problem and I think this replace-nulls-with-? behavior is better overall.

@Neumann-A
Copy link
Contributor

It definetly is #394 since @ras0219-msft used the utf16 changes in the boost PR and the output worked.

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Mar 8, 2022

The start /wait was the change that broke it for me. Additionally, I have not observed issues with not waiting long enough via just cmd /c.

That said, I do support this change to reduce the dependence on "knowing" the encoding of the child process. While it's unlikely to happen to msiexec, I think it's easy to imagine error scenarios in other tools where the encoding can become extremely runtime-dependent. Barring some future heuristic like "If there is at least one embedded null, attempt to decode as UTF-16", I think this is the next best thing. I don't believe that hardcoding the expected encoding of child processes will result in a reliable system.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 8, 2022

The start /wait was the change that broke it for me. Additionally, I have not observed issues with not waiting long enough via just cmd /c.

I'm sorry for the unintented side effect. The usage of misexec in vcpkg is limited, but my history with this tool is > 10 years. There were lot of strange msi packages and error contexts in that time.

@BillyONeal
Copy link
Member Author

I simplified this a lot based on the feedback above; it preserves the ability to interpret as UTF-16 when we know that's the case, as we do for msiexec, but also keeps the null to ? replacement.

@BillyONeal BillyONeal changed the title Try again to get msiexec output. Revert start /wait for msiexec and change nulls to ? in UTF-8 mode. Mar 12, 2022
@BillyONeal
Copy link
Member Author

I have an approval now and Robert is out sick so I'm merging this. Thanks for the reviews folks!

@BillyONeal BillyONeal merged commit 2920672 into microsoft:main Mar 14, 2022
@BillyONeal BillyONeal deleted the msiexec-output-again branch March 14, 2022 18:31
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.

5 participants