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

Apply CI compiler warning flags in CMake #1809

Closed
wants to merge 5 commits into from

Conversation

cwpearson
Copy link
Contributor

This makes the warning flags we use in CI directly accessible in development with -DKokkosKernels_ENABLE_COMPILER_WARNINGS=ON

@@ -824,6 +824,6 @@ cd $STORE_KOKKOSKERNELS_BUILD_PATH

# Configure kokkos-kernels
echo ""
echo cmake $COMPILER_CMD -DKokkos_DIR="${KOKKOS_FIND_PATH}" -DCMAKE_CXX_FLAGS=\"${KOKKOS_CXXFLAGS}\" -DCMAKE_INSTALL_PREFIX="${PREFIX}" -DKokkosKernels_ENABLE_TESTS_AND_PERFSUITE=${KOKKOSKERNELS_DO_PERFSUITE} -DKokkosKernels_ENABLE_TESTS=${KOKKOSKERNELS_DO_TESTS} -DKokkosKernels_ENABLE_PERFTESTS=${KOKKOSKERNELS_DO_PERFTESTS} -DKokkosKernels_ENABLE_EXAMPLES:BOOL=${KOKKOSKERNELS_DO_EXAMPLES} -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=${CMAKE_EXPORT_COMPILE_COMMANDS} ${KOKKOSKERNELS_SCALARS_CMD} ${KOKKOSKERNELS_ORDINALS_CMD} ${KOKKOSKERNELS_OFFSETS_CMD} ${KOKKOSKERNELS_LAYOUTS_CMD} ${KOKKOSKERNELS_TPLS_CMD} ${KOKKOSKERNELS_USER_TPL_PATH_CMD} ${KOKKOSKERNELS_USER_TPL_LIBNAME_CMD} -DCMAKE_EXE_LINKER_FLAGS=\"${KOKKOSKERNELS_EXTRA_LINKER_FLAGS_PARSED}\" ${KOKKOSKERNELS_BUILDTYPE_CMD} -DBUILD_SHARED_LIBS=${BUILD_SHARED_LIBRARIES} ${KOKKOSKERNELS_COMPONENTS_CMD} ${KOKKOSKERNELS_SPACES_CMD} ${KERNELS_DEFAULT_ETI_OPTION} ${KERNELS_DEFAULT_TEST_ETI_ONLY_OPTION} -DKokkosKernels_ENABLE_DOCS=${KOKKOSKERNELS_DO_DOCS} ${KOKKOSKERNELS_PATH}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is hard to read - it just adds -DKokkosKernels_ENABLE_COMPILER_WARNINGS=ON to each line

@cwpearson
Copy link
Contributor Author

Thoughts on this general approach @e10harvey @lucbv?

@@ -819,40 +805,38 @@ elif [ "$MACHINE" = "kokkos-dev-2" ]; then
BUILD_LIST_CUDA_NVCC="Cuda_Serial,Cuda_Threads"
BUILD_LIST_CLANG="Serial,Threads,OpenMP"

CLANG8_CUDA_WARNING_FLAGS="-Wall,-Wshadow,-pedantic,-Werror,-Wsign-compare,-Wtype-limits,-Wuninitialized,-Wno-pass-failed"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we support this anymore, so I didn't try to detect this scenario

Copy link
Contributor

@lucbv lucbv left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.
At some point we might actually want to check that the current compiler used is the same as the one used to compile Kokkos and at least warn users if they are different?

Copy link
Contributor

@e10harvey e10harvey left a comment

Choose a reason for hiding this comment

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

@cwpearson: Before merging, please add a documentation block to the top of cm_test_all_sandia noting to maintainers where compiler flags get set now. Please also share a diff CMakeCache_before_these_changes.txt CMakeCache_after_these_changes.txt for all configurations that we care about.

@e10harvey
Copy link
Contributor

Thoughts on this general approach @e10harvey @lucbv?

This approach looks reasonable to me. FWIW, this is a similar divergence that was used in trilinos (branching in both shell and cmake scripts for different systems and compilers) that led to significant complexity and maintainability challenges. cm_test_all_sandia has kept this branching and complexity mostly within the script itself, which, in my opinion, has worked very well.

These configuration use-cases are exactly what GenConfig was designed to address. I do not think this PR introduces significant complexity and maintainability challenges. If managing this configuration infrastructure becomes a pain point, please consider addressing it.

@cwpearson
Copy link
Contributor Author

Converting this to an issue to possibly revisit later.

@cwpearson cwpearson closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants