Skip to content

Standardize package found logic for tpls #429

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
bartgol wants to merge 4 commits into
base: master
Choose a base branch
from
+90 −30
Open

Standardize package found logic for tpls #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58d1906
Standardize package found logic for tpls
bartgol Apr 10, 2026
3dde409
Ensure yaml-cpp scoped target exists for downstream customers
bartgol Apr 10, 2026
4430bbc
Ensure spdlog/Catch2 scoped target exists, like we do for yaml-cpp
bartgol Apr 10, 2026
9b088bb
Make TPLs handling extra-extra-safe
bartgol Apr 11, 2026
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
48 changes: 34 additions & 14 deletions cmake/EkatConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,35 @@ endif()

# yaml-cpp
if (@EKAT_ENABLE_YAML_PARSER@)
if (@EKAT_BUILDS_YAMLCPP@)
# We're installing yaml-cpp alongside ekat
find_dependency(yaml-cpp REQUIRED QUIET HINTS @CMAKE_INSTALL_PREFIX@)
else()
find_dependency(yaml-cpp REQUIRED QUIET HINTS @yaml-cpp_DIR@)
if (NOT TARGET yaml-cpp::yaml-cpp AND NOT TARGET yaml-cpp)
if (@EKAT_BUILDS_YAMLCPP@)
# We're installing yaml-cpp alongside ekat
find_dependency(yaml-cpp REQUIRED QUIET HINTS @CMAKE_INSTALL_PREFIX@)
else()
find_dependency(yaml-cpp REQUIRED QUIET HINTS @yaml-cpp_DIR@)
endif()
endif()

# Ensure the namespaced target exists for downstream consumers even if we created it on the fly
if (TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp)
add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp)
endif()
endif()

if (@EKAT_ENABLE_LOGGING@)
# spdlog
if (@EKAT_BUILDS_SPDLOG@)
# We're installing yaml-cpp alongside ekat
find_dependency(spdlog REQUIRED QUIET HINTS @CMAKE_INSTALL_PREFIX@)
else()
find_dependency(spdlog REQUIRED QUIET HINTS @spdlog_DIR@)
if (NOT TARGET spdlog::spdlog AND NOT TARGET spdlog)
if (@EKAT_BUILDS_SPDLOG@)
# We're installing spdlog alongside ekat
find_dependency(spdlog REQUIRED QUIET HINTS @CMAKE_INSTALL_PREFIX@)
else()
find_dependency(spdlog REQUIRED QUIET HINTS @spdlog_DIR@)
endif()
endif()

# Ensure the namespaced target exists for downstream consumers even if we created it on the fly
if (TARGET spdlog AND NOT TARGET spdlog::spdlog)
add_library(spdlog::spdlog ALIAS spdlog)
endif()
endif()

Expand All @@ -51,10 +65,16 @@ if (@EKAT_ENABLE_BOOST_STACKTRACE@)
endif()

# Catch2 testing system
if (@EKAT_BUILDS_CATCH2@)
find_dependency (Catch2 REQUIRED QUIET HINTS @CMAKE_INSTALL_PREFIX@)
else()
find_dependency (Catch2 REQUIRED QUIET HINTS @Catch2_DIR@)
if (NOT TARGET Catch2::Catch2 AND NOT TARGET Catch2)
if (@EKAT_BUILDS_CATCH2@)
find_dependency (Catch2 REQUIRED QUIET HINTS @CMAKE_INSTALL_PREFIX@)
else()
find_dependency (Catch2 REQUIRED QUIET HINTS @Catch2_DIR@)
endif()
endif()
# Ensure the namespaced target exists for downstream consumers even if we created it on the fly
if (TARGET Catch2 AND NOT TARGET Catch2::Catch2)
add_library(Catch2::Catch2 ALIAS Catch2)
endif()

include (${CMAKE_CURRENT_LIST_DIR}/EkatTargets.cmake)
Expand Down
27 changes: 23 additions & 4 deletions src/logging/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,39 @@
include(GNUInstallDirs)

# spdlog is a REQUIRED dep of this package

set (spdlog_FOUND FALSE)
if (TARGET spdlog::spdlog OR TARGET spdlog)
set(spdlog_FOUND TRUE)
message(STATUS "EKAT: Using spdlog provided by parent project")
endif()

option (EKAT_SKIP_FIND_SPDLOG "Skip find_package for spdlog, and fetch via FetchContent" OFF)
if (NOT EKAT_SKIP_FIND_SPDLOG)
if (NOT spdlog_FOUND AND NOT EKAT_SKIP_FIND_SPDLOG)
message (STATUS "Looking for spdlog ...")
find_package(spdlog QUIET NO_DEFAULT_PATH)
if (spdlog_FOUND)
message (STATUS "Looking for spdlog ... FOUND")
message (STATUS " spdlog_DIR: ${spdlog_DIR}")
# It is possible that the installation provides the spdlog target, but not the spdlog::spdlog target. If so, define the alias target
if (NOT TARGET spdlog::spdlog)
if (NOT TARGET spdlog)
message (FATAL_ERROR "Neither spdlog nor spdlog::spdlog targets were found")
endif()
add_library (spdlog::spdlog ALIAS spdlog)
endif()
else()
message (STATUS "Looking for spdlog ... NOT FOUND")
endif()
endif()

if (NOT TARGET spdlog)
# If we found spdlog (via find_package or as an existing tgt)
# ensure it provides the scoped target
if (spdlog_FOUND)
if (TARGET spdlog AND NOT TARGET spdlog::spdlog)
add_library(spdlog::spdlog ALIAS spdlog)
endif()
endif()

