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

Run msiexec with start /wait #394

Merged
merged 1 commit into from
Feb 28, 2022
Merged

Run msiexec with start /wait #394

merged 1 commit into from
Feb 28, 2022

Conversation

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 27, 2022

Relevant e2e test still passing:

[end-to-end-tests.ps1] [14/22] Running suite D:\a\1\s\azure-pipelines\end-to-end-tests-dir\fetch.ps1
D:\a\1\s\build.x86.debug\vcpkg.exe --triplet x86-windows --x-buildtrees-root=D:\a\1\s\build.x86.debug\work\testing\buildtrees --x-install-root=D:\a\1\s\build.x86.debug\work\testing\installed --x-packages-root=D:\a\1\s\build.x86.debug\work\testing\packages --overlay-ports=D:\a\1\s\azure-pipelines/e2e_ports/overlays --overlay-triplets=D:\a\1\s\azure-pipelines/e2e_ports/triplets fetch 7zip --vcpkg-root=D:\a\1\s\build.x86.debug\work\testing
A suitable version of 7zip was not found (required v19.0.0). Downloading portable 7zip v19.0.0...
Downloading 7zip...
  https://www.7-zip.org/a/7z1900-x64.msi -> D:\a\1\s\build.x86.debug\work\testing\down loads\7z1900-x64.msi
Extracting 7zip...
D:\a\1\s\build.x86.debug\work\testing\down loads\tools\7zip-19.00-windows\Files\7-Zip\7z.exe

@dg0yt dg0yt marked this pull request as ready for review February 27, 2022 17:00
@vicroms vicroms merged commit 64df030 into microsoft:main Feb 28, 2022
@BillyONeal
Copy link
Member

I believe this is unlikely to change anything about the situation.

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 1, 2022

I believe this is unlikely to change anything about the situation.

At least is should improve error reporting and repeated attempts.

@BillyONeal
Copy link
Member

At least is should improve error reporting and repeated attempts.

I don't believe it will, because /c already has the effect here. If we were not waiting for exit things would have been long broken long before now.

@BillyONeal
Copy link
Member

Some combination of this change and #397 resulted in us now getting no output :(.

Looking at it again I think both changes were wrong and the actual problem is that we don't report the partial read that happens for the last stdout write from the child:

Loading dependency information for 187 packages...
[DEBUG] CreateProcessW("D:\downloads\tools\cmake-3.22.2-windows\cmake-3.22.2-windows-i386\bin\cmake.exe" --version)
[DEBUG] ReadFile() finished with GetLastError(): 109
[DEBUG] 6124: cmd_execute_and_stream_data() returned 0 after    31460 us
[DEBUG] CreateProcessW("D:\downloads\tools\cmake-3.22.2-windows\cmake-3.22.2-windows-i386\bin\cmake.exe" -DVCPKG_ROOT_DIR=C:/a/1/s -DPACKAGES_DIR=D:/packages -DBUILDTREES_DIR=D:/buildtrees -D_VCPKG_INSTALLED_DIR=D:/installed -DDOWNLOADS=D:/downloads -DVCPKG_MANIFEST_INSTALL=OFF -P "D:\buildtrees\0.vcpkg_dep_info.cmake")
[DEBUG] ReadFile() finished with GetLastError(): 109
[DEBUG] 6124: cmd_execute_and_stream_data() returned 0 after    36684 us
Loading dependency information for 513 packages...

Note that ReadFile is getting ERROR_BROKEN_PIPE.

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Mar 3, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 4, 2022

I think it would help to have a test which checks how errors are handled.

@BillyONeal
Copy link
Member

I think it would help to have a test which checks how errors are handled.

If you have ideas on how to write such a test I think we would be all ears.

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Mar 11, 2022
@dg0yt dg0yt deleted the 7zip-msi branch March 11, 2022 07:06
BillyONeal added a commit that referenced this pull request Mar 14, 2022
…412)

* Revert "Run msiexec with start /wait (#394)"

This reverts commit 64df030.

* In UTF-8 mode, replace nulls with ?s.
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