Fix CMake export and include interfaces by NikEfth · Pull Request #181 · ETSInitiative/PETSIRD

NikEfth · 2025-12-16T15:17:03Z

Changes in this pull request

When PETSIRD is used as a subproject and exported by a parent project, CMake
requires that exported targets do not reference source-tree paths and that all
public dependencies are also exported.
This PR fixes those issues while keeping standalone PETSIRD builds unchanged.

Related issues

Checklist before requesting a review

[ X ] I have performed a self-review of my code
I have added docstrings/doxygen in line with the guidance in the developer guide
[ X ] The code builds and runs on my machine

Please tick the following:

[ X ] The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in the ETSI software (the Work) under the terms and conditions of the Apache-2.0 License.

for more information, see https://pre-commit.ci

- Add install/export rules for PETSIRD targets - Provide PETSIRDConfig.cmake and version file - Fix include directories using BUILD/INSTALL interfaces - Export PETSIRD::petsird target for find_package() - Prepare PETSIRD for external consumers (STIR -- this has been going on for a while )

for more information, see https://pre-commit.ci

NikEfth · 2025-12-24T11:19:42Z

Finally, this took me 2 week of bouncing around and many problems in STIR:

Very brief summary.

This PR fixes PETSIRD integration issues by properly exporting PETSIRD as a CMake package instead of relying on in-tree submodule builds.

Specifically, it:
• Adds a proper PETSIRDConfig.cmake export
• Exports PETSIRD targets (petsird, petsird_generated)
• Enables downstream projects (e.g. STIR, STIR2PETSIRD) to use find_package(PETSIRD)
• Eliminates ODR / ABI inconsistencies caused by building PETSIRD multiple times
• Fixes hard-to-debug runtime errors where DetectionPosition map lookups failed despite matching coordinates
• Makes PETSIRD usable as a standalone, installed dependency

⸻

Background / Motivation

PETSIRD was previously consumed via a submodule and built inside STIR, which caused:
• Multiple definitions of inline operators (ODR violations)
• ABI mismatches across translation units
• std::map / std::unordered_map lookups failing for identical DetectionPosition values
• AddressSanitizer crashes in listmode and normalisation code paths

These issues became visible once CI enabled stricter sanitizers and newer compilers.

Exporting PETSIRD as a proper CMake package ensures a single canonical definition of all PETSIRD/STIR types across the entire build.

NikEfth · 2025-12-25T14:11:14Z

Further to the previous posts,
This PR fixes a series of CMake and CI issues that prevented PETSIRD from being reliably consumed by downstream projects (well ... STIR) in a clean build environment.

While PETSIRD built correctly on my PC, it failed when used as an external dependency in CI.
The failures gave linker errors as:
ld: cannot find -lxtensor
and missing include errors in STIR.
fatal error: petsird/protocols.h: No such file or directory
These, only appeared in clean environments, which made them easy to miss locally.

Several independent problems linked.

Header-only depencies linked as libraries
Yardl-generated CMakeLists added:
target_link_libraries(petsird_generated xtensor)
Incomplete exported interface

PETSIRD headers were installed, STIR didn't receive include paths unless explicitly added.

CI environment differences
CI used a clean conda-based toolchain. - xtensor is not a system library - architecture-specific compilers were present - implicit system include paths were missing

Fixes:

Correct CMake exports
Remove linking of header-only libraries

KrisThielemans

I think the export is a good idea. It'll also make it easier for putting on conda etc. However, I think it'd be best to export 2 targets: PETSIRD::generated and PETSIRD::helpers.

Potentially, we should INSTALL the examples.

And I guess we need a simple CI test for this as well. sorry.

KrisThielemans · 2025-12-27T19:19:13Z

cpp/helpers/CMakeLists.txt

-find_package(xtensor-blas REQUIRED)
+find_package(xtensor REQUIRED)
+# find_package(xtensor-blas REQUIRED)
+find_package(BLAS REQUIRED)


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)

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.

KrisThielemans · 2025-12-27T19:21:45Z

cpp/helpers/CMakeLists.txt

+
+target_include_directories(petsird_helpers
+  INTERFACE
+  ${xtensor_INCLUDE_DIRS}


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

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```

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

cpp/helpers/CMakeLists.txt

KrisThielemans · 2025-12-27T19:25:44Z

cpp/helpers/CMakeLists.txt

+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()


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

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.

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

cpp/cmake/PETSIRDConfig.cmake.in

cpp/CMakeLists.txt

KrisThielemans · 2025-12-27T22:40:31Z

cpp/CMakeLists.txt

+    date::date
+)
+
+target_compile_features(petsird PUBLIC cxx_std_17)


Interesting. Do we have to do this explicitly?

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

cpp/CMakeLists.txt

NikEfth

I will try step-by-step to bring lxtensor-blas back and see when linking becomes an issue.

NikEfth · 2025-12-28T13:38:44Z

cpp/helpers/CMakeLists.txt

+
+target_include_directories(petsird_helpers
+  INTERFACE
+  ${xtensor_INCLUDE_DIRS}


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

NikEfth · 2025-12-28T13:42:15Z

cpp/helpers/CMakeLists.txt

+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()


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.

NikEfth · 2025-12-28T15:22:11Z

@KrisThielemans I am about to finish the CI yml and I think I have to apologize but my yml skill are not as good as yours (or @casperdcl ) and I make just as typing commands in the terminal :) I could not modify yours successfully, running to all sort of issues. Have a look at mine and let me know if I can improve.

Co-authored-by: Kris Thielemans <[email protected]>

Export 2 targets: PETSIRD::generated and PETSIRD::helpers and PETSIRD as interface target. Reverted Xtesnor dependency

KrisThielemans

I have no idea what you tried to change in CI. I see you use APT, but then conda anyway. I see no fantastic reason to try and avoid conda for the moment. (There's microsoft/yardl#154)

I can't see anything else in the ci.yml that we immediately need for testing functionality of this PR.

I'd therefore suggest to revert ci.yml, but maybe I'm missing something.

Instead, I think we need to do a "ninja install" and add a new step which then links an independent program that against the installed version to test (similar to what we do for the STIR example here).

Maybe we can create cpp/example/ (or examples/cpp I suppose) with a cut-down version of the analysis.cpp.

I can do that, if you prefer.

KrisThielemans · 2025-12-28T19:24:30Z

cpp/CMakeLists.txt

+    PETSIRD::helpers
+)
+
+target_compile_features(petsird INTERFACE cxx_std_17)


looks like this is set minimum version, and is therefore useful, see https://stackoverflow.com/questions/70667513/cmake-cxx-standard-vs-target-compile-features and in particular https://stackoverflow.com/questions/70667513/cmake-cxx-standard-vs-target-compile-features#comment135186815_70667652

However, probably should be added to the generated library already. (In fact, should be in the generated CMakeLists.txt)

microsoft/yardl#261. but we can still add it ourselves

cpp/helpers/CMakeLists.txt

KrisThielemans · 2025-12-28T19:42:27Z

cpp/helpers/CMakeLists.txt

+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()


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

NikEfth · 2025-12-30T15:35:14Z

I had to change the CI, because I could not make it to compile yardl. I tried.
We need yardl from the main branch as 0.6.3 does not include the fixes to the xtensor includes.
The lines that I really need are the following:

 - name: Setup Go
        uses: actions/setup-go@v5
        with:
          go-version: '1.22'

      - name: Build and install Yardl
        run: |
          cd yardl/tooling/cmd/yardl
          go build
          mkdir -p "$HOME/.local/bin"
          install -m 755 yardl $HOME/.local/bin/yardl
          echo "$HOME/.local/bin" >> $GITHUB_PATH
          "$HOME/.local/bin/yardl" --version

      - name: Setup tmate session if triggered
        #if: ${{ failure() }}
        uses: mxschmitt/action-tmate@v3
        timeout-minutes: 30
        if: ${{ github.event_name == 'workflow_dispatch' && inputs.debug_enabled == 'true' }}

      - name: Install just
        run: |
          sudo apt-get update
          sudo apt-get install -y cmake ninja-build just

If you can tell me how to make this work, I am happy to revert.

Instead, I think we need to do a "ninja install" and add a new step which then links an independent program that against the installed version to test (similar to what we do for the STIR example here).

I do this quite extensively in STIR and STIR2PETSIRD. But, sure we can do it here, too.

NikEfth and others added 3 commits December 16, 2025 16:10

Fix CMake export and include interfaces

d5a94db

Adjust CMake to support embedding in parent project

74fecb7

[pre-commit.ci] auto fixes from pre-commit.com hooks

640644a

for more information, see https://pre-commit.ci

pre-commit-ci bot temporarily deployed to pypi December 17, 2025 00:41 Inactive

KrisThielemans self-requested a review December 17, 2025 00:41

KrisThielemans self-assigned this Dec 17, 2025

Install in the correct location when build with STIR

62455c3

NikEfth marked this pull request as draft December 24, 2025 08:16

NikEfth and others added 5 commits December 24, 2025 09:51

[pre-commit.ci] auto fixes from pre-commit.com hooks

f1e880a

for more information, see https://pre-commit.ci

More changes for CMake export path fix

e5db953

This should be the last commit. STIR links successfully now.

9cffc2b

Merge branch 'ETSInitiative:main' into fix/cmake-export

960796b

NikEfth marked this pull request as ready for review December 24, 2025 11:17

NikEfth added 5 commits December 25, 2025 11:49

This seems to necessary for CI.

8751dae

Restore previous commit.

2d6daa1

Still something asks for lxtensor.

f898abd

Some conda package is broke and linlks to lxtensor.

66a54c1

lxtensor still leaks.

9d8ef4d

Find dependencies (even if optional)

b813de2

KrisThielemans requested changes Dec 27, 2025

View reviewed changes

NikEfth closed this Dec 28, 2025

NikEfth reopened this Dec 28, 2025

NikEfth commented Dec 28, 2025

View reviewed changes

NikEfth and others added 3 commits December 28, 2025 17:49

Update cpp/CMakeLists.txt

0c8fb71

Co-authored-by: Kris Thielemans <[email protected]>

Update cpp/CMakeLists.txt

e0d9ca0

Co-authored-by: Kris Thielemans <[email protected]>

Update cpp/CMakeLists.txt

e028d87

Co-authored-by: Kris Thielemans <[email protected]>

NikEfth added 2 commits December 28, 2025 18:26

New CI workflow that builds and deploys PyPI package

94e39b0

Export 2 targets: PETSIRD::generated and PETSIRD::helpers and PETSIRD as interface target. Reverted Xtesnor dependency

pre-commit

4a98590

NikEfth requested a review from KrisThielemans December 28, 2025 17:37

KrisThielemans requested changes Dec 28, 2025

View reviewed changes

Conversation

NikEfth commented Dec 16, 2025

Changes in this pull request

Related issues

Checklist before requesting a review

Uh oh!

NikEfth commented Dec 24, 2025

Uh oh!

NikEfth commented Dec 25, 2025

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikEfth Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NikEfth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikEfth commented Dec 28, 2025

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikEfth commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NikEfth Dec 28, 2025 •

edited

Loading