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

cmake: Fix building with system minizip-ng #12910

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

ReillyBrogan
Copy link
Contributor

Dolphin currently fails to build when the Linux system building it includes headers/pkgconfigs for minizip-ng built in both minizip-ng mode and legacy compat mode (the minizip API).

This is because minizip-ng is checked for in cmake however the code is not actually compatible against minizip-ng built in non-legacy mode. Until that is rectified Dolphin should just check for a pkgconfig for minizip. If the system has a pkgconfig for minizip with a version >= 4 then the system package is minizip-ng built in compat mode which is exactly what we want.

Dolphin currently fails to build when the Linux system building it includes headers/pkgconfigs for minizip-ng built in both minizip-ng mode and legacy compat mode (the minizip API).

This is because minizip-ng is checked for in cmake however the code is not actually compatible against minizip-ng built in non-legacy mode. Until that is rectified Dolphin should just check for a pkgconfig for minizip. If the system has a pkgconfig for minizip with a version >= 4 then the system package is minizip-ng built in compat mode which is exactly what we want.
@Tilka Tilka merged commit 933da07 into dolphin-emu:master Jul 23, 2024
11 checks passed
@ReillyBrogan ReillyBrogan deleted the fix-minizip-ng branch July 24, 2024 04:06
@Ferdi265
Copy link

This PR breaks the build on Arch Linux. Arch Linux ships minizip 1.3.1 (built from zlib sources) and minizip-ng 4.0.7 (built from zlib-ng/minizip-ng), which means the updated pkg-config check fails because the minizip version is not >= 4.

One solution would potentially be to check for minizip>=4 first, and fall back on minizip-ng>=4 if the first check fails.

@ReillyBrogan
Copy link
Contributor Author

This PR breaks the build on Arch Linux

Arch Linux is building minizip-ng incorrectly. Minizip-ng can be built in legacy mode which is intended to replace the zlib minizip library. When built in this mode the pkgconfig is minizip and the library is libminizip. It can also be built with the newer API at which point the pkgconfig is minizip-ng and the library libminizip-ng.

Bizarrely the minizip-ng package on Arch Linux builds the library in legacy mode, but then uses -DMZ_LIB_SUFFIX="-ng" to force it to have the pkgconfig minizip-ng and the library libminizip-ng. This is completely incorrect, any package that searches for minizip-ng expecting to find a library with the new API will fail to build against the Arch minizip-ng package.

Dolphin is not compatible with the newer minizip-ng API, so it is incorrect for it to search for the minizip-ng pkgconfig file. That will break on any distribution that ships the old zlib minizip library and the newer minizip-ng library built correctly.

I don't know what the solution for Arch Linux is here. Since building minizip-ng under the wrong library name is an Arch-specific issue I would recommend carrying a patch to fix it on your side.

@Ferdi265
Copy link

I will report this issue on the Arch Linux bug tracker for minizip-ng, let's see if it gets fixed.

@Ferdi265
Copy link

Ferdi265 commented Jul 25, 2024

I looked a bit into other distros that ship minizip-ng, and noticed the following:

  • Debian/Ubuntu don't ship minizip-ng
  • Fedora ships minizip as -ng in compat mode, minizip-ng without compat mode, version 3.x
  • Alpine Linux ships minizp from zlib, minizip-ng in compat mode, version 4.x
  • Arch ships minizip from zlib, minizip-ng in compat mode, version 4.x
  • Gentoo ships minizip from zlib, minizip-ng with a a use flag for minizip-ng to set compat mode, warns that replacing system minizip is dangerous, version 4.x
  • NixOS ships minizip from zlib, minizip-ng in compat mode, version 4.x
  • FreeBSD ships minizip from zlib, minizip-ng in compat mode, version 4.x

A large majority of distros is shipping minizip-ng in the same (broken) configuration that Arch Linux is using.

Of these, Gentoo and Fedora are the only ones that build compat mode with the "minizip" pkgconfig name (and Fedora has a version that is too old). All other distros ship compat mode minizip-ng with the "minizip-ng" pkgconfig name.

I didn't look at the whole list in repology, but these are the first few I looked at.

@Ferdi265
Copy link

Arch Linux AUR dolphin-emu-git now ships a downstream patch for this that reverts this commit:

commit c0754c5870ad03d0ff46a6e34cde20fad64270fb
Author: Daniel Peukert <daniel@peukert.cc>
Date:   Fri Jul 26 08:51:52 2024 +0200

    Revert "cmake: Fix building with system minizip-ng"
    
    This reverts commit b06b816d4c29edd314e0611f4305af4c50a4c56f.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ce5d64715f..5b246b193b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -677,7 +677,7 @@ dolphin_find_optional_system_library_pkgconfig(ZSTD libzstd>=1.4.0 zstd::zstd Ex
 dolphin_find_optional_system_library_pkgconfig(ZLIB zlib-ng ZLIB::ZLIB Externals/zlib-ng)
 
 dolphin_find_optional_system_library_pkgconfig(MINIZIP
-  "minizip>=4.0.4" minizip::minizip Externals/minizip-ng
+  "minizip-ng>=4.0.4;minizip>=4.0.4" minizip::minizip Externals/minizip-ng
 )
 
 dolphin_find_optional_system_library(LZO Externals/LZO)

Seeing this patch, wouldn't preferring "minizip>=4.0.4", but falling back on "minizip-ng>=4.0.4" be better? (so, basically just reversing the order of the dependencies)

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -677,7 +677,7 @@ dolphin_find_optional_system_library_pkgconfig(ZSTD libzstd>=1.4.0 zstd::zstd Ex
 dolphin_find_optional_system_library_pkgconfig(ZLIB zlib-ng ZLIB::ZLIB Externals/zlib-ng)
 
 dolphin_find_optional_system_library_pkgconfig(MINIZIP
-  "minizip-ng>=4.0.4;minizip>=4.0.4" minizip::minizip Externals/minizip-ng
+  "minizip>=4.0.4;minizip-ng>=4.0.4" minizip::minizip Externals/minizip-ng
 )
 
 dolphin_find_optional_system_library(LZO Externals/LZO)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants