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

extract 7z archive part from 7z installers #405

Merged
merged 10 commits into from
Mar 31, 2022

Conversation

Neumann-A
Copy link
Contributor

@BillyONeal This code can separate the 7z archive from the self extracting PortableGit installer (and probably more). The extracted 7z archive can then be passed to cmake -E tar without having to use 7z to extract the installer silently.

src/vcpkg/archives.cpp Outdated Show resolved Hide resolved
Neumann-A and others added 2 commits March 3, 2022 09:25
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

minor changes

src/vcpkg/archives.cpp Outdated Show resolved Hide resolved
src/vcpkg/archives.cpp Outdated Show resolved Hide resolved
src/vcpkg/archives.cpp Outdated Show resolved Hide resolved
src/vcpkg/archives.cpp Outdated Show resolved Hide resolved
src/vcpkg/archives.cpp Outdated Show resolved Hide resolved
Neumann-A and others added 3 commits March 3, 2022 22:11
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Comment on lines +210 to +214
Checks::check_exit(VCPKG_LINE_INFO,
Strings::case_insensitive_ascii_equals(subext, ".7z"),
"Unable to extract 7z archive from Installer %s. Missing '.7z.exe' extension.",
archive);

Copy link
Member

Choose a reason for hiding this comment

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

Similar to my comment in the other PR I don't think we should treat .7z.exe differently than .exe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not? vcpkg treats a number of extension differently here so treating sub-extensions differently is just the same. Since vcpkg is in control of the file name I don't really see an issue with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also the other PR is just for exactly one tool (7z). This PR is for multiple tools having a .7z.exe installer package. (although it currently only applies to PortableGit since it is the only 7z installer package).
So the scope and application is a bit different to the other PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying we should be interpreting all .exe as .7z.exe, because that is true for all the .exes we support passing to this right now. In the other example you checked the whole file name rather than just the extension in order to determine that we had a 7zip installer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok moved everything into the .exe path but I want to keep the requirement that the file is named .7z.exe before the extraction

@BillyONeal
Copy link
Member

Do we want this now that we have determined that we always need 7z anyway?

@Neumann-A
Copy link
Contributor Author

Do we want this now that we have determined that we always need 7z anyway?

as far as I know vcpkg downloads the console version of 7z which cannot extract executables which I learned in microsoft/vcpkg#23659 where i tried to use the console version but had to switch to the full version.

@BillyONeal
Copy link
Member

I decided that it wasn't worth standing in front of this bus :)

@BillyONeal BillyONeal merged commit e67456f into microsoft:main Mar 31, 2022
@Neumann-A Neumann-A deleted the extract_7z_from_7z_exe branch April 1, 2022 05:49
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.

6 participants