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.

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

Conversation

kab163
Copy link
Contributor

@kab163 kab163 commented Feb 15, 2024

There is an issue when using an internal build of Umpire that the fmt_DIR is not set correctly and users have to point cmake at where Umpire has it's fmt install. To fix that, we have to set the fmt_DIR in the tpl CMakeLists file

white238
white238 previously approved these changes Feb 15, 2024
@@ -147,6 +147,7 @@ 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 ()
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.

☔ 🌧️

@nselliott
Copy link

I tried to test this branch in my code, but I also need the fixes in task/update-blt-tpl-exports for things to work for me.

@kab163
Copy link
Contributor Author

kab163 commented Feb 15, 2024

I tried to test this branch in my code, but I also need the fixes in task/update-blt-tpl-exports for things to work for me.

We could try merging this branch into the blt-tpl-exports branch and then you could test...

adayton1
adayton1 previously approved these changes Feb 15, 2024
Copy link
Member

@adayton1 adayton1 left a comment

Choose a reason for hiding this comment

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

LGTM

@white238 white238 changed the title Setting fmt_DIR location in tpl/CMakeLists Hardening internal fmt/camp TPL install config cases Feb 15, 2024
adayton1
adayton1 previously approved these changes Feb 15, 2024
Copy link
Member

@adayton1 adayton1 left a comment

Choose a reason for hiding this comment

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

LGTM

@kab163
Copy link
Contributor Author

kab163 commented Feb 16, 2024

The nvcc Docker test is failing because "no space left on device".. how can I resolve that?

@rhornung67
Copy link
Member

@kab163 we've had similar issues with RAJA. The cause is that the containers contain a bunch of Spack stuff that is not needed and azure reduced the amount of disk space allowed for free accounts. Chris was working on building a new container repo to address this but I don't know what the status of that is. For RAJA, we turned off nvcc and rocm checks in azure since we are covering those cases in LC GItLab CI.

@kab163 kab163 merged commit af57c78 into develop Feb 16, 2024
28 checks passed
@kab163 kab163 deleted the bugfix/kab163/fix-tpl-fmt-install branch February 16, 2024 20:26
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.

5 participants