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

CMake updates #1181

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

CMake updates #1181

wants to merge 55 commits into from

Conversation

victorapm
Copy link
Contributor

@victorapm victorapm commented Nov 9, 2024

  • Finalized implementation of HIP support in CMake build
  • Achieve feature parity between CMake and Autotools builds
  • Upgrade CMake minimum required version to 3.21 (required by HIP)
  • Update CMake to use modern keywords and best practices
  • Better status messaging when running cmake
  • Add HYPRE_SetupGPUToolkit.cmake to manage GPU options in cmake build
  • Refactor top-level CMakeLists and moved helper functions to HYPRE_CMakeUtilities.cmake, e.g., setup_git_version_info for configuring and displaying hypre version using git version info
  • Add SOVERSION field to CMake build
  • Enable configuration of third-party libraries (TPLs) via CMake module
  • Add summary table of options used to configure hypre in CMake build
  • Remove HYPRE_INSTALL_PATH in favor of the commonly used CMAKE_INSTALL_PATH
  • Remove HYPRE_BUILD_TYPE in favor of the commonly used CMAKE_BUILD_TYPE
  • Remove HYPRE_ENABLE_SHARED in favor of the commonly used BUILD_SHARED_LIBS
  • Update documentation to reflect mainly CMake changes + a few fixes
  • Add CMake tests to machine-tioga
  • Add CTest support

Closes #1104
Closes #673
Closes #1084
Closes #1073
Closes #1072
Closes #1039
Closes #907
Closes #767
Closes #757
Closes #771
Closes #501
Closes #473
Closes #228
Closes #928

victorapm and others added 30 commits September 12, 2024 22:51
if (${HYPRE_SOURCE_DIR} STREQUAL ${HYPRE_BINARY_DIR})
message(FATAL_ERROR "In-place build not allowed! Please use a separate build directory. See the Users Manual or INSTALL file for details.")
endif ()

if (EXISTS ${HYPRE_SOURCE_DIR}/../.git)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? I think this has to do with the version check stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #472

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Rob, I moved this check to inside the new macro:

if (EXISTS "${HYPRE_GIT_DIR}")

@rfalgout
Copy link
Contributor

Hi @victorapm . I upgraded my version of cmake, but when I do cmake .. in the src/cmbuild directory I get an error:

Could NOT find BLAS (missing: BLAS_LIBRARIES)

We shouldn't require the blas to build hypre. Let me know what I should do. Thanks!

@rfalgout
Copy link
Contributor

BTW, I am starting to think we should remove the README.txt file from cmbuild. I often like to just remove everything and start over with a new cmake .. since hypre is so quick to compile. Also, the caching can be tricky. What do you think?

@victorapm
Copy link
Contributor Author

We shouldn't require the blas to build hypre

Hi Rob, thanks for catching this. I fixed a typo in CMakeLists and this should work as expected now. Thanks!

I am starting to think we should remove the README.txt

I agree we should do this for the same reason you brought up

@rfalgout
Copy link
Contributor

Hi @victorapm . I still need to review this, but I started to think a bit more about using cmake as my default build option when writing code. It's a bit cumbersome to use right out of the box right now, so I thought we could discuss this more at our next meeting. I added an agenda item. Also, I added a draft idea in commit 9179d21 that copies the executables (in test or examples) to the original source directory to aid with testing. We can chat about this too.

@victorapm
Copy link
Contributor Author

Sounds great, thanks Rob!

@victorapm
Copy link
Contributor Author

victorapm commented Nov 21, 2024

All, I've added the remaining options to the cmake build (including specific debug flags according to compilers), updated documentation, and changed WITH options to ENABLE

@rfalgout
Copy link
Contributor

All, I've added the remaining options to the cmake build (including specific debug flags according to compilers), updated documentation, and changed WITH options to ENABLE

Thanks, @victorapm ! FYI, one thing I noticed regarding the compiler options is that they are not used in the test directory, so -Wall warnings and such don't get caught for the drivers. I assume this is something easy to fix?

.. code-block:: bash

cd src/AUTOTEST
./machine_name.sh ../src
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ./test.sh machine_name.sh ../src . However, I'm not sure we should be documenting this in the user manual. There is a README.txt in the AUTOTEST directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I added info about AUTOTEST just for completeness in that section. Let me know if you think it's better to remove it, or just point to the README.txt in the AUTOTEST folder

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep some very minimal info here and point to the README.txt file for details. That will give us lots of flexibility to make modifications to AUTOTEST without having to touch the online documentation (updating the README.txt and -help information in the scripts instead).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Rob! Just updated this on 0055eeb

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks!

@victorapm
Copy link
Contributor Author

-Wall warnings and such don't get caught for the drivers

Hi Rob, this should be fixed now, thanks for catching!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants