Skip to content

Fix legacy CMake targets - #1105

Open
adayton1 wants to merge 7 commits into
developfrom
bugfix/cmake_target
Open

Fix legacy CMake targets#1105
adayton1 wants to merge 7 commits into
developfrom
bugfix/cmake_target

Conversation

@adayton1

@adayton1 adayton1 commented Aug 5, 2026

Copy link
Copy Markdown
Member

Apparently there are issues when trying to export targets twice even if they end up with different names. So now we try creating an INTERFACE library called "umpire" and link it to "umpire::umpire". We also copy some INTERFACE properties from "umpire::umpire" to "umpire". The same process is used for creating an "umpire_device" target from the "umpire::umpire_device" target.

The following is an error from exporting the "umpire" target twice:

CMake Error: install(EXPORT "chai-targets" ...) includes target "chai" which requires target "umpire" that is not in this export set, but in multiple other export sets: lib/cmake/umpire/umpire-targets.cmake, lib/cmake/umpire/umpire-non-namespaced-targets.cmake.
 
An exported target cannot depend upon another target which is exported multiple times. Consider consolidating the exports of the "umpire" target to a single export.

If the "umpire" target is not created as an "IMPORTED" library, then CHAI fails to export it's own targets:

CMake Error: install(EXPORT "chai-targets" ...) includes target "chai" which requires target "umpire" that is not in any export set.

If INTERFACE_INCLUDE_DIRECTORIES are not copied from "umpire::umpire" to "umpire", then downstream clients can't locate the umpire headers.

Link information is supposed to be transitive, so we link "umpire" to "umpire::umpire" and don't copy INTERFACE properties related to linking.

CMake has no internal way to just copy all target properties, or even to get a list of all the properties on a target. We just copy a few of the most common and important ones. INTERFACE_INCLUDE_DIRECTORIES and INTERFACE_COMPILE_DEFINITIONS are known to be needed.

See #1102, #1101, #1100, #1083, #1067

rhornung67
rhornung67 previously approved these changes Aug 5, 2026
kab163
kab163 previously approved these changes Aug 5, 2026
@kab163

kab163 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

@adayton1 - Did this work for your use case? I want to double check before we merge these changes.

@adayton1

adayton1 commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

It's still failing in some cases - just trying to work through them.

@adayton1

adayton1 commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

This is almost ready. If all tests pass here, in the CHAI release candidate, and in a downstream application, then it should be good to go.

@adayton1

adayton1 commented Aug 7, 2026

Copy link
Copy Markdown
Member Author

Should we make this a patch release or do that in a separate PR?

@adayton1
adayton1 requested review from kab163 and rhornung67 August 7, 2026 23:04

@adayton1 adayton1 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@lc-hubcast approve

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

Successfully merging this pull request may close these issues.

3 participants