Skip to content

Cmake incremental builds improvement #1586

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Cmake incremental builds improvement #1586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
414a9d6
Do not install header files which are copied during building steps
oleksandr-pavlyk Mar 7, 2024
fd1d43c
Formatting changes only
oleksandr-pavlyk Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
cmake_minimum_required(VERSION 3.21...3.27 FATAL_ERROR)

project(dpctl
VERSION 0.15
VERSION 0.17
LANGUAGES CXX
DESCRIPTION "Python interface for XPU programming"
)
Expand Down Expand Up @@ -47,11 +47,6 @@ endif()

add_subdirectory(libsyclinterface)

file(GLOB _dpctl_capi_headers dpctl/apis/include/*.h*)
install(FILES ${_dpctl_capi_headers}
DESTINATION dpctl/include
)

# Define CMAKE_INSTALL_xxx: LIBDIR, INCLUDEDIR
include(GNUInstallDirs)

Expand Down
64 changes: 32 additions & 32 deletions dpctl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ foreach(hf ${_syclinterface_h})
add_custom_command(OUTPUT ${_target_header_file}
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${hf} ${_target_header_file}
DEPENDS ${hf} _build_time_create_dpctl_include
VERBATIM
VERBATIM
)
endforeach()

Expand Down Expand Up @@ -144,19 +144,19 @@ function(build_dpctl_ext _trgt _src _dest)
Python_add_library(${_trgt} MODULE WITH_SOABI ${_generated_src})
if (BUILD_DPCTL_EXT_SYCL)
add_sycl_to_target(TARGET ${_trgt} SOURCES ${_generated_src})
if(_dpctl_sycl_targets)
# make fat binary
if(_dpctl_sycl_targets)
# make fat binary
target_compile_options(
${_trgt}
PRIVATE
-fsycl-targets=${_dpctl_sycl_targets}
)
target_link_options(
${_trgt}
PRIVATE
-fsycl-targets=${_dpctl_sycl_targets}
)
endif()
target_link_options(
${_trgt}
PRIVATE
-fsycl-targets=${_dpctl_sycl_targets}
)
endif()
endif()
target_include_directories(${_trgt} PRIVATE ${NumPy_INCLUDE_DIR} ${DPCTL_INCLUDE_DIR})
add_dependencies(${_trgt} _build_time_create_dpctl_include_copy ${_cythonize_trgt})
Expand All @@ -173,31 +173,31 @@ function(build_dpctl_ext _trgt _src _dest)
set(_generated_api_h "${_generated_src_dir}/${_name_wle}_api.h")
set(_copy_trgt "${_trgt}_copy_capi_include")
add_custom_target(
${_copy_trgt} ALL
COMMAND ${CMAKE_COMMAND}
-DSOURCE_FILE=${_generated_public_h}
-DDEST=${CMAKE_CURRENT_SOURCE_DIR}
-P ${CMAKE_SOURCE_DIR}/dpctl/cmake/copy_existing.cmake
COMMAND ${CMAKE_COMMAND}
-DSOURCE_FILE=${_generated_api_h}
-DDEST=${CMAKE_CURRENT_SOURCE_DIR}
-P ${CMAKE_SOURCE_DIR}/dpctl/cmake/copy_existing.cmake
DEPENDS ${_trgt}
VERBATIM
COMMENT "Copying Cython-generated headers to dpctl"
${_copy_trgt} ALL
COMMAND ${CMAKE_COMMAND}
-DSOURCE_FILE=${_generated_public_h}
-DDEST=${CMAKE_CURRENT_SOURCE_DIR}
-P ${CMAKE_SOURCE_DIR}/dpctl/cmake/copy_existing.cmake
COMMAND ${CMAKE_COMMAND}
-DSOURCE_FILE=${_generated_api_h}
-DDEST=${CMAKE_CURRENT_SOURCE_DIR}
-P ${CMAKE_SOURCE_DIR}/dpctl/cmake/copy_existing.cmake
DEPENDS ${_trgt}
VERBATIM
COMMENT "Copying Cython-generated headers to dpctl"
)
if (DPCTL_GENERATE_COVERAGE)
set(_copy_cxx_trgt "${_trgt}_copy_cxx")
add_custom_target(
${_copy_cxx_trgt} ALL
COMMAND ${CMAKE_COMMAND}
-DSOURCE_FILE=${_generated_src}
-DDEST=${CMAKE_CURRENT_SOURCE_DIR}
-P ${CMAKE_SOURCE_DIR}/dpctl/cmake/copy_existing.cmake
DEPENDS ${_trgt}
VERBATIM
COMMENT "Copying Cython-generated source to dpctl"
)
set(_copy_cxx_trgt "${_trgt}_copy_cxx")
add_custom_target(
${_copy_cxx_trgt} ALL
COMMAND ${CMAKE_COMMAND}
-DSOURCE_FILE=${_generated_src}
-DDEST=${CMAKE_CURRENT_SOURCE_DIR}
-P ${CMAKE_SOURCE_DIR}/dpctl/cmake/copy_existing.cmake
DEPENDS ${_trgt}
VERBATIM
COMMENT "Copying Cython-generated source to dpctl"
)
endif()
install(TARGETS ${_trgt} LIBRARY DESTINATION ${_dest})
endfunction()
Expand Down
2 changes: 1 addition & 1 deletion libsyclinterface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ set_target_properties(DPCTLSyclInterface

if (SKBUILD)
set(_lib_destination dpctl)
set(_include_destination dpctl/include/syclinterface)
set(_include_destination ${CMAKE_INSTALL_PREFIX}/unused)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoiding that syclinterface header, which are copied to dpctl/include/syclinterface as part of build step be overwritten by the install. The install just puts them in _skbuild/*/cmake-install/unused folder, and setuptools does not copy them into the Python layout, and instead copies files from dpctl/include/syclinterface like intended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why is this step needed all together? Will it not work, if the line is completely removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is used in subsequent 3 install commands.

Removing this line would require moving these install commands inside if/else branch. It would make the script longer, but it can be done

else()
set(_lib_destination ${CMAKE_INSTALL_PREFIX}/lib)
set(_include_destination ${CMAKE_INSTALL_PREFIX}/include)
Expand Down