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

Adding CMake Presets #1536

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from
Open

Adding CMake Presets #1536

wants to merge 18 commits into from

Conversation

MarcelKoch
Copy link
Member

@MarcelKoch MarcelKoch commented Jan 19, 2024

This PR can be used to discuss the addition of CMake presets. So far, I've added only very basic presets which are compatible with the current workflows.

But these preset can be used to set common standards, especially regarding warnings. ATM, we only enforce the -Wpedantic warnings for our development builds. We could use the presets to define a more rigorous set of warnings.

There might also be other options we might want to set here, this is only a starting point for a discussion.

The workflow using the presets would be as follows:

cd ginkgo
cmake . --presets develop [cmake options...]
cmake --build build-develop

Also, the presets are usually picked up by IDEs.

@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:documentation This is related to documentation. labels Jan 19, 2024
{
"name": "develop",
"inherits": "base",
"cacheVariables": {
Copy link
Member

Choose a reason for hiding this comment

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

From our previous discussions, I would suggest adding CMAKE_EXPORT_COMPILE_COMMANDS=ON, CMAKE_COLOR_DIAGNOSTICS=ON, GINKGO_DEVEL_TOOLS=ON, CMAKE_LINK_DEPENDS_NO_SHARED=ON

@upsj
Copy link
Member

upsj commented Jan 19, 2024

Another useful feature might be the environment object, with which we can configure path-independent CXX and CUDACXX compiler executable names

"inherits": "base",
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"CMAKE_CXX_FLAGS": "$env{CXXFLAGS} -Wpedantic"
Copy link
Member Author

Choose a reason for hiding this comment

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

In terms of warnings I was thinking of

-Wall -Wextra -Wconversion -Wunused

@MarcelKoch
Copy link
Member Author

Another useful feature might be the environment object, with which we can configure path-independent CXX and CUDACXX compiler executable names

Are you meaning something like CXX=g++? I think that would make sense if we would extend the presets to be used in our CI, but otherwise I'm not sure of the benefit of that.

@upsj
Copy link
Member

upsj commented Jan 19, 2024

I could imagine develop-clang being useful from time to time, especially with its better diagnostics

@greole
Copy link
Collaborator

greole commented Jan 21, 2024

I guess that should also work with cmakes new --workflow option.

{
"name": "base",
"hidden": true,
"generator": "Ninja",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to require Ninja as dependency? Often needs a separate module to be loaded and is not available on some systems.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use presets version < 3, then we need to specify a generator. But even if we don't need to specify it, I would still prefer to set Ninja, since it seems to handle dependencies better than make, and it's not as verbose. It's always possible to overwrite the settings from the preset, by using the normal CLI arguments.

@@ -113,26 +113,6 @@ macro(ginkgo_modify_flags name)
string(REPLACE "\"" "\\\"" ${name}_MODIFY "${${name}}")
endmacro()

# Extract the clang version from a clang executable path
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is superseded by CMAKE_CXX_COMPILER_VERSION?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but this function was never used.

@pratikvn
Copy link
Member

Can we extend this approach to enable builds for HPC systems ? Frontier, Horeka etc.

@upsj
Copy link
Member

upsj commented Jan 22, 2024

@pratikvn I think the issue there is that they depend on loaded modules, and the executable names (gcc etc) are the same otherwise, so the configs may not help and differ that much between different systems.

We could consider adding a '-march=native` preset that also includes other optimizations (mixed precision, maybe jacobi full optimizations)

@MarcelKoch
Copy link
Member Author

I tend to agree that adding supported machines may clutter the file a bit too much. But it is possible to include configurations from other files, so perhaps we could put these configs into a separate file.

}
},
{
"name": "ci-cuda-msvc-nompi-release",
Copy link
Member Author

Choose a reason for hiding this comment

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

I chose the naming scheme ci-[device (multiple connected with +)]-[toolchain]-[nompi|mpi]-[build_type][optional configs]. Should this be kept?
Also, in our ci we seem to use / as a separator instead of -. We could do the same here, with the side effect, that we would create nested folders.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was just a convention we used when building our containers. IMO using - seems reasonable. Does having nested folders have any advantages ?

Copy link
Member

@upsj upsj Jan 25, 2024

Choose a reason for hiding this comment

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

The only impact (not sure if it's an advantage) is that the directories used by the Gitlab runner share the same nested structure.

Copy link
Member 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 there would be a difference, it's just preference. Usually you would only use one CI preset anyway.

@MarcelKoch MarcelKoch self-assigned this Jan 26, 2024
@MarcelKoch MarcelKoch force-pushed the cmake-presets branch 5 times, most recently from 9e4ac1d to e996ec5 Compare January 30, 2024 16:54
"rhs": "Windows"
}
}
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it makes sense to also add buildPresets like

Suggested change
]
],
"buildPresets": [
{
"name": "develop-gcc",
"configurePreset": "develop-gcc"
}
]

that way one can build with cmake --build --preset develop-gcc without knowing the exact binaryDir

Copy link
Member Author

Choose a reason for hiding this comment

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

For the 'normal' configuration I can see this making sense. But for the CI configs it would probably be too much.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think buildPresets make especially sense in combination with the workflow option. So that a user can configure and build a preset with a single command. However, workflows are only available for version 6 and above, which needs cmake 3.25.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just another thought on the buildPresets, we could also add a format preset with a targets
key

"targets": [
       "format",
     ]

However, I don't know how to then just pick the "current" binaryDir via the preset.

Copy link
Member

@upsj upsj Apr 4, 2024

Choose a reason for hiding this comment

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

I think we'll probably move away from that with pre-commit handling the formatting entirely? Also make format or ninja format is not really more complicated than calling it via preset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing I like about presets is that it allows you to run cmake --build --preset format from the source directory without knowing the corresponding build directory. However, you are right that moving towards precommit makes it unnecessary.

{
"name": "base",
"hidden": true,
"binaryDir": "${sourceDir}/build-${presetName}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

My personal preference would be nesting the build folder and adding buildPresets. So that there won't be multiple build- folders in the sourceDir

Suggested change
"binaryDir": "${sourceDir}/build-${presetName}"
"binaryDir": "${sourceDir}/build/${presetName}"

Copy link
Member Author

Choose a reason for hiding this comment

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

I would agree. Although typically there wouldn't be too many different build dirs anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about it, I usually build in a build/<branchName> directory to keep different builds separated. But I guess that information is not available for CMakePresets.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but it's always possible to overwrite this from the CLI

Copy link
Member Author

Choose a reason for hiding this comment

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

If you really want to go for build/<branchName>, it might be possible to realize it via git hooks.

@MarcelKoch MarcelKoch added this to the Ginkgo 1.8.0 milestone Apr 5, 2024
Copy link
Collaborator

@greole greole left a comment

Choose a reason for hiding this comment

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

LGTM!

  • I think, however, we should also mention it the presets in the README.md and maybe give an example of how to use presets in combination with normal flags to enable compiling specific backends.
  • And if possible add a release version of the presets.

"name": "base-dev",
"hidden": true,
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nowadays I use presets almost everywhere, thus having a Release version would be also great.

@tcojean tcojean modified the milestones: Ginkgo 1.8.0, Ginkgo 1.9.0 May 31, 2024
The CMake variable is now marked as advanced. There is no incompatibility with other CMake options anymore. The source tuning_variables.cpp is added to the device libs cuda, dpcpp, and hip if the CMake option is set.
Comment on lines 89 to 109
- |
if [ "$(get_cache_val GINKGO_RUN_EXAMPLES)" == "ON" ]; then
export EX_ARG="reference"
ninja run_all_examples
ninja validate_all_examples
if [ "$(get_cache_val GINKGO_BUILD_OMP)" == "ON" ]; then
export EX_ARG="omp"
ninja run_all_examples
ninja validate_all_examples
fi
if [ "$(get_cache_val GINKGO_BUILD_CUDA)" == "ON" ]; then
export EX_ARG="cuda"
ninja run_all_examples
ninja validate_all_examples
fi
if [ "$(get_cache_val GINKGO_BUILD_HIP)" == "ON" ]; then
export EX_ARG="hip"
ninja run_all_examples
ninja validate_all_examples
fi
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

Was there a replacement for this, or do we just not test this anymore?

.gitlab-ci.yml Outdated
Comment on lines 367 to 317
CXX_COMPILER: "clang++"
BUILD_OMP: "ON"
MPI_AS_ROOT: "ON"
BUILD_MPI: "ON"
CXX_FLAGS: "-Wpedantic -D_GLIBCXX_DEBUG=1"
# The tests are prohibitively slow in Debug
BUILD_TYPE: "Release"
CI_CMAKE_PRESET: "ci-omp-clang-mpi-glibcxx+debug-release"
Copy link
Member Author

Choose a reason for hiding this comment

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

does this config even make sense to run with _GLIBCXX_DEBUG in release mode?

@MarcelKoch MarcelKoch marked this pull request as ready for review August 13, 2024 15:28
@MarcelKoch MarcelKoch requested review from upsj, greole and pratikvn August 13, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reg:build This is related to the build system. reg:documentation This is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants