Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build Python bindings against existing sparrow installation #9

Open
awvwgk opened this issue Mar 25, 2022 · 8 comments
Open

Build Python bindings against existing sparrow installation #9

awvwgk opened this issue Mar 25, 2022 · 8 comments

Comments

@awvwgk
Copy link

awvwgk commented Mar 25, 2022

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

@nabbelbabbel
Copy link
Member

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
Copy link
Author

awvwgk commented Mar 25, 2022

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
Copy link
Member

nabbelbabbel commented Mar 25, 2022

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
Copy link
Author

awvwgk commented Mar 26, 2022

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

Patch for scine/utilities
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 $<TARGET_FILE:UtilsOSModule> ${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 --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_FILE:${target}>")
     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
Copy link
Member

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
Copy link
Author

awvwgk commented Mar 28, 2022

I'm currently using this setup to build the Python bindings for scine/utilities in conda-forge/staged-recipes#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
Copy link
Member

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
Copy link
Author

awvwgk commented Mar 28, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants