-
Notifications
You must be signed in to change notification settings - Fork 46
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
rapids-cmake can generate pinned versions file #530
rapids-cmake can generate pinned versions file #530
Conversation
… they are testing
rapids-cmake can now generate a `versions.json` that contains the exact git SHA1 used by depdendencies which can be re-used to construct reproducible builds.
ec84328
to
cbba2a0
Compare
bfd2ffc
to
0d0111e
Compare
testing/cpm/cpm_generate_pins-format-patches/patches/hide_kernels.patch
Outdated
Show resolved
Hide resolved
testing/export/write_language-multiple-nested-enables/CMakeLists.txt
Outdated
Show resolved
Hide resolved
Co-authored-by: Bradley Dice <[email protected]>
…ts.txt Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
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.
Yay! Thanks so much for your hard work and many iterations. This has improved a lot from the initial draft. 🙇♂️
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.
Do we want to make rapids-cpm-init
respect an environment variable/CMake option so that users can do something like cmake ... -DRAPIDS_CMAKE_GENERATE_PINNED_VERSIONS
(name TBD)? Or do we expect that consumers of this feature will modify their calls to rapids_cpm_init
?
I haven't reviewed tests yet, will do that in a second pass after we get through this first round of questions.
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.
My question on the overall review (not tied to any line) from the last review remains outstanding, but I don't think it's super important. The other remaining items are pretty small and we should be good to finish this up in one more round.
# Everything should have shallow marked as false | ||
# so that clones by SHA1 work | ||
if(${proj}_shallow) | ||
message(FATAL_ERROR "${proj}_shallow} is expected to be false, but got ${${proj}_shallow}") | ||
endif() |
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.
Out of scope for this PR, but given the amount of duplication required to write CMake tests it might be good for us to centralize helpers that get used in multiple tests.
/merge |
Modifies pinning tests from #530: * to only test projects that were downloaded by CPM (e.g. ignoring the `fmt` that might already exist in the build environment) * to echo out `pinned_versions.json` and `versions.json` in logs from failed tests, to make debugging faster ## Notes for Reviewers #592 proposes some testing changes that aren't specific to the goals of that PR. Since that PR might be stuck for a bit (rapidsai/build-planning#56 (comment)), this proposes pulling those out into a separate PR: * so that other changes in this project benefit from them * to shrink the diff of #592 and therefore the risk of merge conflicts ### How I tested this Pushed a commit with the new test error message content changes but keeping `fmt` in the failing tests, to confirm that the expected tests failed. <details><summary>got the expected outputs (click me)</summary> ```text The following tests FAILED: 698 - cpm_generate_pins-nested-makefile (Failed) 700 - cpm_generate_pins-nested-ninja (Failed) 702 - cpm_generate_pins-nested-ninja_multi-config (Failed) 722 - cpm_generate_pins-simple-makefile (Failed) 724 - cpm_generate_pins-simple-ninja (Failed) 726 - cpm_generate_pins-simple-ninja_multi-config (Failed) ``` And they failed in the expected way more informative logs! ```text CMake Error at CMakeLists.txt:51 (message): pinned fmt tag (10.2.1) should differ compared to baseline 10.2.1 pinned_versions.json: { "always_download" : true, "git_shallow" : false, "git_tag" : "${version}", "git_url" : "https:/fmtlib/fmt.git", "version" : "10.2.1" } versions.json: { "git_tag" : "${version}", "git_url" : "https:/fmtlib/fmt.git", "version" : "10.2.1" } ``` ([build link](https:/rapidsai/rapids-cmake/actions/runs/8837613213/job/24267079292?pr=592)) </details> # Authors: - James Lamb (https:/jameslamb) Approvers: - Robert Maynard (https:/robertmaynard) - Kyle Edwards (https:/KyleFromNVIDIA) URL: #599
Description
Add rapids_cpm_generate_pinned_versions to support reproducible builds
rapids-cmake can now generate a
versions.json
that contains the exact git SHA1 used by depdendencies which can be re-usedto construct reproducible builds.
Checklist
cmake-format.json
is up to date with these changes.include_guard(GLOBAL)
)