Skip to content

Conversation

@zachcran
Copy link
Contributor

@zachcran zachcran commented Jun 23, 2025

Is this pull request associated with an issue(s)?

Fixes #166.

Description

Main Fix

This PR updates cmaize_find_optional_dependency() (cfod()) and cmaize_find_or_build_optional_dependency() (cfobod()) calls to reference the correct target name when adding the cmaize_option() enabling the optional dependency to the dependency target's target_compile_definitions().

Enhanced Output

While debugging this issue, I had to investigate code spanning most of the life cycle of a target (both dependency targets and otherwise), so I also added more verbose output where possible, which can be enabled by setting the message log level at or finer than VERBOSE (log levels here).

TODOs

  • Tested on Sigma and TensorWrapper with both cfod() and cfobod() calls for the Eigen dependency, both when Eigen is installed and not.
  • Fix failing unit tests (I am testing with CMake 3.26-rc4).
  • Test CMake>=4. Even if the CMake 3.x tests pass, docs/bare_bones_cmake was failing in CMake 4.0.2 and needs to be checked manually.
  • Check integration tests on my system.

@zachcran zachcran requested a review from ryanmrichard June 23, 2025 16:07
@zachcran zachcran self-assigned this Jun 23, 2025
ryanmrichard
ryanmrichard previously approved these changes Jun 23, 2025
@zachcran
Copy link
Contributor Author

zachcran commented Jun 23, 2025

Fixed failing tests on CMake 3.26.0-rc4 and the unit tests here are passing. Still need to verify on CMake 4.x.

@zachcran
Copy link
Contributor Author

On CMake 4.0.2-dirty, tests/docs/bare_bones_cmake fails due to no cmake_minimum_required() command being present. This is not related to the changes made here (CMake 4 requires a minimum version now; see CMP0000), so I vote to move forward with merging these changes regardless of the failing test. I'll move this discovery to a separate issue.

Just to be clear, while this removes a blocker, I still do not consider this PR r2g yet until I have a chance to test the integration tests as well.

@zachcran
Copy link
Contributor Author

@ryanmrichard This is r2g. Integration tests passed on my system, as well as all of the other reported tests that I conducted.

@zachcran zachcran requested a review from ryanmrichard June 24, 2025 00:53
@ryanmrichard ryanmrichard merged commit dc8a4f7 into master Jun 24, 2025
7 checks passed
@ryanmrichard ryanmrichard deleted the 166_fix_find_optional_dependencies branch June 24, 2025 03:25
@github-actions
Copy link

🚀 [bumpr] Bumped!
New version:v1.1.7
Changes:v1.1.6...v1.1.7

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.

[BUG] When installed target is found, cmaize_find_or_build_optional_dependency() causes an error

3 participants