madler / zlib

A massively spiffy yet delicately unobtrusive compression library.
http://zlib.net/
Other
5.71k stars 2.45k forks source link

Add CMake option to link MSVC runtime statically #878

Closed tbeu closed 9 months ago

tbeu commented 11 months ago

This fixes #849 (and the broken CRT static linkage) in a proper way. Call

cmake.exe" -DSKIP_INSTALL_FILES=ON -DZLIB_STATIC_LINK_CRT=ON -DCMAKE_STATIC_LINKER_FLAGS=/SUBSYSTEM:CONSOLE,6.01 -G"NMake Makefiles" -DCMAKE_BUILD_TYPE=RELEASE

to link the C runtime library statically. Default (still) is dynamically.

This should supersed #535.

madler commented 9 months ago

Incorporated. Thanks.

nmoinvaz commented 9 months ago

This seems rather strange. Why can't you just pass -D CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebug on the command line?

Neustradamus commented 9 months ago

Merged commit:

nmoinvaz commented 9 months ago

According to the CMake docs this is wrong and the variable should be set before project(). But honestly it should be better passed on the command line instead of the CMakeLists.txt because it can apply to your entire build not to just a single project, of which zlib may be one of many.

madler commented 9 months ago

@nmoinvaz Are you recommending that I undo this commit?

Neustradamus commented 9 months ago

Or adding a new change to be perfect?

There is this PR too:

nmoinvaz commented 9 months ago

I would undo this commit yes, the docs clearly state it must be set before project().

image

Even if you were to move it, CMAKE_GENERATOR would likely be invalid before project() so it would need some rework.

I also oppose #535, as I don't think you need a specialized option to statically link with runtime library. Instead it is easy enough to do:

Windows:

cmake -S . -B build -D CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded

Linux/Apple:

export LDFLAGS=-static
cmake -S . -B build 

Or even:

cmake -S . -B build -D CMAKE_EXE_LINKER_FLAGS=-static -D CMAKE_SHARED_LINKER_FLAGS=-static

Any user can also set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded") in their parent CMake project if they wanted. Also this PR change would discard any CMAKE_MSVC_RUNTIME_LIBRARY setting that might be set by a parent project because it sets it in the else() condition always.

CMake command instructions, like the ones mentioned, are better suited for the GitHub wiki page for this project. I have edited it here. Feel free to change it.

madler commented 9 months ago

Thanks! I didn't even know there was a wiki.

madler commented 9 months ago

Sorry @tbeu. Reverted.

nmoinvaz commented 9 months ago

I may have read that documentation wrong, it seems only CMP0091 needs to be set before project().

I think if you keep this PR, at the very least it should be changed to if(POLICY CMP0091 AND NOT DEFINED CMAKE_MSVC_RUNTIME_LIBRARY) . I fear it will mess up parent projects that already set this a different way.