Link against libz and pthread privately (#258)

* Link against libz and pthread privately Fixes #235 * Test compilation of cpp source with CMake * Nice error message * Try private link * Try to get windows working * No make on windows :) * Program name * test in ci... * test in ci... * test in ci... * test in ci... * test in ci... * test in ci... * test in ci... * test in ci... * Strict compilation settings * test in ci... * Fix a more pedantic warning * Fix more pedantic warnings * Give up on pedantic windows * Give up on windows * Use macos-13 only for py3.7 * Remove debug stuff * Use macos-13 only for py3.7
cms-nanoAOD · Aug 21, 2024 · 79832f6 · 79832f6
1 parent 87d0a14
commit 79832f6
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 31 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -31,12 +31,16 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        python-version: ["3.7", "3.11", "3.12"]
-        runs-on: [ubuntu-latest, macos-13, windows-latest]
+        python-version: ["3.11", "3.12"]
+        runs-on: [ubuntu-latest, macos-latest, windows-latest]
 
         include:
-        - python-version: "3.12"
-          runs-on: macos-latest
+        - python-version: "3.7"
+          runs-on: ubuntu-latest
+        - python-version: "3.7"
+          runs-on: macos-13
+        - python-version: "3.7"
+          runs-on: windows-latest
     steps:
     - uses: actions/checkout@v4
       with:

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -1,5 +1,8 @@
 cmake_minimum_required(VERSION 3.11)
 
+if(NOT CORRECTIONLIB_VERSION)
+  set(CORRECTIONLIB_VERSION "0.0.0")  # overriden by setup.py
+endif()
 string(REPLACE "." ";" VERSION_SPLIT ${CORRECTIONLIB_VERSION})
 list(GET VERSION_SPLIT 0 SPLIT_VERSION_MAJOR)
 list(GET VERSION_SPLIT 1 SPLIT_VERSION_MINOR)
@@ -19,16 +22,6 @@ set(THREADS_PREFER_PTHREAD_FLAG ON)
 find_package(Threads)
 find_package(ZLIB)
 
-include(CheckCXXCompilerFlag)
-
-check_cxx_compiler_flag(-Wall has_wall)
-if(has_wall)
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall")
-endif()
-if(MSVC)
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zc:__cplusplus /utf-8")
-endif()
-
 
 configure_file(include/version.h.in ${CMAKE_CURRENT_BINARY_DIR}/include/correctionlib_version.h)
 install(FILES ${CMAKE_CURRENT_BINARY_DIR}/include/correctionlib_version.h DESTINATION ${PKG_INSTALL}/include)
@@ -48,21 +41,32 @@ target_include_directories(correctionlib
     $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/pcg-cpp/include>
   )
 target_compile_features(correctionlib PUBLIC cxx_std_17)
+if(MSVC)
+  target_compile_options(correctionlib PRIVATE /Zc:__cplusplus /utf-8)
+else()
+  target_compile_options(correctionlib PRIVATE -Wall -Wextra -Wpedantic -Werror)
+endif()
 if(ZLIB_FOUND)
-  target_link_libraries(correctionlib ZLIB::ZLIB)
+  target_link_libraries(correctionlib PRIVATE ZLIB::ZLIB)
 endif()
 if(Threads_FOUND AND CMAKE_SYSTEM_NAME STREQUAL "Linux")
-  target_link_libraries(correctionlib Threads::Threads)
+  target_link_libraries(correctionlib PRIVATE Threads::Threads)
 endif()
 install(TARGETS correctionlib
   EXPORT correctionlib-targets
   LIBRARY DESTINATION ${PKG_INSTALL}/lib
+  ARCHIVE DESTINATION ${PKG_INSTALL}/lib
   RUNTIME DESTINATION ${PKG_INSTALL}/lib
   PUBLIC_HEADER DESTINATION ${PKG_INSTALL}/include
   )
 
 
 pybind11_add_module(_core MODULE src/python.cc)
+if(MSVC)
+  target_compile_options(_core PRIVATE /W4 /WX)
+else()
+  target_compile_options(_core PRIVATE -Wall -Wextra -Wpedantic -Werror)
+endif()
 target_link_libraries(_core PRIVATE correctionlib)
 set_target_properties(_core PROPERTIES BUILD_WITH_INSTALL_RPATH ON)
 if (APPLE)

diff --git a/include/version.h.in b/include/version.h.in
@@ -7,6 +7,6 @@ namespace correction {
   constexpr std::string_view correctionlib_version{"@CORRECTIONLIB_VERSION@"};
   constexpr int correctionlib_major_version{@correctionlib_VERSION_MAJOR@};
   constexpr int correctionlib_minor_version{@correctionlib_VERSION_MINOR@};
-};
+}
 
 #endif // CORRECTIONLIB_VERSION_H
