Skip to content

Fix CMake export and include interfaces #181

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

Open
NikEfth wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Fix CMake export and include interfaces #181

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d5a94db
Fix CMake export and include interfaces
NikEfth Dec 16, 2025
74fecb7
Adjust CMake to support embedding in parent project
NikEfth Dec 16, 2025
640644a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 16, 2025
62455c3
Install in the correct location when build with STIR
NikEfth Dec 18, 2025
7a68f0e
CMake: export PETSIRD as a proper installable package
NikEfth Dec 24, 2025
f1e880a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 24, 2025
e5db953
More changes for CMake export path fix
NikEfth Dec 24, 2025
9cffc2b
This should be the last commit. STIR links successfully now.
NikEfth Dec 24, 2025
960796b
Merge branch 'ETSInitiative:main' into fix/cmake-export
NikEfth Dec 24, 2025
8751dae
This seems to necessary for CI.
NikEfth Dec 25, 2025
2d6daa1
Restore previous commit.
NikEfth Dec 25, 2025
f898abd
Still something asks for lxtensor.
NikEfth Dec 25, 2025
66a54c1
Some conda package is broke and linlks to lxtensor.
NikEfth Dec 25, 2025
9d8ef4d
lxtensor still leaks.
NikEfth Dec 25, 2025
b813de2
Find dependencies (even if optional)
NikEfth Dec 25, 2025
0c8fb71
Update cpp/CMakeLists.txt
NikEfth Dec 28, 2025
e0d9ca0
Update cpp/CMakeLists.txt
NikEfth Dec 28, 2025
e028d87
Update cpp/CMakeLists.txt
NikEfth Dec 28, 2025
94e39b0
New CI workflow that builds and deploys PyPI package
NikEfth Dec 28, 2025
4a98590
pre-commit
NikEfth Dec 28, 2025
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
70 changes: 66 additions & 4 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
cmake_minimum_required(VERSION 3.12.0) # older would work, but could give warnings on policy CMP0074
project(petsird VERSION 0.7.2)
project(petsird VERSION 0.7.2 LANGUAGES CXX)
include(GNUInstallDirs)
set(PETSIRD_CMAKE_DIR "${CMAKE_INSTALL_LIBDIR}/cmake/PETSIRD")

include(CMakePackageConfigHelpers)

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

if(WIN32)
add_compile_options(/W3 /WX)
Expand All @@ -15,14 +20,71 @@ if(CCACHE_PROGRAM)
message(STATUS "ccache found, so we will use it.")
endif()

find_package(date REQUIRED)

# assume that yardl was run with the following in the _package.yml
# sourcesOutputDir: ../cpp/generated/petsird

add_subdirectory(generated/petsird)
add_subdirectory(helpers)
# create a petsird library target
# Currently it is just empty, but allows creating target properties which are transitive
add_library(petsird)
target_link_libraries(petsird PUBLIC petsird_generated)
target_include_directories(petsird PUBLIC "${PROJECT_SOURCE_DIR}/generated")
add_library(PETSIRD::petsird ALIAS petsird)

add_subdirectory(helpers)
target_link_libraries(petsird
PUBLIC
petsird_generated
petsird_helpers
date::date
)

target_compile_features(petsird PUBLIC cxx_std_17)
Copy link
Contributor

@KrisThielemans KrisThielemans Dec 27, 2025

Choose a reason for hiding this comment

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

Interesting. Do we have to do this explicitly?

Copy link
Author

@NikEfth NikEfth Dec 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I thought it would be a good idea. idk? Initially I was getting many mysterious compiler errors and tried to cover all bases.


target_include_directories(petsird
PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/generated>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/helpers/include>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)

install(TARGETS petsird petsird_generated petsird_helpers
EXPORT petsirdTargets
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
)

install(
DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/generated/petsird/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/petsird
)

