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

Disable more compression methods for minizip #602

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Disable more compression methods for minizip #602

merged 2 commits into from
Dec 13, 2023

Conversation

Jan200101
Copy link
Contributor

Doing this because I ran into the following error while compiling with msvc-wine

z:/home/sentry/git/NorthstarLauncher/thirdparty/minizip/mz_strm_zstd.c(17): fatal error C1083: Cannot open include file: 'zstd.h': No such file or directory

What is actually needed from minizip?
This was likely not an issue on windows because of MZ_FETCH_LIBS. Should we turn that off to ensure no extra dependencies get in without our knowledge?

@Jan200101 Jan200101 requested a review from Alystrasz November 19, 2023 00:13
@Jan200101 Jan200101 added the feedback wanted Feedback is wanted whether the changes by this PR are desired label Nov 19, 2023
@Alystrasz
Copy link
Contributor

Alystrasz commented Nov 21, 2023

minizip was introduced in #545, and is only used to uncompress archives (no need for compression methods afaik).

I see that some compression features are already disabled in this repository (https://github.com/R2Northstar/NorthstarLauncher/blob/aeecd7a69be8de3afdca124ab624677bfd5582cf/cmake/Findminizip.cmake#L5C61-L5C61), maybe we wanna disable more indeed.

My guess is that MZ_ZLIB is required, but I have to try compiling without it to check.

EDIT: my dumbass thought this was an issue, not a pull request...
I'll check whether mod downloading still works on this.

@Jan200101
Copy link
Contributor Author

Jan200101 commented Nov 22, 2023

My guess is that MZ_ZLIB is required, but I have to try compiling without it to check.

ZLIB would be required for DEFLATE compressed zips (which is the most common format) but others are also possible.

Considering mod verification is a manual process, should there be some kind of requirement what compression it needs to use?

@Jan200101
Copy link
Contributor Author

Updated to enable ZLIB for DEFLATE

cmake/Findminizip.cmake Show resolved Hide resolved
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

I confirm disabling those methods does not have any negative impact on mod downloading (tested with #595 branch).


Given we agreed on Discord to disable as many flags as possible, I tried disabling more, while ensuring mod downloading still works; I ended up with the following configuration:

if(NOT minizip_FOUND)
	check_init_submodule(${PROJECT_SOURCE_DIR}/thirdparty/minizip)

	set(MZ_ZLIB ON CACHE BOOL "Disable ZLIB compression")
	set(MZ_BZIP2 OFF CACHE BOOL "Disable BZIP2 compression")
	set(MZ_LZMA OFF CACHE BOOL "Disable LZMA & XZ compression")
	set(MZ_PKCRYPT OFF CACHE BOOL "Disable PKWARE traditional encryption")
	set(MZ_WZAES OFF CACHE BOOL "Disable WinZIP AES encryption")
	set(MZ_ZSTD OFF CACHE BOOL "Disable ZSTD compression")
	set(MZ_SIGNING OFF CACHE BOOL "Disable zip signing support")

	add_subdirectory(${PROJECT_SOURCE_DIR}/thirdparty/minizip minizip)
	set(minizip_FOUND 1 PARENT_SCOPE)
endif()

@GeckoEidechse GeckoEidechse added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed feedback wanted Feedback is wanted whether the changes by this PR are desired labels Dec 13, 2023
@GeckoEidechse
Copy link
Member

Given that minizip is only used for the not officially release mod-auto-download, I think we can be a lot more lax with review requirements on this PR ^^

@Jan200101
Copy link
Contributor Author

commited, rebased and merged (because why not)
Can be merged and released when ready

Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Confirmed working as described in #602 (review).

@GeckoEidechse GeckoEidechse added the READY TO MERGE This mergeable right now label Dec 13, 2023
@GeckoEidechse
Copy link
Member

Merging based on reviews

@GeckoEidechse GeckoEidechse merged commit 8a41071 into R2Northstar:main Dec 13, 2023
2 checks passed
@Jan200101 Jan200101 deleted the PR/lesscompress branch June 24, 2024 12:40
@Jan200101 Jan200101 removed almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge READY TO MERGE This mergeable right now labels Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants