qcscine / sparrow

https://scine.ethz.ch
BSD 3-Clause "New" or "Revised" License
78 stars 15 forks source link

Python is not imported if tests are disabled #11

Closed awvwgk closed 2 years ago

awvwgk commented 2 years ago

Enabling the Python bindings and turning off the tests will not import Python and therefore fail to install

❯ cmake -B __build -G Ninja -DSCINE_BUILD_TESTS=OFF -DSCINE_BUILD_PYTHON_BINDINGS=ON -DCMAKE_INSTALL_PREFIX=ON -DCMAKE_INSTALL_PREFIX=$PWD/PREFIX -DSCINE_MARCH=""
-- The C compiler identification is GNU 11.1.0
-- The CXX compiler identification is GNU 11.1.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - 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: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Setting build type to default 'RelWithDebInfo'
-- Scine::UtilsOS found locally at /home/awvwgk/projects/src/git/scine/sparrow/PREFIX/lib/cmake/ScineUtilsOS
CMake Deprecation Warning at __build/cereal-src/CMakeLists.txt:1 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

CMake Warning (dev) at __build/cereal-src/CMakeLists.txt:2 (project):
  Policy CMP0048 is not set: project() command manages VERSION variables.
  Run "cmake --help-policy CMP0048" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.

  The following variable(s) would be set to empty:

    PROJECT_VERSION
    PROJECT_VERSION_MAJOR
    PROJECT_VERSION_MINOR
    PROJECT_VERSION_PATCH
    cereal_VERSION
    cereal_VERSION_MAJOR
    cereal_VERSION_MINOR
    cereal_VERSION_PATCH
    cereal_VERSION_TWEAK
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Cereal was not found in your PATH, so it was downloaded.
-- Found OpenMP_C: -fopenmp (found version "4.5") 
-- Found OpenMP_CXX: -fopenmp (found version "4.5") 
-- Found OpenMP: TRUE (found version "4.5")  
-- Found Boost: /usr/lib64/cmake/Boost-1.78.0/BoostConfig.cmake (found version "1.78.0") found components: program_options filesystem 
-- Targets to copy for python bindings: Sparrow;Scine::UtilsOS
-- Could NOT find PY_sphinx (missing: PY_SPHINX) 
-- Sphinx python package not found, cannot build python documentation
-- Configuring done
-- Generating done
-- Build files have been written to: /home/awvwgk/projects/src/git/scine/sparrow/__build

The generated install code is missing the Python interpreter which should invoke pip:

❯ cat __build/src/Sparrow/cmake_install.cmake
...
if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  execute_process(COMMAND  -m pip install --prefix=/home/awvwgk/projects/src/git/scine/sparrow/PREFIX --upgrade --no-deps /home/awvwgk/projects/src/git/scine/sparrow/__build/src/Sparrow
                     RESULT_VARIABLE retcode)
     if(NOT ${retcode} EQUAL 0)
       message(FATAL_ERROR "Fatal error when installing Python module using PIP.")
     endif()
endif()

Potential fix:

diff --git a/src/Sparrow/CMakeLists.txt b/src/Sparrow/CMakeLists.txt
index 3d67744..9562705 100644
--- a/src/Sparrow/CMakeLists.txt
+++ b/src/Sparrow/CMakeLists.txt
@@ -17,6 +17,10 @@ if(NOT TARGET Boost::filesystem OR NOT TARGET Boost::program_options)
   find_package(Boost REQUIRED COMPONENTS program_options filesystem)
 endif()

+if(SCINE_BUILD_TESTS OR SCINE_BUILD_PYTHON_BINDINGS)
+  find_package(PythonInterp REQUIRED)
+endif()
+
 # Sparrow is a Scine module and is always shared, never linked against
 add_library(Sparrow SHARED ${SPARROW_MODULE_FILES})

@@ -87,7 +91,6 @@ if(SCINE_BUILD_TESTS)
     ${CMAKE_DL_LIBS}
   )
   add_test(NAME Sparrow COMMAND Sparrow_tests)
-  find_package(PythonInterp REQUIRED)
   if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
     set(TEST_SELECTION "::TestSparrowFast")
   else()
nabbelbabbel commented 2 years ago

Thanks, the fix looks fine, I have added it to the next release.

reiher-research-group commented 2 years ago

Release 3.0.1 of Sparrow is out, so this should be fixed.