Repository: cms-nanoAOD/correctionlib (language: python)

Issue:
The exported CMake configuration doesn't look for dependencies correctly
The following `CMakeLists.txt` fails to generate in LCG 104 (correctionlib 2.2.2, but as far as I can tell the code hasn't changed since), with a trivial `test.cc`:
```cmake
cmake_minimum_required(VERSION 3.16 FATAL_ERROR)
project(test)
find_package(correctionlib)
add_executable(test test.cc)
target_link_libraries(test correctionlib)
```
I get the following output:
```
$ cmake -B build $(correction config --cmake)
-- The C compiler identification is GNU 12.1.0
-- The CXX compiler identification is GNU 12.1.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /cvmfs/sft.cern.ch/lcg/releases/gcc/12.1.0-57c96/x86_64-centos7/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /cvmfs/sft.cern.ch/lcg/releases/gcc/12.1.0-57c96/x86_64-centos7/bin/g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done (0.8s)
CMake Error at /cvmfs/sft.cern.ch/lcg/views/LCG_104/x86_64-centos7-gcc12-opt/cmake/correctionlib-targets.cmake:59 (set_target_properties):
  The link interface of target "correctionlib" contains:

    ZLIB::ZLIB

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /cvmfs/sft.cern.ch/lcg/views/LCG_104/x86_64-centos7-gcc12-opt/cmake/correctionlibConfig.cmake:10 (include)
  CMakeLists.txt:3 (find_package)


-- Generating done (0.0s)
CMake Generate step failed.  Build files cannot be regenerated correctly.
```
The reason is that ZLIB (and Threads) are added as `PUBLIC` dependencies here (not specifying anything equates to `PUBLIC`): https://github.com/cms-nanoAOD/correctionlib/blob/5b75bf5c42e24aea38b994e385e61e8735a08a93/CMakeLists.txt#L52-L55

This informs CMake that any dependent target needs to add link `libz` and `libpthread` explicitly. This is wrong: neither `zlib.h` nor threads are part of the public interface of correctionlib. These should be made `PRIVATE` instead:
```cmake

if(ZLIB_FOUND)
  target_link_libraries(correctionlib PRIVATE ZLIB::ZLIB)
endif()
if(Threads_FOUND AND CMAKE_SYSTEM_NAME STREQUAL "Linux")
  target_link_libraries(correctionlib PRIVATE Threads::Threads)
endif()
```
Note that linking the dependent targets against `libz.so` or `libpthreads.so` is not needed, the linker is smart enough to figure out this part.

Alternatively, [Config.cmake.in](https://github.com/cms-nanoAOD/correctionlib/blob/master/Config.cmake.in) should use [find_dependency](https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html#command:find_dependency) to find zlib and pthreads at import time.


Discussion / hints:
Thank you for your report. Would you be interested in making a PR to fix the issue?
If not, I am happy to but may request a review from you if that is ok.
Hi @nsmith-, thanks! I'm a bit overwhelmed by the number of PRs I have currently, so it would be great if you could do it. I'm happy to review the PR

Fix patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 75fe2a4d..2be6c03f 100644
--- 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
index 679eb6c3..d0696ea3 100644
--- 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
index 5248c23b..06c892fd 100644
--- 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
index 7dbfb19a..5723410c 100644
--- 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
index 77187de7..b3d2b24d 100644
--- 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
   }
 }


Test patch:
diff --git a/tests/test_binding.py b/tests/test_binding.py
index 0c290709..5f41e70d 100644
--- 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)