diff --git a/src/correction.cc b/src/correction.cc
@@ -791,9 +791,11 @@ std::unique_ptr<CorrectionSet> CorrectionSet::from_file(const std::string& fn) {
   if ( fp == nullptr ) {
     throw std::runtime_error("Failed to open file: " + fn);
   }
-  constexpr unsigned char magicref[4] = {0x1f, 0x8b};
+  constexpr unsigned char magicref[2] = {0x1f, 0x8b};
   unsigned char magic[2];
-  fread(magic, sizeof *magic, 2, fp);
+  if (fread(magic, sizeof *magic, 2, fp) != 2) {
+    throw std::runtime_error("Failed to read file magic: " + fn);
+  }
   rewind(fp);
   char readBuffer[65536];
   rapidjson::ParseResult ok;

diff --git a/src/formula_ast.cc b/src/formula_ast.cc
@@ -23,7 +23,7 @@ namespace {
         AstPtr peg_ast;
         int pos;
         std::string msg;
-        parser_.log = [&](size_t ln, size_t col, const std::string &themsg) {
+        parser_.log = [&](size_t, size_t col, const std::string &themsg) {
           pos = col;
           msg = themsg;
         };
@@ -241,11 +241,12 @@ double FormulaAst::evaluate(const std::vector<Variable::Type>& values, const std
         case UnaryOp::Acosh: return std::acosh(arg);
         case UnaryOp::Asinh: return std::asinh(arg);
         case UnaryOp::Atanh: return std::atanh(arg);
-      }
+        default: std::abort();
+      };
     }
     case NodeType::Binary: {
-      auto left = children_[0].evaluate(values, params);
-      auto right = children_[1].evaluate(values, params);
+      const auto left = children_[0].evaluate(values, params);
+      const auto right = children_[1].evaluate(values, params);
       switch (std::get<BinaryOp>(data_)) {
         case BinaryOp::Equal: return (left == right) ? 1. : 0.;
         case BinaryOp::NotEqual: return (left != right) ? 1. : 0.;
@@ -261,9 +262,9 @@ double FormulaAst::evaluate(const std::vector<Variable::Type>& values, const std
         case BinaryOp::Atan2: return std::atan2(left, right);
         case BinaryOp::Max: return std::max(left, right);
         case BinaryOp::Min: return std::min(left, right);
+        default: std::abort();
       };
     }
-    default:
-      std::abort(); // never reached if the switch/case is exhaustive
+    default: std::abort(); // never reached if the switch/case is exhaustive
   }
 }
diff --git a/tests/test_binding.py b/tests/test_binding.py
@@ -1,14 +1,16 @@
+import os
+import shutil
+import subprocess
+import tempfile
+
 import pytest
 
 import correctionlib
 import correctionlib.schemav2 as cs
 
 
-def test_pyroot_binding():
-    ROOT = pytest.importorskip("ROOT")
-    correctionlib.register_pyroot_binding()
-    assert ROOT.correction.CorrectionSet
-
+@pytest.fixture(scope="module")
+def csetstr():
     ptweight = cs.Correction(
         name="ptweight",
         version=1,
@@ -27,10 +29,74 @@ def test_pyroot_binding():
         ),
     )
     cset = cs.CorrectionSet(schema_version=2, corrections=[ptweight])
-    csetstr = cset.model_dump_json().replace('"', r"\"")
+    return cset.model_dump_json().replace('"', r"\"")
+
+
+def test_pyroot_binding(csetstr: str):
+    ROOT = pytest.importorskip("ROOT")
+    correctionlib.register_pyroot_binding()
+    assert ROOT.correction.CorrectionSet
 
     ROOT.gInterpreter.Declare(
         f'auto cset = correction::CorrectionSet::from_string("{csetstr}");'  # noqa: B907
     )
     ROOT.gInterpreter.Declare('auto corr = cset->at("ptweight");')
     assert ROOT.corr.evaluate([1.2]) == 1.1
+
+
+CMAKELIST_SRC = """\
+cmake_minimum_required(VERSION 3.21 FATAL_ERROR)
+project(test)
+find_package(correctionlib)
+add_executable(test test.cc)
+target_link_libraries(test PRIVATE correctionlib)
+# Because windows has no RPATH, we need to copy the DLLs to the executable directory
+add_custom_command(TARGET test POST_BUILD
+  COMMAND ${CMAKE_COMMAND} -E copy -t $<TARGET_FILE_DIR:test> $<TARGET_RUNTIME_DLLS:test>
+  COMMAND_EXPAND_LISTS
+)
+"""
+
+TESTPROG_SRC = """\
+#include "correction.h"
+
+using correction::CorrectionSet;
+
+int main(int argc, char** argv) {
+  auto cset = CorrectionSet::from_string("%s");
+  auto corr = cset->at("ptweight");
+  if (corr->evaluate({1.2}) != 1.1) {
+    return 1;
+  }
+  return 0;
+}
+"""
+
+
+@pytest.mark.skipif(shutil.which("cmake") is None, reason="cmake not found")
+@pytest.mark.skipif(os.name == "nt", reason="there is a segfault I cannot debug")
+def test_cmake_static_compilation(csetstr: str):
+    with tempfile.TemporaryDirectory() as tmpdir:
+        cmake = os.path.join(tmpdir, "CMakeLists.txt")
+        with open(cmake, "w") as f:
+            f.write(CMAKELIST_SRC)
+        testprog = os.path.join(tmpdir, "test.cc")
+        with open(testprog, "w") as f:
+            f.write(TESTPROG_SRC % csetstr)
+        flags = (
+            subprocess.check_output(["correction", "config", "--cmake"])
+            .decode()
+            .split()
+        )
+        ret = subprocess.run(["cmake", "."] + flags, capture_output=True, cwd=tmpdir)
+        if ret.returncode != 0:
+            print(ret.stdout.decode())
+            print(ret.stderr.decode())
+            raise RuntimeError(f"cmake configuration failed (args: {ret.args})")
+        ret = subprocess.run(["cmake", "--build", "."], capture_output=True, cwd=tmpdir)
+        if ret.returncode != 0:
+            print(ret.stdout.decode())
+            print(ret.stderr.decode())
+            raise RuntimeError(f"cmake build failed (args: {ret.args})")
+        prog = r"Debug\test.exe" if os.name == "nt" else "test"
+        subprocess.run([os.path.join(tmpdir, prog)], check=True, cwd=tmpdir)