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

replace zlib with zlib-ng #10888

Merged
merged 3 commits into from Jul 26, 2022
Merged

replace zlib with zlib-ng #10888

merged 3 commits into from Jul 26, 2022

Conversation

shuffle2
Copy link
Contributor

similar crc32 perf to #10882 , but this fork can be made a submodule and has more optimizations throughout other parts of their codebase (notably for zlib compress as well as decompress).

@AdmiralCurtiss
Copy link
Contributor

Did you mean to include the last two commits here?

@shuffle2
Copy link
Contributor Author

yes

@AdmiralCurtiss
Copy link
Contributor

How exactly is USE_SHARED_LIBPNG related to zlib, then...?

@shuffle2
Copy link
Contributor Author

it's needed for me to build with cmake

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jul 24, 2022

Okay, gave this a look-over. Seems fine to me except for one thing: As far as I can tell, the zlib-ng CMakeLists.txt only builds the static zlib (which is probably the desired outcome) because other external projects we use (cubeb, glslang, pugixml, maybe something else) sets BUILD_SHARED_LIBS to OFF by default. If you didn't include those projects, BUILD_SHARED_LIBS would be undefined, and it would build both a static and shared lib instead.

We should probably make that explicit. I suggest adding a option(BUILD_SHARED_LIBS "Build shared library" OFF) in our zlib-ng CMakeLists.txt wrapper.

@AdmiralCurtiss
Copy link
Contributor

(To be clear, I think this is poor behavior of zlib-ng -- an undefined BUILD_SHARED_LIBS should be treated the same as OFF based on what I can find in the CMake documentation. That its CMakeLists.txt treats it differently is questionable, but we have to deal with it if we don't want a completely custom CMakeLists.txt for it.)

@AdmiralCurtiss
Copy link
Contributor

Oh and also, do you want to set the shallow = true for the submodule?

@shuffle2
Copy link
Contributor Author

shuffle2 commented Jul 24, 2022

BUILD_SHARED_LIBS stuff

yea sounds good

Oh and also, do you want to set the shallow = true for the submodule?

sure. tbh i haven't checked if it actually ...works? I always seem to have to put it manually, git does not insert it itself (even when submodule is created with depth=1 or something)

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jul 24, 2022

Hm, apparently shallow = true only works if you initially clone with git clone --recurse-submodules. git submodule update does fetch everything regardless unless you explicitly pass --depth=1. That's silly...

@shuffle2
Copy link
Contributor Author

i also dropped the libpng cmake change

since the benefits are so high, don't link with shared zlib
not required but it seems nice
@shuffle2
Copy link
Contributor Author

(only updated Externals/licenses.md for zlib-ng)

@AdmiralCurtiss AdmiralCurtiss merged commit e4ff497 into dolphin-emu:master Jul 26, 2022
11 checks passed
@theofficialgman
Copy link

since I can't create a bug report I will report this directly here

@shuffle2 since the merge of this PR. building dolphin installs zlibng over the system zlib, breaking multiple other applications that depend on the functional system zlib

@AdmiralCurtiss
Copy link
Contributor

Can you clarify? What system/distribution/version and how exactly are you building dolphin? The default settings should only build a static zlib-ng that gets linked directly into the dolphin binary, which shouldn't affect any other program.

@theofficialgman
Copy link

theofficialgman commented Jul 27, 2022

ubuntu bionic 18.04. using system libs whenever possible which is why only the required submodules are pulled.
This was reported to me by a user and I needed to replicate myself which is why I took a while to respond.

git clone https://github.com/dolphin-emu/dolphin
cd dolphin
git submodule update --init Externals/mGBA
git submodule update --init Externals/spirv_cross
git submodule update --init Externals/zlib-ng
git submodule update --init Externals/libspng 
mkdir -p build
cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS=-mcpu=native -DCMAKE_C_FLAGS=-mcpu=native -DCMAKE_C_FLAGS_INIT="-static" -DCMAKE_C_COMPILER=gcc-11 -DCMAKE_CXX_COMPILER=g++-11
make -j$(nproc)
sudo make install

I see that during the make install, libz.a is copied to /usr/local/lib/libz.a which is not static at all

and you can see here in the CMakeCache.txt that it isn't static either

//Value Computed by CMake
zlib_BINARY_DIR:STATIC=/home/garrett/dolphin/build/Externals/zlib-ng/zlib-ng

//Value Computed by CMake
zlib_IS_TOP_LEVEL:STATIC=OFF

you can see the file output before installing dolphin:

garrett@garrett-usb:~$ file /usr/local/lib/libz.a
/usr/local/lib/libz.a: cannot open `/usr/local/lib/libz.a' (No such file or directory)

and after installing dolphin

garrett@garrett-usb:~$ file /usr/local/lib/libz.a
/usr/local/lib/libz.a: thin archive with 197 symbol entries
-- Installing: /usr/local/lib/libz.a
-- Installing: /usr/local/include/zlib.h
-- Installing: /usr/local/include/zlib_name_mangling.h
-- Installing: /usr/local/include/zconf.h
-- Installing: /usr/local/lib/pkgconfig/zlib.pc

@shuffle2
Copy link
Contributor Author

that's unfortunate @theofficialgman :(

fwiw we are just calling into zlib-ng's cmake file: https://github.com/zlib-ng/zlib-ng/blob/develop/CMakeLists.txt , and only expect it to be building a static library, which should not get installed along with dolphin.

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