-
Notifications
You must be signed in to change notification settings - Fork 51
Clang Tidy and Sanitizer: Use clang-19 on Ubuntu 24.04 #1783
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
Clang Tidy and Sanitizer: Use clang-19 on Ubuntu 24.04 #1783
Conversation
does not work
f833106
to
58fbcad
Compare
58fbcad
to
1d41d46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two notes below for review @ax3l
|
||
mirrors: | ||
E4S: https://cache.e4s.io | ||
spack-public: https://mirror.spack.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the change of mirror URL, cache.e4s.io is outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, perfect! :)
share/openPMD/download_samples.sh build | ||
export LDFLAGS="${LDFLAGS} -fsanitize=address,undefined -shared-libsan" | ||
export CXXFLAGS="${CXXFLAGS} -fsanitize=address,undefined -shared-libsan" | ||
export CXXFLAGS="${CXXFLAGS} -fsanitize=address,undefined -shared-libsan -DOMPI_SKIP_MPICXX" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check -DOMPI_SKIP_MPICXX
. For some reason, this run pulls in the C++ bindings of MPI and then fails. This can be avoided with this preprocessor define, but there might be a better solution. I see that we have some finicky config in CMakeLists.txt
on whether we link with MPI::MPI_C
or MPI::MPI_CXX
. Changing one to the other made no difference in my tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found in the past that surprisingly, the C API of MPI sometimes depends on the (outdated) C++ bindings (e.g., BullMPI, which is an OpenMPI variant)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need C CXX
but not MPICXX
https://cmake.org/cmake/help/latest/module/FindMPI.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a CMake option MPI_CXX_SKIP_MPICXX
Add some definitions that will disable the MPI-2 C++ bindings. Currently supported are MPICH, Open MPI, Platform MPI and derivatives thereof, for example MVAPICH or Intel MPI.
Let's add that! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will do that in a follow-up PR, so we can patch it out if it causes issues after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, splendid! 🚀 ✨
Uh oh!
There was an error while loading. Please reload this page.