refactor: consolidate flags and simplify cmake (VowpalWabbit#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

@@ -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}

.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

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 $<$<CONFIG:DEBUG>:${linux_debug_config}> $<$<CONFIG:RELEASE>:${linux_release_config}> $<$<CONFIG:RelWithDebInfo>:${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 $<CONFIG>)
-    # The VS Generator builds fall over in test when STDOUT/ERR contains strings of the form
-    # "(<other-stuff, snipped>)?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="<args here>"

Makefile

@@ -6,53 +6,55 @@ 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
 	cd build; cmake .. -DSTATIC_LINK_VW=On
 
 # 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

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 $<$<CONFIG:Debug>:${LINUX_DEBUG_CONFIG}> $<$<CONFIG:Release>:${LINUX_RELEASE_CONFIG}> $<$<CONFIG:RelWithDebInfo>:${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)

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)

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)

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()