if (NOT spdlog_FOUND)
include(EkatBuildSpdlog)
set (EKAT_BUILDS_SPDLOG ON CACHE BOOL "Whether Ekat builds spdlog" FORCE)
else()
Expand Down
25 changes: 17 additions & 8 deletions src/parser/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
# yaml-cpp is a REQUIRED dep of this package

set (yaml-cpp_FOUND FALSE)
if (TARGET yaml-cpp::yaml-cpp OR TARGET yaml-cpp)
set(yaml-cpp_FOUND TRUE)
message(STATUS "EKAT: Using yaml-cpp provided by parent project")
endif()

option (EKAT_SKIP_FIND_YAML_CPP
"Skip find_package for yaml-cpp, and fetch via FetchContent" OFF)
if (NOT EKAT_SKIP_FIND_YAML_CPP)
if (NOT yaml-cpp_FOUND AND NOT EKAT_SKIP_FIND_YAML_CPP)
# I am having issues getting the env var YAML_CPP_ROOT being picked up
# by cmake. I suspect this has to do with the presence of the hyphen
# CMake *should* convert '-' to '_' when forming the cmake/env var name,
Expand All @@ -17,19 +23,22 @@ if (NOT EKAT_SKIP_FIND_YAML_CPP)
if (yaml-cpp_FOUND)
message (STATUS "Looking for yaml-cpp ... FOUND")
message (STATUS " yaml-cpp_DIR: ${yaml-cpp_DIR}")
# It is possible that the installation provides the yaml-cpp target, but not the yaml-cpp::yaml-cpp target. If so, define the alias target
if (NOT TARGET yaml-cpp::yaml-cpp)
if (NOT TARGET yaml-cpp)
message (FATAL_ERROR "Neither yaml-cpp nor yaml-cpp::yaml-cpp targets were found")
endif()
add_library (yaml-cpp::yaml-cpp ALIAS yaml-cpp)
if (NOT TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp)
endif()
else()
message (STATUS "Looking for yaml-cpp ... NOT FOUND")
endif()
endif()

if (NOT TARGET yaml-cpp)
# If we found yaml-cpp (via find_package or as an existing tgt)
# ensure it provides the scoped target
if (yaml-cpp_FOUND)
if (TARGET yaml-cpp AND NOT TARGET yaml-cpp::yaml-cpp)
add_library(yaml-cpp::yaml-cpp ALIAS yaml-cpp)
endif()
Comment on lines +33 to +38
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

yaml-cpp_FOUND can be TRUE even if neither yaml-cpp::yaml-cpp nor yaml-cpp targets exist (some installs set *_FOUND but don't export targets). In that case, the build path is skipped but ekat_yamlparser still links to yaml-cpp::yaml-cpp, which will fail at configure/generate time. Please add an explicit check: if yaml-cpp_FOUND is TRUE and the targets are missing, either create the required imported target or raise a FATAL_ERROR (or set yaml-cpp_FOUND back to FALSE to fall back to FetchContent).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@bartgol bartgol Apr 11, 2026

Choose a reason for hiding this comment

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

This is starting to look a lot like having to write our own FindYamlCpp.cmake... Gemini seems to agree with me:

Copilot is technically correct, but it’s suffering from "Perfectionist's Bloat."
In the world of CMake, there is a point of diminishing returns where you transition from "robust build system" to "writing a full-blown package manager." Copilot wants you to reach the latter.

The current impl would work for 99.99% of the cases. I think we're safe enough for now. If someone loses an hour of work b/c this issue fires back, I'll gladly buy them a few rounds of drinks to make up for it.

endif()

if (NOT yaml-cpp_FOUND)
include(EkatBuildYamlCpp)
set (EKAT_BUILDS_YAMLCPP ON CACHE BOOL "Whether Ekat builds yaml-cpp" FORCE)
else()
Expand Down
20 changes: 16 additions & 4 deletions src/testing-support/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
include(GNUInstallDirs)

### CATCH MAIN ###

set (Catch2_FOUND FALSE)
if (TARGET Catch2::Catch2 OR TARGET Catch2)
set(Catch2_FOUND TRUE)
message(STATUS "EKAT: Using Catch2 provided by parent project")
endif()

# Catch2 is a REQUIRED dep of this package
option (EKAT_SKIP_FIND_CATCH2 "Skip find_package for Catch2, and fetch via FetchContent" OFF)
if (NOT EKAT_SKIP_FIND_CATCH2)
if (NOT Catch2_FOUND AND NOT EKAT_SKIP_FIND_CATCH2)
message (STATUS "Looking for Catch2 ...")
find_package(Catch2 QUIET NO_DEFAULT_PATH)
if (Catch2_FOUND)
Expand All @@ -15,7 +19,15 @@ if (NOT EKAT_SKIP_FIND_CATCH2)
endif()
endif()

if (NOT TARGET Catch2::Catch2)
# If we found Catch2 (via find_package or as an existing tgt)
# ensure it provides the scoped target
if (Catch2_FOUND)
if (TARGET Catch2 AND NOT TARGET Catch2::Catch2)
add_library(Catch2::Catch2 ALIAS Catch2)
endif()
endif()

if (NOT Catch2_FOUND)
set (EKAT_BUILDS_CATCH2 ON CACHE BOOL "Whether Ekat builds Catch2" FORCE)
include (EkatBuildCatch2)
else()
Expand Down
Loading