jeremy-rifkin / libassert

The most over-engineered C++ assertion library
MIT License
547 stars 37 forks source link

Disable the blanket warnings-as-errors for MSVC when distributing the package #107

Open SidneyCogdill opened 3 weeks ago

SidneyCogdill commented 3 weeks ago

Currently /WX is enabled for MSVC:

https://github.com/jeremy-rifkin/libassert/blob/df3a4bdfa5d6f80999ae0b381b298dfd1c277402/CMakeLists.txt#L119

And this causes problems when installing the package from conan package manager:

[cmake] C:\Users\user\.conan2\p\b\libased3e6c110d0ca\b\src\src\tokenizer.cpp(1): error C2220: the following warning is treated as an error
[cmake] C:\Users\user\.conan2\p\b\libased3e6c110d0ca\b\src\src\tokenizer.cpp(1): warning C4819: The file contains a character that cannot be represented in the current code page (936). Save the file in Unicode format to prevent data loss
[cmake] [3/9] Building CXX object CMakeFiles\libassert-lib.dir\src\printing.cpp.obj
[cmake] cl : Command line warning D9025 : overriding '/W3' with '/W4'
[cmake] [4/9] Building CXX object CMakeFiles\libassert-lib.dir\src\paths.cpp.obj
[cmake] cl : Command line warning D9025 : overriding '/W3' with '/W4'
[cmake] [5/9] Building CXX object CMakeFiles\libassert-lib.dir\src\platform.cpp.obj
[cmake] cl : Command line warning D9025 : overriding '/W3' with '/W4'
[cmake] [6/9] Building CXX object CMakeFiles\libassert-lib.dir\src\utils.cpp.obj
[cmake] cl : Command line warning D9025 : overriding '/W3' with '/W4'
[cmake] [7/9] Building CXX object CMakeFiles\libassert-lib.dir\src\assert.cpp.obj
[cmake] cl : Command line warning D9025 : overriding '/W3' with '/W4'
[cmake] [8/9] Building CXX object CMakeFiles\libassert-lib.dir\src\analysis.cpp.obj
[cmake] cl : Command line warning D9025 : overriding '/W3' with '/W4'
[cmake] ninja: build stopped: subcommand failed.

(The offending character that triggers C4819 is probably in the comments and shouldn't interfere with compilation)

Quoting from OpenSSF Hardening Guide:

We recommend developers to additionally use a blanket -Werror to treat all warnings as errors during development. However, -Werror should not be used in this blanket form when distributing source code, as this use of -Werror creates a dependency on specific toolchain vendors and versions. The selective form-Werror= that promotes specific warnings as error in cases that should never occur in the code can be used both during development and when distributing sources.

Note that changing to $<$<AND:$<CONFIG:Debug>,$<CXX_COMPILER_ID:MSVC>>:/W4 /WX> doesn't help in this case as conan will follow CMake's configuration and compile with Debug mode if the user is using Debug build. Ideally there would be a separate CMake preset for development in additional to the default preset for package distribution. Or, an easier way would be to just define a cached variable to indicate that you want to use "development mode" (which is off by default) and enable all the things that aren't suitable for distribution.

SidneyCogdill commented 3 weeks ago

Also note that CMake itself supports setting the warnings-as-errors flag without requiring target_compile_options: https://cmake.org/cmake/help/latest/variable/CMAKE_COMPILE_WARNING_AS_ERROR.html

jeremy-rifkin commented 2 weeks ago

Hi, Thanks for opening this. The OpenSSF recommendation seems fine and I think it would be reasonable to either have a flag to enable -Werror locally or a flag to disable it when packaged. The reason -Werror is used right now is that warnings are usually something I want to take care of. In this case the fix is simple, there are just some non-ascii quotes in the file.

flagarde commented 1 week ago

@jeremy-rifkin -Werror could be set private and put in something like $<$<BOOL:${IS_MAIN_PROJECT}>:-Werror>.