qcscine / sparrow

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

Build Python bindings against existing sparrow installation #9

Open awvwgk opened 2 years ago

awvwgk commented 2 years ago

Is there a possibility to build the Python bindings against an existing installation of sparrow?

nabbelbabbel commented 2 years ago

I think the answer is: no.

The wrapper requires its own source files that are present in the repository. It is thus required to have these sources when building the wrapper. This means all source files of the entire project exist at the build time of the wrapper. We have not added a mechanism to build just the wrapper from "new" sources, while using an "old" installation for the libraries/executable.

I think this is what what you are asking for, right?

awvwgk commented 2 years ago

For some context, I'm trying to build Python bindings with multiple versions of Python and want to reduce the overhead from compiling the C++ part over and over again. Having just the C++ library installed first and than compile the Python bindings for several Python versions against the "old" library would be very useful.

nabbelbabbel commented 2 years ago

Ohh. then this may be all you need:

cmake -DSCINE_BUILD_PYTHON_BINDINGS=ON -DPYTHON_EXECUTABLE=/usr/bin/python3.9 ..
make install
cmake -DSCINE_BUILD_PYTHON_BINDINGS=ON -DPYTHON_EXECUTABLE=/usr/bin/python3.8 ..
make install
...

You should be able to just reconfigure cmake, not needing to rebuild the other targets. (I have not tested this, but it may work.)

This still has the overhead of installing the main library every time, but it hopefully builds it only once.

awvwgk commented 2 years ago

I think I could solve this by introducing a new switch to skip the build of the library

Patch for scine/utilities ```diff diff --git a/src/Utils/CMakeLists.txt b/src/Utils/CMakeLists.txt index f435593..cfc8fb7 100644 --- a/src/Utils/CMakeLists.txt +++ b/src/Utils/CMakeLists.txt @@ -29,6 +29,7 @@ if(SCINE_PARALLELIZE) find_package(OpenMP REQUIRED) endif() +if(NOT SCINE_SKIP_LIBRARY) # Obey standard CMake behavior regarding shared/static libraries add_library(UtilsOS ${UTILS_HEADERS} ${UTILS_CPPS}) if(NOT BUILD_SHARED_LIBS) @@ -119,6 +120,10 @@ if(SCINE_BUILD_TESTS) DESTINATION ${CMAKE_CURRENT_BINARY_DIR} ) endif() +else() +include(ImportUtilsOS) +import_utils_os() +endif() # Python bindings if(SCINE_BUILD_PYTHON_BINDINGS) @@ -164,11 +169,13 @@ if(SCINE_BUILD_PYTHON_BINDINGS) ${CMAKE_CURRENT_BINARY_DIR}/scine_utilities/__init__.py ) file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/Python/README.rst DESTINATION ${CMAKE_CURRENT_BINARY_DIR}) + if(NOT SCINE_SKIP_LIBRARY) add_custom_command(TARGET UtilsOSModule POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy $ ${CMAKE_CURRENT_BINARY_DIR}/scine_utilities COMMENT "Copying UtilsOS module into python package directory" ) add_dependencies(scine_utilities UtilsOSModule) + endif() # Figure out which targets need to be copied along set(_py_targets_to_copy Scine::Core) # Core is always shared @@ -187,11 +194,13 @@ if(SCINE_BUILD_PYTHON_BINDINGS) string(APPEND utils_PY_DEPS ", \"${_target_filename}\"") endforeach() + if(NOT SCINE_SKIP_LIBRARY) add_custom_command(TARGET UtilsOS POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy ${_py_target_gen_exprs} ${CMAKE_CURRENT_BINARY_DIR}/scine_utilities COMMENT "Copying dependent shared libraries into python package directory" ) message(STATUS "Targets to copy for python bindings: ${_py_targets_to_copy}") + endif() unset(_py_targets_to_copy) # Typing stubs @@ -227,10 +236,12 @@ if(SCINE_BUILD_PYTHON_BINDINGS) OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/doc-py DOCTEST ) + if(NOT SCINE_SKIP_LIBRARY) # The UtilsOSModule is technically also a dependency of the documentation if(TARGET scine_utilitiesDocumentation) add_dependencies(scine_utilitiesDocumentation UtilsOSModule) endif() + endif() endif() if(SCINE_BUILD_PYTHON_BINDINGS) ```
Patch for scine/sparrow ```diff diff --git a/src/Sparrow/CMakeLists.txt b/src/Sparrow/CMakeLists.txt index 3d67744..5f14bd6 100644 --- a/src/Sparrow/CMakeLists.txt +++ b/src/Sparrow/CMakeLists.txt @@ -17,6 +17,11 @@ 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() + +if(NOT SCINE_SKIP_LIBRARY) # Sparrow is a Scine module and is always shared, never linked against add_library(Sparrow SHARED ${SPARROW_MODULE_FILES}) @@ -87,7 +92,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() @@ -106,6 +110,10 @@ if(SCINE_BUILD_TESTS) ENVIRONMENT PATH=${CMAKE_CURRENT_BINARY_DIR}:$ENV{PATH} ) endif() +else() +include(ImportSparrow) +import_sparrow() +endif() # Set the RPATH to be used when installing. if(APPLE) @@ -115,6 +123,7 @@ else() set(CMAKE_INSTALL_RPATH "\$ORIGIN;\$ORIGIN/../lib") endif() +if(NOT SCINE_SKIP_LIBRARY) # Executable add_executable(SparrowApp ${SPARROW_APP_FILES}) add_executable(Scine::SparrowApp ALIAS SparrowApp) @@ -144,6 +153,7 @@ target_link_libraries(RTSpectroscopyDriver if(WIN32 AND MINGW) target_link_libraries(SparrowApp PRIVATE ws2_32) endif() +endif() # Python Bindings if(SCINE_BUILD_PYTHON_BINDINGS) @@ -159,18 +169,22 @@ if(SCINE_BUILD_PYTHON_BINDINGS) # Generate generator expressions for each targets and figure out filenames # for the python setup file set(sparrow_PY_DEPS "") + if(NOT SCINE_SKIP_LIBRARY) foreach(target ${_py_targets_to_copy}) list(APPEND _py_target_gen_exprs "\$") target_lib_filename(${target} _target_filename) string(APPEND sparrow_PY_DEPS ", \"${_target_filename}\"") endforeach() message(STATUS "Targets to copy for python bindings: ${_py_targets_to_copy}") + endif() unset(_py_targets_to_copy) + if(NOT SCINE_SKIP_LIBRARY) add_custom_command(TARGET Sparrow POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy ${_py_target_gen_exprs} ${CMAKE_CURRENT_BINARY_DIR}/scine_sparrow COMMENT "Copying required shared libraries into python package directory" ) + endif() unset(_py_target_gen_exprs) install(CODE @@ -245,7 +259,9 @@ if(SCINE_BUILD_PYTHON_BINDINGS) ) endif() +if(NOT SCINE_SKIP_LIBRARY) add_subdirectory(Embed) # Targets install(TARGETS SparrowApp RUNTIME DESTINATION bin) +endif() ```
nabbelbabbel commented 2 years ago

I have tested the above codes, both versions, yours and mine work.

While I understand the usage of your flag in the use case that you describe I am unsure if it is clean to just add it. The ImportXYZ statements are not versioned, as in, they return any version there is on the system. I think for this behavior it would be best if there was a tight version check, making sure that the wrapper actually does what it is intended to do.

For now this issue will stay open, I have not staged that patch for the next release.

awvwgk commented 2 years ago

I'm currently using this setup to build the Python bindings for scine/utilities in https://github.com/conda-forge/staged-recipes/pull/18482. The version is currently only pinned to an upper bound using the major version as constraint, but I can make the version pin exact if this is preferred.

nabbelbabbel commented 2 years ago

To clarify:

I was talking about the import in your patch.

if(NOT SCINE_SKIP_LIBRARY)
  [...]
else()
  include(ImportSparrow)
  import_sparrow()
endif()

with this it is possible to have version 3.0.0 installed on a system, but to try and build the version 4.0.0 bindings against it. This is something that I think is unclean.

If you generate the same possible problem in the conda dependency resolution, then yes I would advise to fix the wrapper version to be the same as the library version.

awvwgk commented 2 years ago

I made the version constraint exact to ensure it always matches inside the conda build and environment. This should ensure my little hack for splitting the C++ and Python part of the library will be compatible.