cmake: add possibility to build zlib from submodule #4778
Conversation
scribam
commented
Jun 18, 2018
•
scribam
commented
- Allow to build RPCS3 when system zlib is not found on Linux systems
- This change should help Windows users who wants to use CMake to build RPCS3
README.md
Outdated
| @@ -109,6 +109,9 @@ This builds RPCS3 with Vulkan support. | |||
| - ```-DUSE_NATIVE_INSTRUCTIONS=ON/OFF``` (default = *ON*) | |||
| This builds rpcs3 with -march=native, which is useful for local builds, but not good for packages. | |||
|
|
|||
| - ```-DUSE_SYSTEM_ZLIB=ON/OFF``` (default = *OFF*) | |||
There was a problem hiding this comment.
I'd like this moved up with the other -DUSE_SYSTEM_ options
.travis.yml
Outdated
| @@ -62,7 +62,7 @@ before_script: | |||
| else | |||
| export CXXFLAGS="$CXXFLAGS -DBRANCH=$TRAVIS_REPO_SLUG/$TRAVIS_BRANCH/#$TRAVIS_PULL_REQUEST"; | |||
| fi; | |||
| - cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DLLVM_DIR=../llvmlibs/lib/cmake/llvm/ -DUSE_NATIVE_INSTRUCTIONS=OFF -G Ninja; | |||
| - cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DLLVM_DIR=../llvmlibs/lib/cmake/llvm/ -DUSE_NATIVE_INSTRUCTIONS=OFF -DUSE_SYSTEM_ZLIB=ON -G Ninja; | |||
There was a problem hiding this comment.
The travis build should be testing defaults. I think it would be better if you inited another submodule rather than using system zlib.
rpcs3/CMakeLists.txt
Outdated
| @@ -403,6 +401,10 @@ endif() | |||
|
|
|||
| target_link_libraries(rpcs3 xxhash yaml-cpp) | |||
|
|
|||
| find_package(ZLIB REQUIRED) | |||
| target_include_directories(rpcs3 PUBLIC ${ZLIB_INCLUDE_DIRS}) | |||
| target_link_libraries(rpcs3 ${ZLIB_LIBRARIES}) | |||
There was a problem hiding this comment.
Unfortunately, when using the submodule, ZLIB::ZLIB does not exist which means it will not work.
README.md
Outdated
| @@ -97,6 +97,9 @@ Build against the shared libpng instead of using the builtin one. libpng 1.6+ hi | |||
| - ```-DUSE_SYSTEM_FFMPEG=ON/OFF``` (default = *OFF*) | |||
| Build against the shared ffmpeg libraries instead of using the builtin patched version. Try this if the builtin version breaks the OpenGL renderer for you. | |||
|
|
|||
| - ```-DUSE_SYSTEM_ZLIB=ON/OFF``` (default = *OFF*) | |||
There was a problem hiding this comment.
Mainly because of Windows users. If it is set to ON by default, Windows users will have to turn it OFF or to use ZLIB_ROOT or to setup correctly zlib on their system (as defined in https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindZLIB.cmake#L62). Using OFF by default guarantees a working out-of-the-box experience.
For Linux users, because zlib is widely available as a system package, it is not a problem to have it ON by default.
An alternative way could be to use different default values based on the operating system but I am not really keen about this method.
There was a problem hiding this comment.
Its wierd ignoring system provided libs when building on linux, something you have been working to move away from recently with recent integration commits where system packaged/maintained versions are used. This feels like it goes against that. All the situations where system libs are not used is usually to work around some issues (ffmpeg, llvm) and I feel that should be the default. I understand the 'out-of-the-box' idea, but if you're building on windows with cmake, you really are not going to be concerned about setting some cmake variables. Is it possible to make the submodule build a fallback?
There was a problem hiding this comment.
I have a new idea. Instead of
if (NOT USE_SYSTEM_ZLIB AND NOT ZLIB_ROOT)
add_subdirectory(3rdparty/zlib EXCLUDE_FROM_ALL)
set(ZLIB_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/zlib" "${CMAKE_CURRENT_BINARY_DIR}/3rdparty/zlib" CACHE INTERNAL "")
set(ZLIB_LIBRARY zlibstatic)
endif()
in CMakeLists.txt, what do you think about the following lines?
find_package(ZLIB QUIET)
if (NOT ZLIB_FOUND)
add_subdirectory(3rdparty/zlib EXCLUDE_FROM_ALL)
set(ZLIB_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/zlib" "${CMAKE_CURRENT_BINARY_DIR}/3rdparty/zlib" CACHE INTERNAL "")
set(ZLIB_LIBRARY zlibstatic)
endif()
If the system package is not found, the submodule is used. This way, the option USE_SYSTEM_ZLIB will not be necessary, Windows users will (mostly) fallback to the submodule while Linux users will keep using the system package.
There was a problem hiding this comment.
That sounds like a good solution to me.
There was a problem hiding this comment.
I pushed the change.
