[ML] More CMake tidyups (elastic#2347)

droberts195 · web-flow · commit 9b8b900f98d7 · 2022-07-07T14:53:00.000+01:00
1. We no longer need references to make as we always use cmake
2. Use camelCase for variables in build.gradle
3. Don't require that BOOST_ROOT be an environment variable when
   building natively on Windows
4. Remove the last occurrence of $ML_KEEP_GOING
5. Include all variations of CMake build directory in Git/Docker ignores
6. Docker builds should not hardcode the "Release" config
diff --git a/.dockerignore b/.dockerignore
@@ -85,5 +85,4 @@ boost_test_results.xml
 cmake_install.cmake
 CMakeCache.txt
 CMakeFiles/
-cmake-build-release/
-cmake-build-debug/
+cmake-build-*/
diff --git a/.gitignore b/.gitignore
@@ -17,7 +17,7 @@ build-eclipse/
 nb-configuration.xml
 nbactions.xml
 
-#vscode files
+# vscode files
 .vscode
 .clangd
 .cache
@@ -81,5 +81,4 @@ boost_test_results.xml
 cmake_install.cmake
 CMakeCache.txt
 CMakeFiles/
-cmake-build-release/
-cmake-build-debug/
+cmake-build-*/
diff --git a/build.gradle b/build.gradle
@@ -94,34 +94,31 @@ if (mlDebug.toBoolean()) {
 }
 
 String msystem = "$System.env.MSYSTEM"
-if ((isWindows && msystem == 'MINGW64') || isMacOsX || isLinux) 
-{
-  // Always do the C++ build using bash (Git bash on Windows using MinGW)
-  project.ext.shell = isWindows ? "C:\\Program Files\\Git\\bin\\bash" : "/bin/bash"
-  project.ext.set_env = "source ./set_env.sh"
-  project.ext.shell_flag = "-c"
-  project.ext.clean_cmd = "rm -rf ${cmakeBuildDir} 3rd_party/eigen"
-  // Stripping the binaries is not necessary on windows, execute a no-op command instead
-  project.ext.strip_cmd = isWindows ? true : "dev-tools/strip_binaries.sh"
-} else {
+if (isWindows && msystem != 'MINGW64') {
   project.ext.shell = "cmd.exe"
-  project.ext.set_env = ".\\set_env.bat"
-  project.ext.shell_flag = "/c"
-  project.ext.clean_cmd = "if exist ${cmakeBuildDir}\\ (rmdir /s /q ${cmakeBuildDir}) && if exist 3rd_party\\eigen (rmdir /s /q 3rd_party\\eigen)"
-  // Stripping the binaries is not necessary on windows, execute a no-op command instead
-  project.ext.strip_cmd = "cd ."
+  project.ext.setEnv = ".\\set_env.bat"
+  project.ext.shellFlag = "/c"
+  project.ext.cleanCmd = "if exist ${cmakeBuildDir}\\ (rmdir /s /q ${cmakeBuildDir}) && if exist 3rd_party\\eigen (rmdir /s /q 3rd_party\\eigen)"
+  // Stripping the binaries is not necessary on Windows, execute a no-op command instead
+  project.ext.stripCmd = "cd ."
+} else {
+  // Use the bash shell for the C++ build (Git bash when on Windows using MinGW)
+  project.ext.shell = isWindows ? "C:\\Program Files\\Git\\bin\\bash" : "/bin/bash"
+  project.ext.setEnv = "source ./set_env.sh"
+  project.ext.shellFlag = "-c"
+  project.ext.cleanCmd = "rm -rf ${cmakeBuildDir} 3rd_party/eigen"
+  // Stripping the binaries is not necessary on Windows, execute a no-op command instead
+  project.ext.stripCmd = isWindows ? 'true' : "dev-tools/strip_binaries.sh"
 }
 
-project.ext.make = (isMacOsX || isWindows) ? "gnumake" : "make"
-project.ext.cmake = "cmake"
 project.ext.numCpus = Runtime.runtime.availableProcessors()
 project.ext.makeEnvironment = [ 'CPP_CROSS_COMPILE': cppCrossCompile,
                                 'VERSION_QUALIFIER': versionQualifier,
                                 'SNAPSHOT': (isSnapshot ? 'yes' : 'no'),
                                 'ML_DEBUG': mlDebug ]
 
 configurations.all {
-  // check for updates every build
+  // Check for updates every build
   resolutionStrategy.cacheChangingModulesFor 0, 'seconds'
 }
 
@@ -130,7 +127,7 @@ clean {
     exec {
       environment makeEnvironment
       commandLine shell
-      args shell_flag, "${set_env} && ${clean_cmd}"
+      args shellFlag, "${setEnv} && ${cleanCmd}"
       workingDir "${projectDir}"
     }
   }
@@ -139,29 +136,29 @@ clean {
 task configure(type: Exec) {
   environment makeEnvironment
   commandLine shell
-  args shell_flag, "${set_env} && ${cmake} -B ${cmakeBuildDir} ${cmakeFlags}"
+  args shellFlag, "${setEnv} && cmake -B ${cmakeBuildDir} ${cmakeFlags}"
   workingDir "${projectDir}"
 }
 
 task compile(type: Exec) {
   environment makeEnvironment
   commandLine shell
-  args shell_flag, "${set_env} && ${cmake} --build ${cmakeBuildDir} --config ${cmakeBuildType} -t install -v -j ${numCpus}"
+  args shellFlag, "${setEnv} && cmake --build ${cmakeBuildDir} --config ${cmakeBuildType} -v -j ${numCpus} -t install"
   workingDir "${projectDir}"
   dependsOn 'configure'
 }
 
 task strip(type: Exec) {
   environment makeEnvironment
   commandLine shell
-  args shell_flag, "${set_env} && ${strip_cmd}"
+  args shellFlag, "${setEnv} && ${stripCmd}"
   workingDir "${projectDir}"
   dependsOn 'compile'
 }
 
 task format(type: Exec) {
   commandLine shell
-  args shell_flag, "${set_env} && ${cmake} -P cmake/clang-format.cmake"
+  args shellFlag, "${setEnv} && cmake -P cmake/clang-format.cmake"
   workingDir "${projectDir}"
 }
 
@@ -220,7 +217,7 @@ String artifactGroupPath = project.group.replaceAll("\\.", "/")
 task test(type: Exec) {
   environment makeEnvironment
   commandLine shell
-  args shell_flag, "${set_env} && ${cmake} --build ${cmakeBuildDir} --config ${cmakeBuildType} -v -j ${numCpus} -t test"
+  args shellFlag, "${setEnv} && cmake --build ${cmakeBuildDir} --config ${cmakeBuildType} -v -j ${numCpus} -t test"
   workingDir "${projectDir}"
   dependsOn 'strip'
   description = 'Run C++ tests'
@@ -428,7 +425,8 @@ task buildDependencyReport(type: Exec) {
   outputs.file("${buildDir}/distributions/dependencies-${version}.csv")
   environment makeEnvironment
   commandLine shell
-  args '-c', "source ./set_env.sh && 3rd_party/dependency_report.sh --csv \"${outputs.files.singleFile}\""
+  // TODO: this won't work on Windows outside of Git bash
+  args shellFlag, "${setEnv} && 3rd_party/dependency_report.sh --csv \"${outputs.files.singleFile}\""
   workingDir "${projectDir}"
   description = 'Create a CSV report on 3rd party dependencies we redistribute'
 }
diff --git a/cmake/variables.cmake b/cmake/variables.cmake
@@ -35,17 +35,16 @@ list(APPEND ML_COMPILE_DEFINITIONS BOOST_ALL_DYN_LINK BOOST_MATH_NO_LONG_DOUBLE_
 
 if(NOT DEFINED ENV{CMAKE_INSTALL_PREFIX})
   if (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
-    set(CMAKE_INSTALL_PREFIX ${CPP_PLATFORM_HOME}/controller.app/Contents/)
+    set(CMAKE_INSTALL_PREFIX "${CPP_PLATFORM_HOME}/controller.app/Contents/")
   else()
-    set(CMAKE_INSTALL_PREFIX ${CPP_PLATFORM_HOME})
-    message(STATUS "CMAKE_INSTALL_PREFIX = ${CMAKE_INSTALL_PREFIX}")
+    set(CMAKE_INSTALL_PREFIX "${CPP_PLATFORM_HOME}")
   endif()
 else()
-  set(CMAKE_INSTALL_PREFIX $ENV{CMAKE_INSTALL_PREFIX})
+  set(CMAKE_INSTALL_PREFIX "$ENV{CMAKE_INSTALL_PREFIX}")
 endif()
 
-string(REPLACE "\\" "/" CMAKE_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
-message(STATUS "CMAKE_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX}")
+string(REPLACE "\\" "/" CMAKE_INSTALL_PREFIX "${CMAKE_INSTALL_PREFIX}")
+message(STATUS "CMAKE_INSTALL_PREFIX = ${CMAKE_INSTALL_PREFIX}")
 
 if (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
   if (CMAKE_CROSSCOMPILING)
@@ -65,12 +64,17 @@ if (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
   endif()
 elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
   set(ML_BOOST_COMPILER_VER "gcc${CMAKE_CXX_COMPILER_VERSION_MAJOR}")
-else()
+elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows")
   string(CONCAT BOOST_VCVER "vc" ${VCVER_MAJOR} ${VCVER_MINOR} )
   string(SUBSTRING ${BOOST_VCVER} 0 5 BOOST_VCVER)
   message(STATUS "BOOST_VCVER ${BOOST_VCVER}")
 
   set(ML_BOOST_COMPILER_VER ${BOOST_VCVER})
+  if(NOT BOOST_ROOT AND NOT DEFINED ENV{BOOST_ROOT})
+    set(BOOST_ROOT ${ROOT}/usr/local)
+  endif()
+else()
+  message(FATAL_ERROR "Unsupported platform: ${CMAKE_SYSTEM_NAME}")
 endif()
 message(STATUS "ML_BOOST_COMPILER_VER ${ML_BOOST_COMPILER_VER}")
 
@@ -153,17 +157,19 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
 endif()
 
 # Dictate which flags to use for a "Release" build
-message(STATUS "CMAKE_CXX_FLAGS_RELEASE = ${CMAKE_CXX_FLAGS_RELEASE}")
 if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
-  set(CMAKE_CXX_FLAGS_RELEASE "/O2 /DNDEBUG /DEXCLUDE_TRACE_LOGGING /Qfast_transcendentals /Qvec-report:1")
+  set(CMAKE_CXX_FLAGS_RELEASE "/O2 /D NDEBUG /D EXCLUDE_TRACE_LOGGING /Qfast_transcendentals /Qvec-report:1")
+  set(CMAKE_CXX_FLAGS_DEBUG "/Od /RTC1")
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
   set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG -DEXCLUDE_TRACE_LOGGING -Wdisabled-optimization -D_FORTIFY_SOURCE=2")
+  set(CMAKE_CXX_FLAGS_DEBUG "-g")
 endif()
 
 if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
   set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG -DEXCLUDE_TRACE_LOGGING")
+  set(CMAKE_CXX_FLAGS_DEBUG "-g")
 endif()
 message(STATUS "CMAKE_CXX_FLAGS_RELEASE = ${CMAKE_CXX_FLAGS_RELEASE}")
 message(STATUS "CMAKE_CXX_FLAGS_DEBUG = ${CMAKE_CXX_FLAGS_DEBUG}")
diff --git a/dev-tools/docker/docker_entrypoint.sh b/dev-tools/docker/docker_entrypoint.sh
@@ -31,7 +31,7 @@ cd "$MY_DIR/../.."
 cmake -B cmake-build-docker ${CMAKE_FLAGS}
 
 # Build the code
-cmake --build cmake-build-docker --config Release -j`nproc` -v -t install
+cmake --build cmake-build-docker -v -j`nproc` -t install
 
 # Strip the binaries
 dev-tools/strip_binaries.sh
@@ -62,6 +62,6 @@ if [ "x$1" = "x--test" ] ; then
     # failure is the unit tests, and then the detailed test results can be
     # copied from the image
     echo passed > build/test_status.txt
-    cmake --build cmake-build-docker --config Release -t test -j`nproc` || echo failed > build/test_status.txt
+    cmake --build cmake-build-docker -v -j`nproc` -t test || echo failed > build/test_status.txt
 fi
 
diff --git a/set_env.bat b/set_env.bat
@@ -1,31 +1,30 @@
-@echo off
-REM
-REM Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
-REM or more contributor license agreements. Licensed under the Elastic License
-REM 2.0 and the following additional limitation. Functionality enabled by the
-REM files subject to the Elastic License 2.0 may only be used in production when
-REM invoked by an Elasticsearch process with a license key installed that permits
-REM use of machine learning features. You may not use this file except in
-REM compliance with the Elastic License 2.0 and the foregoing additional
-REM limitation.
-REM
-
-REM Set up a build environment, to ensure repeatable builds
-
-REM Initialize the Visual Studio command prompt environment variables
-call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64
-
-REM Set %CPP_SRC_HOME% to be an absolute path to this script's location, as
-REM different builds will come from different repositories and go to different
-REM staging areas
-SET CPP_SRC_HOME=%~dp0
-
-REM Logical filesystem root
-SET ROOT=%CD:~0,2%
-
-SET PATH=%ROOT%/PROGRA~1/CMake/bin;%CPP_SRC_HOME%/build/distribution/platform/windows-x86_64/bin;%PATH%
-		
-SET BOOST_ROOT=%ROOT%/usr/local
-
-SET INCLUDE=
-SET LIBPATH=
+@echo off
+rem
+rem Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+rem or more contributor license agreements. Licensed under the Elastic License
+rem 2.0 and the following additional limitation. Functionality enabled by the
+rem files subject to the Elastic License 2.0 may only be used in production when
+rem invoked by an Elasticsearch process with a license key installed that permits
+rem use of machine learning features. You may not use this file except in
+rem compliance with the Elastic License 2.0 and the foregoing additional
+rem limitation.
+rem
+
+rem Set up a build environment, to ensure repeatable builds
+
+rem Initialize the Visual Studio command prompt environment variables
+call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64
+
+rem Set %CPP_SRC_HOME% to be an absolute path to this script's location, as
+rem different builds will come from different repositories and go to different
+rem staging areas
+set CPP_SRC_HOME=%~dp0
+
+rem Assume the drive letter where our 3rd party dependencies are installed under
+rem \usr\local is the current drive at the time this script is run
+set ROOT=%CD:~0,2%
+
+set PATH=C:\Program Files\CMake\bin;%CPP_SRC_HOME%\build\distribution\platform\windows-x86_64\bin;%PATH%
+
+set INCLUDE=
+set LIBPATH=
diff --git a/set_env.sh b/set_env.sh
@@ -130,44 +130,28 @@ case $SIMPLE_PLATFORM in
 
 esac
 
-# do not cache in Jenkins
+# Do not cache in Jenkins
 if [ -z "$JOB_NAME" ] ; then
-    # check if CCACHE is available and use it if it's found
+    # Check if CCACHE is available and use it if it's found
     if [ -d "/usr/lib/ccache" ] ; then
-        PATH=/usr/lib/ccache/:$PATH
-    # on centos it should be in /usr/lib64/ccache when installed with yum.
+        PATH=/usr/lib/ccache:$PATH
+    # On CentOS it should be in /usr/lib64/ccache when installed with yum.
     elif [ -d "/usr/lib64/ccache" ] ; then
-        PATH=/usr/lib64/ccache/:$PATH
-    # on Mac it should be /usr/local/lib when installed with brew
+        PATH=/usr/lib64/ccache:$PATH
+    # On macOS it should be /usr/local/lib when installed with brew
     elif [ -d "/usr/local/lib/ccache" ] ; then
-        PATH=/usr/local/lib/ccache/:$PATH
+        PATH=/usr/local/lib/ccache:$PATH
     fi
 fi
 
-
 export PATH
 
-# GNU make
-case $SIMPLE_PLATFORM in
-
-    linux*)
-        MAKE=`which make`
-        ;;
-
-    macos|windows)
-        MAKE=`which gnumake`
-        ;;
-
-esac
-
-export MAKE
-
 # Localisation - don't use any locale specific functionality
 export LC_ALL=C
 
 # Unset environment variables that affect the choice of compiler/linker, or
-# where the compiler/linker look for dependencies - we only want what's in our
-# Makefiles
+# where the compiler/linker look for dependencies - we only want to use what's
+# explicitly defined in our build files
 unset CPP
 unset CC
 unset CXX
@@ -186,9 +170,3 @@ else
     unset LIBRARY_PATH
 fi
 
-# Tell the build to keep going under Jenkins so that we get as many errors as
-# possible from an automated build (Jenkins sets $JOB_NAME)
-if [ -n "$JOB_NAME" ] ; then
-    export ML_KEEP_GOING=1
-fi
-
