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

Migrate CI configs to CMake presets. #324

Merged
merged 68 commits into from
Oct 16, 2023
Merged

Conversation

alliepiper
Copy link
Collaborator

@alliepiper alliepiper commented Aug 11, 2023

Description

Closes #252

MIgrate Thrust and CUB's Github CI config/build/test/ steps to use CMake presets.

@alliepiper alliepiper changed the title Migrate Thrust/CUB CI configs to CMake presets. Migrate CI configs to CMake presets. Aug 18, 2023
@alliepiper alliepiper force-pushed the more-presets branch 2 times, most recently from 7f828ee to a16631d Compare August 19, 2023 12:23
@alliepiper alliepiper marked this pull request as ready for review August 19, 2023 13:25
@alliepiper alliepiper requested review from a team as code owners August 19, 2023 13:25
@alliepiper alliepiper requested review from wmaxey, ericniebler, trxcllnt, robertmaynard, miscco and jrhemstad and removed request for a team August 19, 2023 13:25
CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
@jrhemstad
Copy link
Collaborator

jrhemstad commented Aug 21, 2023

An overarching goal is to push as much logic into the cmake presets or cmake targets as possible. In other words, there should be very little logic in the build scripts themselves because that needs to be replicated across both Linux and Windows build scripts. Ideally, cmake should be the source of truth for everything.

  • This currently breaks how clangd is configured because it expects the compiler commands database to be in build/latest. This is done via a symlink in the build_common.sh. Instead, the symlinking should be done as part of the cmake configure. We should change this symlinking to instead symlink the compile-commands.json to the cccl source dir so that it can work outside of VSCode/devcontainer too.
  • The sccache stats logic from build_common.sh should be made part of the cmake build such that it will always prints sccache stats when you do a build via one of the presets. (Deferred to future work)
  • It would be very nice if we can make sure the cmake presets work outside of the context of a devcontainer if possible, even if at some reduced functionality (e.g., no sccache).
  • Verify that CTEST_PARALLEL_LEVEL doesn't actually make the tests slower
  • To maximize the effectiveness of sccache, we want CI and local devs to build with the same set of architectures whenever possible. Therefore, those should be set in the cmake presets to be the same between local dev/CI.
  • The build directory for the cmake presets should ideally check if running in a devcontainer and set to a unique build directory to avoid conflicts when using multiple containers.

@jrhemstad
Copy link
Collaborator

jrhemstad commented Aug 21, 2023

re: architecture, I remembered that for the CI jobs, that is currently set in the matrix file here:
https://github.com/NVIDIA/cccl/blob/486e55792b4f516a16c58c3d303b7a48e724471e/ci/matrix.yaml#L30C128-L30C134

I want to have a single source of truth whenever possible, so I think we could eliminate the gpu_build_archs from the matrix file and just defer to whatever is set in the preset.

Alternatively, the cmake presets could parse the architecture from the matrix.yaml file (assuming we had a top-level entry for the gpu archs).

@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 30, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@alliepiper alliepiper requested review from a team as code owners August 30, 2023 16:20
@alliepiper alliepiper requested review from elstehle and removed request for a team August 30, 2023 16:20
.github/workflows/build-cccl-infra.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
ci/build_common.sh Outdated Show resolved Hide resolved
We were destructively parsing the CLI args in build_common,
but the test scripts needed those to persist so they could
be passed to the build scripts after sourcing build_common.sh.

This was causing the build/test scripts to execute with
different parameters.
The usage of the atomics was removed in a previous
commit, this removes the header to unblock the
cl builds on sm60.
@jrhemstad
Copy link
Collaborator

Can we also update the contributing guide section that describes how to use the scripts: https://github.com/NVIDIA/cccl/blob/main/CONTRIBUTING.md#building-and-testing

# Prepare environment for CMake:
export CMAKE_BUILD_PARALLEL_LEVEL="${PARALLEL_LEVEL}"
export CTEST_PARALLEL_LEVEL="1"
export CXX="${HOST_COMPILER}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

question (for self): Does this overwrite the CXX envvar after running the script?

@alliepiper alliepiper dismissed robertmaynard’s stale review October 12, 2023 13:49

Addressed requested changes

@alliepiper
Copy link
Collaborator Author

This is ready for another round of reviews.

@alliepiper alliepiper merged commit a8971b4 into NVIDIA:main Oct 16, 2023
463 checks passed
@alliepiper alliepiper deleted the more-presets branch October 16, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEA]: Setup CMake presets for common build configurations
5 participants