install(
DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/helpers/include/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

install(EXPORT petsirdTargets
NAMESPACE PETSIRD::
DESTINATION ${PETSIRD_CMAKE_DIR}
)


configure_package_config_file(
cmake/PETSIRDConfig.cmake.in
${CMAKE_CURRENT_BINARY_DIR}/PETSIRDConfig.cmake
INSTALL_DESTINATION ${PETSIRD_CMAKE_DIR}
)

write_basic_package_version_file(
${CMAKE_CURRENT_BINARY_DIR}/PETSIRDConfigVersion.cmake
VERSION ${PROJECT_VERSION}
COMPATIBILITY SameMajorVersion
)

install(FILES
${CMAKE_CURRENT_BINARY_DIR}/PETSIRDConfig.cmake
${CMAKE_CURRENT_BINARY_DIR}/PETSIRDConfigVersion.cmake
DESTINATION ${PETSIRD_CMAKE_DIR}
)
12 changes: 12 additions & 0 deletions cpp/cmake/PETSIRDConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
@PACKAGE_INIT@
include(CMakeFindDependencyMacro)
find_dependency(date)
find_dependency(BLAS)
if(@PETSIRD_GENERATED_USE_NDJSON@)
find_dependency(nlohmann_json)
endif()
if(@PETSIRD_GENERATED_USE_HDF5@)
find_dependency(HDF5 COMPONENTS CXX)
endif()
include("${CMAKE_CURRENT_LIST_DIR}/petsirdTargets.cmake")
check_required_components(PETSIRD)
30 changes: 25 additions & 5 deletions cpp/helpers/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,33 @@
find_package(xtensor-blas REQUIRED)
find_package(xtensor REQUIRED)
# find_package(xtensor-blas REQUIRED)
find_package(BLAS REQUIRED)
Copy link
Contributor

@KrisThielemans KrisThielemans Dec 27, 2025

Choose a reason for hiding this comment

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

I disagree.

  • geometry.h uses
#include <xtensor-blas/xlinalg.hpp>
  • helpers depends on generated, which has 
    find_package(xtensor ${XTENSOR_MINIMUM_VERSION} REQUIRED)
    We don't use BLAS directly.
  • xtensor-blas will require xtensor (and I see no reason we need its variables, see below)

Copy link
Author

@NikEfth NikEfth Dec 28, 2025

Choose a reason for hiding this comment

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

I commented out xtensor-blas because of an unwanted lxtensor linkage (When linking with STIR, in a conda environemet I was gettign a lxtensor ), but that’s a packaging issue. I will bring this back now and see how it goes. Maybe other changes resolved this.


# create a petsird_helpers library target
# Currently it is just empty, but allows creating target properties which are transitive
add_library(petsird_helpers INTERFACE)
target_link_libraries(petsird_helpers INTERFACE petsird xtensor-blas)
target_include_directories(petsird_helpers INTERFACE "${PROJECT_SOURCE_DIR}/helpers/include")

target_include_directories(petsird_helpers
INTERFACE
${xtensor_INCLUDE_DIRS}
Copy link
Contributor

@KrisThielemans KrisThielemans Dec 27, 2025

Choose a reason for hiding this comment

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

should not be necessary. Depending on xtensor-blas should take care of this

Copy link
Author

@NikEfth NikEfth Dec 28, 2025

Choose a reason for hiding this comment

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

See the comment before. Xtensor is headers only, but somehow STIR, for example:

| collect2: error: ld returned 1 exit status
| gmake[2]: *** [src/test/numerics/CMakeFiles/test_IR_filters.dir/build.make:121: src/test/numerics/test_IR_filters] Error 1
| gmake[1]: *** [CMakeFiles/Makefile2:8070: src/test/numerics/CMakeFiles/test_IR_filters.dir/all] Error 2
| gmake[1]: *** Waiting for unfinished jobs....

/root/micromamba/envs/yardl/bin/ld: cannot find -lxtensor: No such file or directory
| collect2: error: ld returned 1 exit status
| gmake[2]: *** [src/test/numerics/CMakeFiles/test_BSplines.dir/build.make:121: src/test/numerics/test_BSplines] Error 1
| gmake[1]: *** [CMakeFiles/Makefile2:8102: src/test/numerics/CMakeFiles/test_BSplines.dir/all] Error 2```

Copy link
Author

@NikEfth NikEfth Dec 28, 2025

Choose a reason for hiding this comment

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

I have to note that the error in the comment above was just an instance of a torrent of similar problems.

$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)

target_link_libraries(petsird_helpers INTERFACE petsird BLAS::BLAS)

add_executable(petsird_generator petsird_generator.cpp)
target_link_libraries(petsird_generator petsird_helpers)
target_link_libraries(petsird_generator PRIVATE petsird_helpers)

add_executable(petsird_analysis petsird_analysis.cpp)
target_link_libraries(petsird_analysis petsird_helpers)
target_link_libraries(petsird_analysis PRIVATE petsird_helpers)

foreach(t petsird petsird_generated petsird_helpers)
if(TARGET ${t})
get_target_property(_libs ${t} INTERFACE_LINK_LIBRARIES)
if(_libs)
list(FILTER _libs EXCLUDE REGEX "xtensor")
set_target_properties(${t}
PROPERTIES INTERFACE_LINK_LIBRARIES "${_libs}")
endif()
endif()
endforeach()
Comment on lines +25 to +34
Copy link
Contributor

@KrisThielemans KrisThielemans Dec 27, 2025

Choose a reason for hiding this comment

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

why did you need this? (probably because you removed target_link_libraries)

Copy link
Author

@NikEfth NikEfth Dec 28, 2025

Choose a reason for hiding this comment

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

To undo an overly-aggressive dependency propagation introduced by generated / helper targets.
xtensor is header-only
However… Some targets propagate xtensor via INTERFACE_LINK_LIBRARIES
Something like:
INTERFACE_LINK_LIBRARIES xtensor::xtensor
This becomes a real problem in STIR / STIR2PETSIRD. Header-only deps should not force downstream linking.

Copy link
Contributor

@KrisThielemans KrisThielemans Dec 28, 2025

Choose a reason for hiding this comment

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

As long as xtensor is header-only, we could use the same trick as https://github.com/UCL/STIR/blob/ec3a5670f9b63cb10f4bebce6edf22d6f028a6fb/src/buildblock/CMakeLists.txt#L125-L137, i.e. only add the include path.

However, this is all quite ugly. I'd rather avoid it as much as possible.

As long as we have find_dependency(xtensor) in the Config.cmake, there should be no problem