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

Enable optional external desul build #5021

Merged
merged 3 commits into from
May 17, 2022
Merged

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented May 12, 2022

No description provided.

@crtrott
Copy link
Member Author

crtrott commented May 12, 2022

Relies on this in order for external build to work: desul/desul#63

@masterleinad
Copy link
Contributor

If you are already moving the bundled desul it would make sense to me to put it into tpls. Also, please let some CI use the external desul.

@dalg24 dalg24 requested a review from nmm0 May 12, 2022 19:48
Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

I agree with @masterleinad in that we should treat it as a TPL if it can be external. Minor question: would it make sense to include it as a subproject (i.e. add_subdirectory'd)?

core/src/CMakeLists.txt Show resolved Hide resolved
core/src/CMakeLists.txt Outdated Show resolved Hide resolved
core/src/CMakeLists.txt Outdated Show resolved Hide resolved
@crtrott
Copy link
Member Author

crtrott commented May 16, 2022

@nmm0 @masterleinad does this look better?

@masterleinad
Copy link
Contributor

@nmm0 @masterleinad does this look better?

Yes, but CI is still missing. 🙂

@crtrott
Copy link
Member Author

crtrott commented May 16, 2022

I am not introducing CI here yet for external desul. We first need to guarantee that we have a stable branch to test against on the desul side, which means we need to introduce CI on the desul side, for which we need the ability to build Kokkos against an external desul ...

KOKKOS_INCLUDE_DIRECTORIES(
${CMAKE_CURRENT_BINARY_DIR}
${CMAKE_CURRENT_SOURCE_DIR}
${KOKKOS_TOP_BUILD_DIR}
)
IF (KOKKOS_ENABLE_IMPL_DESUL_ATOMICS AND NOT desul_atomics_FOUND)
KOKKOS_INCLUDE_DIRECTORIES(
${CMAKE_CURRENT_SOURCE_DIR}/../../tpls/desul/include
Copy link
Member

Choose a reason for hiding this comment

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

We both use relative path from CMAKE_CURRENT_SOURCE_DIR and absolute from CMAKE_SOURCE_DIR below. Is there a reason you can't use CMAKE_SOURCE_DIR here?
Also did you check whether CMAKE_SOURCE_DIR works with Trilinos?

Copy link
Member Author

Choose a reason for hiding this comment

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

CMAKE_SOURCE_DIR didn't even work for in-tree builds, one of the reasons CI was failing.

)
MESSAGE(STATUS "Using internal DESUL atomics copy")
ELSE()
MESSAGE(STATUS "Using external DESUL atomics copy")
Copy link
Member

Choose a reason for hiding this comment

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

Print the path to the package that was found.

Copy link
Member Author

Choose a reason for hiding this comment

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

how?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect you can use desul_atomics_DIR

Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

None yet

4 participants