From 9a963722447e95d323cbedfbeb9f9eaa57699039 Mon Sep 17 00:00:00 2001 From: Jack Gerrits Date: Thu, 14 Apr 2022 08:25:18 -0700 Subject: [PATCH] refactor: consolidate flags and simplify cmake (#3871) * refactor: consolidate flags and simplify cmake * use old version of generator expression * bring back gcov as an option and name VW_GCOV * flags * disallow VW_GCOV on windows --- .scripts/linux/build-static-java.sh | 2 +- .scripts/linux/build-with-coverage.sh | 2 +- CMakeLists.txt | 100 ++---------------- Makefile | 28 ++--- cmake/VWFlags.cmake | 46 ++++++++ cmake/VowpalWabbitUtils.cmake | 26 +++++ vowpalwabbit/CMakeLists.txt | 23 +--- vowpalwabbit/active_interactor/CMakeLists.txt | 4 - 8 files changed, 100 insertions(+), 131 deletions(-) create mode 100644 cmake/VWFlags.cmake diff --git a/.scripts/linux/build-static-java.sh b/.scripts/linux/build-static-java.sh index 85b6d3a0dbf..eae870a833c 100755 --- a/.scripts/linux/build-static-java.sh +++ b/.scripts/linux/build-static-java.sh @@ -13,5 +13,5 @@ cd build cmake .. -DCMAKE_BUILD_TYPE=Release -DWARNINGS=Off -DBUILD_JAVA=On -DBUILD_DOCS=Off -DBUILD_FLATBUFFERS=On\ -DBUILD_PYTHON=Off -DSTATIC_LINK_VW_JAVA=On -DCMAKE_C_COMPILER=/usr/local/bin/gcc -DCMAKE_CXX_COMPILER=/usr/local/bin/g++ \ -DBUILD_TESTING=Off -NUM_PROCESSORS=$(cat nprocs.txt) +NUM_PROCESSORS=$(nproc) make all -j ${NUM_PROCESSORS} diff --git a/.scripts/linux/build-with-coverage.sh b/.scripts/linux/build-with-coverage.sh index 4f2847d1ea1..74fda4db810 100755 --- a/.scripts/linux/build-with-coverage.sh +++ b/.scripts/linux/build-with-coverage.sh @@ -6,5 +6,5 @@ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" REPO_DIR=$SCRIPT_DIR/../../ cd $REPO_DIR -cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DGCOV=ON -DWARNINGS=OFF -DBUILD_JAVA=Off -DBUILD_PYTHON=Off -DBUILD_TESTING=On -DBUILD_FLATBUFFERS=On +cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DVW_GCOV=ON -DWARNINGS=OFF -DBUILD_JAVA=Off -DBUILD_PYTHON=Off -DBUILD_TESTING=On -DBUILD_FLATBUFFERS=On cmake --build build diff --git a/CMakeLists.txt b/CMakeLists.txt index 02299be1493..d2e85c1158a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -26,12 +26,9 @@ list(GET VW_VERSION_LIST 2 VW_VERSION_PATCH) # Set this to on so that tooling can make use of the outputted compile commands (such as clang-tidy) set(CMAKE_EXPORT_COMPILE_COMMANDS On) -set(DEFAULT_BUILD_TYPE "Release") -if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES) - message(STATUS "Setting build type to '${DEFAULT_BUILD_TYPE}' as none was specified.") - set(CMAKE_BUILD_TYPE "${DEFAULT_BUILD_TYPE}" CACHE STRING "Choose the type of build." FORCE) - # Set the possible values of build type for cmake-gui - set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release" "MinSizeRel" "RelWithDebInfo") +if (NOT CMAKE_BUILD_TYPE AND NOT GENERATOR_IS_MULTI_CONFIG) + message(STATUS "No build type selected, defaulting to Release") + set(CMAKE_BUILD_TYPE "Release") endif() if(WIN32) @@ -85,14 +82,7 @@ ELSE(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/.git) SET(vw_GIT_COMMIT "") ENDIF(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/.git) -include(ProcessorCount) -ProcessorCount(NumProcessors) -message(STATUS "Number of processors: ${NumProcessors}") -file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/nprocs.txt ${NumProcessors}) - -option(PROFILE "Turn on flags required for profiling" OFF) -option(VALGRIND_PROFILE "Turn on flags required for profiling with valgrind" OFF) -option(GCOV "Turn on flags required for gcov" OFF) +option(VW_GCOV "Turn on flags required for gcov" OFF) option(WARNINGS "Turn on warning flags. ON by default." ON) option(WARNING_AS_ERROR "Turn on warning as error. OFF by default." OFF) option(STATIC_LINK_VW "Link VW executable statically. Off by default." OFF) @@ -151,7 +141,9 @@ if(VW_INSTALL AND NOT RAPIDJSON_SYS_DEP) message(WARNING "Installing with a vendored version of rapidjson is not recommended. Use RAPIDJSON_SYS_DEP to use a system dependency or specify VW_INSTALL=OFF to silence this warning.") endif() -string(TOUPPER "${CMAKE_BUILD_TYPE}" CONFIG) +if(VW_GCOV AND (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")) + message(FATAL_ERROR "VW_GCOV requires Debug build type.") +endif() # When using Ninja, or CCache GCC and Clang do not output colors as it sees it as a non-terminal output. # Use this option to force color output in these modes. @@ -164,56 +156,10 @@ if(FORCE_COLORED_OUTPUT) endif() endif() -if(WIN32 AND (PROFILE OR VALGRIND_PROFILE OR GCOV OR STATIC_LINK_VW OR BUILD_JAVA OR LTO)) +if(WIN32 AND (STATIC_LINK_VW OR BUILD_JAVA OR VW_GOV)) message(FATAL_ERROR "Unsupported option enabled on Windows build") endif() -# This was the previous default but is only valid on x86_64 systems. -set(linux_x86_64_opt_flags "") -if("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "x86_64") - set(linux_x86_64_opt_flags -msse2 -mfpmath=sse) -endif() - -# Add -ffast-math for speed, remove for testability. -# no-stack-check is added to mitigate stack alignment issue on Catalina where there is a bug with aligning stack-check instructions, and stack-check became default option -set(linux_release_config -O3 -fno-strict-aliasing ${linux_x86_64_opt_flags} -fno-stack-check) -set(linux_debug_config -g -O0 -fno-stack-check) - -if((NOT PROFILE) AND (NOT GCOV)) - set(linux_release_config ${linux_release_config} -fomit-frame-pointer) -endif() - -#Use default visiblity on UNIX otherwise a lot of the C++ symbols end up for exported and interpose'able -set(linux_flags -fvisibility=hidden $<$:${linux_debug_config}> $<$:${linux_release_config}> $<$:${linux_release_config}>) - -if(LTO) - if(NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") - message(FATAL_ERROR "LTO requires Clang") - endif() - if("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS "8.0.0") - message(FATAL_ERROR "LTO requires Clang 8.0 (llvm 3.9) or later") - endif() - If("${CONFIG}" STREQUAL "DEBUG") - message(FATAL_ERROR "LTO only works with Release builds") - endif() - set(linux_flags ${linux_flags} -flto=thin) -endif() - -# for profiling -- note that it needs to be gcc -if(PROFILE) - set(linux_flags ${linux_flags} -fno-strict-aliasing -pg) -endif() - -# for valgrind profiling: run 'valgrind --tool=callgrind PROGRAM' then 'callgrind_annotate --tree=both --inclusive=yes' -if(VALGRIND_PROFILE) - set(linux_flags ${linux_flags} -g -fomit-frame-pointer -fno-strict-aliasing) -endif() - -# gcov configuration -if(GCOV) - set(linux_flags ${linux_flags} -g -O0 -fprofile-arcs -ftest-coverage -fno-strict-aliasing -pg) -endif() - # Use folders in VS solution set_property(GLOBAL PROPERTY USE_FOLDERS ON) @@ -303,36 +249,6 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING) find_package(Boost REQUIRED COMPONENTS unit_test_framework) add_subdirectory(test) - set(vw_c_api_unit_test_target "") - if(BUILD_EXPERIMENTAL_BINDING) - set(vw_c_api_unit_test_target "vw_c_api_unit_test") - endif() - - set(vw_cs_unittest_targets "") - if (vw_BUILD_NET_FRAMEWORK) - set(vw_cs_unittest_targets - cs_unittest - cs_unittest_nofriend - ) - endif() - - # The Visual Studio expression generator - set(cmake_test_configuration "") - set(cmake_test_verbosity "--verbose") - if (${CMAKE_GENERATOR} MATCHES "^Visual Studio") - set(cmake_test_configuration -C $) - # The VS Generator builds fall over in test when STDOUT/ERR contains strings of the form - # "()?error[^:]*:.*", which our native unit tests output as part of a - # successful run. - # - # This is caused by MSBuild being "helpful": - # https://stackoverflow.com/questions/48117302/does-the-msbuild-exec-task-search-stdout-for-the-string-error - # - # The CMake team has discussed this issue, but are using an MSBuild mechanism that does not support - # overriding this behaviour. The fix is to disable test output on Visual Studio generator builds. - set(cmake_test_verbosity "") - endif() - # Don't offer these make dependent targets on Windows if(NOT WIN32) # make bigtests BIG_TEST_ARGS="" diff --git a/Makefile b/Makefile index 92de1511545..f29528339fd 100644 --- a/Makefile +++ b/Makefile @@ -6,17 +6,19 @@ ensure_cmake: mkdir -p build cd build; cmake .. +# for profiling -- note that it needs to be gcc ensure_cmake_gcov: mkdir -p build - cd build; cmake .. -DGCOV=On + cd build; cmake .. -DCMAKE_BUILD_TYPE=Debug -DVW_GCOV=On ensure_cmake_profile: mkdir -p build - cd build; cmake .. -DPROFILE=On + cd build; cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS="-pg" +# for valgrind profiling: run 'valgrind --tool=callgrind PROGRAM' then 'callgrind_annotate --tree=both --inclusive=yes' ensure_cmake_valgrind: mkdir -p build - cd build; cmake .. -DVALGRIND_PROFILE=On -DCMAKE_BUILD_TYPE=Debug + cd build; cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fomit-frame-pointer -fno-strict-aliasing" ensure_cmake_static: mkdir -p build @@ -24,35 +26,35 @@ ensure_cmake_static: # Build targets spanning_tree_build: - cd build; make -j$(shell cat ./build/nprocs.txt) vw_spanning_tree_bin + cd build; make -j$(nproc) vw_spanning_tree_bin vw_build: - cd build; make -j$(shell cat ./build/nprocs.txt) vw-bin + cd build; make -j$(nproc) vw-bin active_interactor_build: - cd build; make -j$(shell cat ./build/nprocs.txt) active_interactor + cd build; make -j$(nproc) active_interactor library_example_build: - cd build; make -j$(shell cat ./build/nprocs.txt) ezexample_predict ezexample_predict_threaded ezexample_train library_example test_search search_generate recommend gd_mf_weights + cd build; make -j$(nproc) ezexample_predict ezexample_predict_threaded ezexample_train library_example test_search search_generate recommend gd_mf_weights python_build: - cd build; make -j$(shell cat ./build/nprocs.txt) pylibvw + cd build; make -j$(nproc) pylibvw java_build: - cd build; make -j$(shell cat ./build/nprocs.txt) vw_jni + cd build; make -j$(nproc) vw_jni test_build: @echo "vw running test-suite..." - cd build; make -j$(shell cat ./build/nprocs.txt) all; make test + cd build; make -j$(nproc) all; make test unit_test_build: - cd build/test/unit_test; make -j$(shell cat ./build/nprocs.txt) vw-unit-test.out test + cd build/test/unit_test; make -j$(nproc) vw-unit-test.out test bigtests_build: - cd build; make -j$(shell cat ./build/nprocs.txt) bigtests BIG_TEST_ARGS="$(MAKEFLAGS)" + cd build; make -j$(nproc) bigtests BIG_TEST_ARGS="$(MAKEFLAGS)" install_build: - cd build; make -j$(shell cat ./build/nprocs.txt) install + cd build; make -j$(nproc) install doc_build: cd build; make doc diff --git a/cmake/VWFlags.cmake b/cmake/VWFlags.cmake new file mode 100644 index 00000000000..f78e276be94 --- /dev/null +++ b/cmake/VWFlags.cmake @@ -0,0 +1,46 @@ +set(VW_LINUX_FLAGS "") +if(LTO) + if(NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") + message(FATAL_ERROR "LTO requires Clang") + endif() + if("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS "8.0.0") + message(FATAL_ERROR "LTO requires Clang 8.0 (llvm 3.9) or later") + endif() + If("${CONFIG}" STREQUAL "DEBUG") + message(FATAL_ERROR "LTO only works with Release builds") + endif() + set(VW_LINUX_FLAGS ${VW_LINUX_FLAGS} -flto=thin) +endif() + +if("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "x86_64") + set(LINUX_X86_64_OPT_FLAGS -msse2 -mfpmath=sse) +endif() + +# Add -ffast-math for speed, remove for testability. +# no-stack-check is added to mitigate stack alignment issue on Catalina where there is a bug with aligning stack-check instructions, and stack-check became default option +set(LINUX_RELEASE_CONFIG -fno-strict-aliasing ${LINUX_X86_64_OPT_FLAGS} -fno-stack-check -fomit-frame-pointer) +set(LINUX_DEBUG_CONFIG -fno-stack-check) + +#Use default visiblity on UNIX otherwise a lot of the C++ symbols end up for exported and interpose'able +set(VW_LINUX_FLAGS -fvisibility=hidden $<$:${LINUX_DEBUG_CONFIG}> $<$:${LINUX_RELEASE_CONFIG}> $<$:${LINUX_RELEASE_CONFIG}>) +set(VW_WIN_FLAGS /MP /Zc:__cplusplus) + +# Turn on warnings +set(WARNING_OPTIONS "") +if(WARNINGS) + if(WIN32) + set(WARNING_OPTIONS /W4) + else() + set(WARNING_OPTIONS -Wall -Wextra -Wpedantic) + endif() +endif(WARNINGS) + +# Turn on warnings as errors +set(WARNING_AS_ERROR_OPTIONS "") +if(WARNING_AS_ERROR) + if(WIN32) + set(WARNING_AS_ERROR_OPTIONS /WX) + else() + set(WARNING_AS_ERROR_OPTIONS -Werror) + endif() +endif(WARNING_AS_ERROR) \ No newline at end of file diff --git a/cmake/VowpalWabbitUtils.cmake b/cmake/VowpalWabbitUtils.cmake index be02e606c36..9df522f11df 100644 --- a/cmake/VowpalWabbitUtils.cmake +++ b/cmake/VowpalWabbitUtils.cmake @@ -1,5 +1,6 @@ include(CMakeParseArguments) include(GNUInstallDirs) +include(VWFlags) include(DetectCXXStandard) set(VW_CXX_STANDARD 11) if(USE_LATEST_STD) @@ -148,6 +149,19 @@ function(vw_add_library) set_property(TARGET ${FULL_LIB_NAME} PROPERTY CXX_STANDARD_REQUIRED ON) set_property(TARGET ${FULL_LIB_NAME} PROPERTY CMAKE_CXX_EXTENSIONS OFF) set_property(TARGET ${FULL_LIB_NAME} PROPERTY POSITION_INDEPENDENT_CODE ON) + target_compile_options(${FULL_LIB_NAME} PRIVATE ${WARNING_OPTIONS} ${WARNING_AS_ERROR_OPTIONS}) + target_compile_definitions(${FULL_LIB_NAME} PRIVATE _FILE_OFFSET_BITS=64) + + if (WIN32) + target_compile_options(${FULL_LIB_NAME} PRIVATE ${VW_WIN_FLAGS}) + else() + target_compile_options(${FULL_LIB_NAME} PRIVATE ${VW_LINUX_FLAGS}) + endif() + + if(VW_GCOV) + target_compile_options(${FULL_LIB_NAME} PRIVATE -fprofile-arcs -ftest-coverage) + target_link_libraries(${FULL_LIB_NAME} PRIVATE gcov) + endif() endif() if(VW_INSTALL AND VW_LIB_ENABLE_INSTALL) @@ -194,7 +208,18 @@ function(vw_add_executable) set_property(TARGET ${FULL_BIN_NAME} PROPERTY CXX_STANDARD ${VW_CXX_STANDARD}) set_property(TARGET ${FULL_BIN_NAME} PROPERTY CXX_STANDARD_REQUIRED ON) set_property(TARGET ${FULL_BIN_NAME} PROPERTY CMAKE_CXX_EXTENSIONS OFF) + target_compile_options(${FULL_BIN_NAME} PRIVATE ${WARNING_OPTIONS} ${WARNING_AS_ERROR_OPTIONS}) + target_compile_definitions(${FULL_BIN_NAME} PRIVATE _FILE_OFFSET_BITS=64) + if (WIN32) + target_compile_options(${FULL_BIN_NAME} PRIVATE ${VW_WIN_FLAGS}) + else() + target_compile_options(${FULL_BIN_NAME} PRIVATE ${VW_LINUX_FLAGS}) + endif() + if(VW_GCOV) + target_compile_options(${FULL_BIN_NAME} PRIVATE -fprofile-arcs -ftest-coverage) + target_link_libraries(${FULL_BIN_NAME} PRIVATE gcov) + endif() if(VW_INSTALL AND VW_EXE_ENABLE_INSTALL) install( @@ -238,6 +263,7 @@ function(vw_add_test_executable) gmock ) target_compile_definitions(${FULL_TEST_NAME} PRIVATE ${VW_TEST_COMPILE_DEFS}) + target_compile_options(${FULL_TEST_NAME} PRIVATE ${WARNING_OPTIONS} ${WARNING_AS_ERROR_OPTIONS}) set_property(TARGET ${FULL_TEST_NAME} PROPERTY CXX_STANDARD ${VW_CXX_STANDARD}) set_property(TARGET ${FULL_TEST_NAME} PROPERTY CXX_STANDARD_REQUIRED ON) set_property(TARGET ${FULL_TEST_NAME} PROPERTY CMAKE_CXX_EXTENSIONS OFF) diff --git a/vowpalwabbit/CMakeLists.txt b/vowpalwabbit/CMakeLists.txt index 3d8392a9a6d..38bcefd712b 100644 --- a/vowpalwabbit/CMakeLists.txt +++ b/vowpalwabbit/CMakeLists.txt @@ -424,28 +424,11 @@ endif() # TODO code analysis if(WIN32) target_compile_definitions(vw PUBLIC __SSE2__) - target_compile_options(vw PUBLIC /MP /Zc:__cplusplus) + target_compile_options(vw PUBLIC ${VW_WIN_FLAGS}) else() - target_compile_options(vw PUBLIC ${linux_flags}) + target_compile_options(vw PUBLIC ${VW_LINUX_FLAGS}) endif() - -# Turn on warnings -if(WARNINGS) - if(MSVC) - target_compile_options(vw PRIVATE /W4) - else(MSVC) - target_compile_options(vw PRIVATE -Wall -Wextra -Wpedantic) - endif(MSVC) -endif(WARNINGS) - -# Turn on warnings as errors -if(WARNING_AS_ERROR) - if(MSVC) - target_compile_options(vw PRIVATE /WX) - else(MSVC) - target_compile_options(vw PRIVATE -Wall -Wextra -Wpedantic -Werror) - endif(MSVC) -endif(WARNING_AS_ERROR) +target_compile_options(vw PRIVATE ${WARNING_OPTIONS} ${WARNING_AS_ERROR_OPTIONS}) if(NOT WIN32) find_file(HELP2MAN_EXECUTABLE help2man HINTS /bin /usr/bin /usr/local/bin) diff --git a/vowpalwabbit/active_interactor/CMakeLists.txt b/vowpalwabbit/active_interactor/CMakeLists.txt index 0b739354512..7607b2395fa 100644 --- a/vowpalwabbit/active_interactor/CMakeLists.txt +++ b/vowpalwabbit/active_interactor/CMakeLists.txt @@ -7,8 +7,4 @@ IF(NOT WIN32) ) target_compile_definitions(vw_active_interactor_bin PUBLIC _FILE_OFFSET_BITS=64) - target_compile_options(vw_active_interactor_bin PUBLIC ${linux_flags}) - if(GCOV) - target_link_libraries(vw_active_interactor_bin PUBLIC gcov --coverage) - endif() endif()