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

[vcpkgTools.xml] Add 7zip binary #40416

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

JavierMatosD
Copy link
Contributor

This just adds the 7zip binaries to vckpgTools in preparation to use 7zip for all extraction on windows.

@Neumann-A
Copy link
Contributor

Question:
a) Why add extra stuff to vcpkgTools.xml I thought it should die?
b) Why prefer 7zip over cmake -E tar ? (yeah I know 7zip can deal with all those nasty self installing binaries and NIST installers etc.)
c) If you don't want to kill vcpkgTools.xml anymore please add an "arch" entry. That has been missing for ages.

@MonicaLiu0311 MonicaLiu0311 added info:internal This PR or Issue was filed by the vcpkg team. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Aug 13, 2024
@JavierMatosD
Copy link
Contributor Author

Question: a) Why add extra stuff to vcpkgTools.xml I thought it should die? b) Why prefer 7zip over cmake -E tar ? (yeah I know 7zip can deal with all those nasty self installing binaries and NIST installers etc.)

As of this moment, I am not sure what the plan is with vcpkgTools.xml. What's motivating this change is we would like to stop using the 7zip.msi and avoid using cmake as a decompressor since 7zip is faster.

c) If you don't want to kill vcpkgTools.xml anymore please add an "arch" entry. That has been missing for ages.

My understanding is that vcpkgTools.xml is primarily for internal use and I am not aware of any need for an architecture field. Can you elaborate?

@Neumann-A
Copy link
Contributor

decompressor since 7zip is faster.

Do you have a fair benchmark for that (e.g. not comparing x64 binaries against x86)? The reason cmake is used is to make it easily cross-platform. This is also now possible with 7zip since it provides binaries for !windows.

I also don't see a reason to give full 7zip in the xml 7zr should be more than enough.

primarily for internal use

What do you mean by internal use? If I don't have a valid cmake version or force exact abi version vcpkg tends to use the CMake from the xml. How is this considered as internal?

I am not aware of any need for an architecture field

If you build on arm you are basically screwed unless you have a correct version installed. Also using the x86 version of cmake compared to the x64 is annoying since it is only using the x86 since that should work on arm64. As such providing an arch field allows to actually provide the correct binaries for a certain host/build platform.

@BillyONeal
Copy link
Member

BillyONeal commented Aug 27, 2024

a) Why add extra stuff to vcpkgTools.xml I thought it should die?
c) If you don't want to kill vcpkgTools.xml anymore please add an "arch" entry. That has been missing for ages.

I agree that it should die but I don't have strong opinions that that death must be sequenced before this fix. I would feel differently if it were trying to add a whole new feature to this XML, such as trying to make arm64 work better. This is just using the facility as is for now.

b) Why prefer 7zip over cmake -E tar ?

7zip is much faster and produces smaller outputs. In a quick and dirty test compressing an ~800MB file, compression is 1.28 times as fast and decompression is 2.86 times as fast:

PS D:\> dir .\Mariner-2.0-x86_64.iso

    Directory: D:\

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---            3/4/2024  5:37 PM      865574912 Mariner-2.0-x86_64.iso

PS D:\> Measure-Command { .\7zr.exe a example7z.zip .\Mariner-2.0-x86_64.iso }              

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 14
Milliseconds      : 522
Ticks             : 145226482
TotalDays         : 0.000168086206018519
TotalHours        : 0.00403406894444444
TotalMinutes      : 0.242044136666667
TotalSeconds      : 14.5226482
TotalMilliseconds : 14522.6482

PS D:\> Measure-Command { cmake.exe -E tar -czf exampleCmake.zip .\Mariner-2.0-x86_64.iso }

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 18
Milliseconds      : 959
Ticks             : 189598756
TotalDays         : 0.00021944300462963
TotalHours        : 0.00526663211111111
TotalMinutes      : 0.315997926666667
TotalSeconds      : 18.9598756
TotalMilliseconds : 18959.8756

PS D:\> dir *.zip

    Directory: D:\

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---           8/27/2024  9:49 AM      821983320 example7z.zip
-a---           8/27/2024  9:50 AM      833554398 exampleCmake.zip

PS D:\> Measure-Command { .\7zr.exe e -oextract7Z .\example7z.zip }                                    
 
Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 3
Milliseconds      : 431
Ticks             : 34310099
TotalDays         : 3.97107627314815E-05
TotalHours        : 0.000953058305555556
TotalMinutes      : 0.0571834983333333
TotalSeconds      : 3.4310099
TotalMilliseconds : 3431.0099

PS D:\> mkdir extractCMake

    Directory: D:\

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           8/27/2024  9:59 AM                extractCMake

PS D:\> pushd .\extractCMake\
PS D:\extractCMake> Measure-Command {  cmake.exe -E tar -xf ..\example7z.zip } 

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 11
Milliseconds      : 357
Ticks             : 113578045
TotalDays         : 0.000131456070601852
TotalHours        : 0.00315494569444444
TotalMinutes      : 0.189296741666667
TotalSeconds      : 11.3578045
TotalMilliseconds : 11357.8045

PS D:\extractCMake>

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I agree that this file should die but this is borderline OK as it isn't truly introducing more badness we have to clean up later.

@JavierMatosD JavierMatosD merged commit 91b615d into microsoft:master Aug 27, 2024
16 checks passed
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Aug 28, 2024
@anders-wind anders-wind mentioned this pull request Sep 2, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants