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

Warn at configuration time if attempting to disable desul atomics and force using it #5801

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jan 24, 2023

Alternative to #5790 where we warn and just ignore the user setting instead of erroring out

Yields both

CMake Warning at cmake/kokkos_enable_options.cmake:167 (MESSAGE):
  Kokkos_ENABLE_IMPL_DESUL_ATOMICS option has been removed.  Desul atomics
  cannot be disabled.
Call Stack (most recent call first):
  cmake/kokkos_tribits.cmake:242 (INCLUDE)
  CMakeLists.txt:213 (KOKKOS_SETUP_BUILD_ENVIRONMENT)

<snip>

CMake Warning:
  Manually-specified variables were not used by the project:

    Kokkos_ENABLE_IMPL_DESUL_ATOMICS

EDIT Does not raise the Manually-specified variables were not used warning with the 2nd commit.
But I think this is good enough for now. In follow-up PRs will remove these variables and the warning will be back.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

I prefer warning slightly. Would have been ok with just removing the option and have CMake warn about unused variable.

Copy link
Contributor

@cz4rs cz4rs left a comment

Choose a reason for hiding this comment

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

No strong preference between the two approaches. Looks good from the technical side.

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.

👍

@dalg24 dalg24 merged commit cb67caf into kokkos:develop Jan 24, 2023
@dalg24 dalg24 deleted the require_desul_atomics_v2 branch January 24, 2023 17:30
@dalg24
Copy link
Member Author

dalg24 commented Jan 25, 2023

Was step 1 in #5789

@dalg24 dalg24 mentioned this pull request Mar 28, 2023
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