Skip to content
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

Hardening internal fmt/camp TPL install config cases #870

Merged
merged 10 commits into from
Feb 16, 2024
Merged

Hardening internal fmt/camp TPL install config cases #870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
055561c
adding fmt_DIR location in tpl CMakeLists
kab163 Feb 15, 2024
74ac114
Update CMakeLists.txt
kab163 Feb 15, 2024
ead02b4
make fmt_DIR a cache variable
white238 Feb 15, 2024
7f94e67
add to install test to use fmt and check for target
white238 Feb 15, 2024
02b6193
Apply style updates
Feb 15, 2024
baefc0f
Trying to trigger test
kab163 Feb 15, 2024
88a4b35
save local variable to prefix
white238 Feb 15, 2024
19c4c77
set the camp_DIR as well and add comments explaining
white238 Feb 15, 2024
c41d7e2
turning off cuda test until docker fix is available
kab163 Feb 16, 2024
5d0ebe7
turningg off nvcc10 test
kab163 Feb 16, 2024
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
2 changes: 0 additions & 2 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ jobs:
docker_target: clang11
clang12:
docker_target: clang12
nvcc10:
docker_target: nvcc10
sycl:
docker_target: sycl
hip:
Expand Down
4 changes: 4 additions & 0 deletions src/tpl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ if (NOT TARGET camp)
)
set_target_properties(camp PROPERTIES IMPORTED_GLOBAL TRUE)
else ()
# In this case, camp will be installed inside of Umpire install prefix
set(camp_DIR ${CMAKE_INSTALL_PREFIX} CACHE PATH "")
add_subdirectory(umpire/camp)
endif()

Expand Down Expand Up @@ -147,6 +149,8 @@ if (NOT TARGET fmt::fmt)
if (NOT EXISTS ${PROJECT_SOURCE_DIR}/src/tpl/umpire/fmt/CMakeLists.txt)
message(FATAL_ERROR "fmt submodule not initialized. Run 'git submodule update --init --recursive' in the git repository or set fmt_DIR to use an external build of fmt.")
else ()
# In this case, fmt will be installed inside of Umpire install prefix
set(fmt_DIR ${CMAKE_INSTALL_PREFIX} CACHE PATH "")
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, fmt_DIR is used here: https://github.com/LLNL/Umpire/blob/develop/umpire-config.cmake.in#L37

When using an external fmt the call to find_package will set fmt_DIR for us, which is searched. However, I'm confused why this doesn't work since PACKAGE_CMAKE_INSTALL_PREFIX should be the same as CMAKE_INSTALL_PREFIX: https://github.com/LLNL/Umpire/blob/develop/umpire-config.cmake.in#L45

Copy link
Member

Choose a reason for hiding this comment

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

hmm... that is an excellent point. Also explains why I couldn't reproduce the original issue. I will need to see if I can force this to fail..

man @adayton1 why must you rain on my parade.

Copy link
Member

Choose a reason for hiding this comment

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

☔ 🌧️

set(FMT_INSTALL ON)
set(FMT_SYSTEM_HEADERS ON)
add_subdirectory(umpire/fmt)
Expand Down
7 changes: 6 additions & 1 deletion tests/install/using-with-cmake/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,10 @@ find_package(umpire REQUIRED
NO_DEFAULT_PATH
PATHS ${umpire_DIR})

# Check if fmt::fmt was imported from above find_package
if(NOT TARGET fmt::fmt)
message(FATAL_ERROR "fmt targets were not imported from installed config files.")
endif()

add_executable(using-with-cmake using-with-cmake.cpp)
target_link_libraries(using-with-cmake umpire)
target_link_libraries(using-with-cmake umpire fmt::fmt)
3 changes: 2 additions & 1 deletion tests/install/using-with-cmake/using-with-cmake.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
//////////////////////////////////////////////////////////////////////////////
#include <iostream>

#include "fmt/format.h"
#include "umpire/Allocator.hpp"
#include "umpire/ResourceManager.hpp"

int main()
{
auto& rm = umpire::ResourceManager::getInstance();
umpire::Allocator alloc = rm.getAllocator("HOST");
std::cout << "Got allocator: " << alloc.getName() << std::endl;
std::cout << fmt::format("Got allocator: {0}", alloc.getName()) << std::endl;

std::cout << "Available allocators: ";
for (auto s : rm.getAllocatorNames()) {
Expand Down
13 changes: 8 additions & 5 deletions umpire-config.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@

include(CMakeFindDependencyMacro)

# cache the prefix dir (could be overriden by find_dependency)
set(UMPIRE_PACKAGE_PREFIX_DIR ${PACKAGE_PREFIX_DIR})

if (@UMPIRE_NEEDS_BLT_TPLS@)
set(BLT_TGTS "${CMAKE_CURRENT_LIST_DIR}/bltTargets.cmake")
if(EXISTS "${BLT_TGTS}")
include("${BLT_TGTS}")
include("${BLT_TGTS}")
endif()
unset(BLT_TGTS)
endif()
Expand All @@ -29,8 +32,8 @@ if (NOT TARGET camp)
find_dependency(camp CONFIG NO_DEFAULT_PATH PATHS
${camp_DIR}
${camp_DIR}/lib/cmake/camp
@PACKAGE_CMAKE_INSTALL_PREFIX@
@PACKAGE_CMAKE_INSTALL_PREFIX@/lib/cmake/camp)
${UMPIRE_PACKAGE_PREFIX_DIR}
${UMPIRE_PACKAGE_PREFIX_DIR}/lib/cmake/camp)
endif ()

if (NOT TARGET fmt::fmt)
Expand All @@ -42,8 +45,8 @@ if (NOT TARGET fmt::fmt)
find_dependency(fmt CONFIG NO_DEFAULT_PATH PATHS
${fmt_DIR}
${fmt_DIR}/lib64/cmake/fmt
@PACKAGE_CMAKE_INSTALL_PREFIX@
@PACKAGE_CMAKE_INSTALL_PREFIX@/lib64/cmake/fmt)
${UMPIRE_PACKAGE_PREFIX_DIR}
${UMPIRE_PACKAGE_PREFIX_DIR}/lib64/cmake/fmt)
endif ()

if (@UMPIRE_ENABLE_SQLITE_EXPERIMENTAL@)
Expand Down
Loading