From c82539c71545f3095f45280d655bfacc490ceda2 Mon Sep 17 00:00:00 2001 From: Vyas Ramasubramani Date: Wed, 4 Dec 2024 14:27:45 -0800 Subject: [PATCH] Fix failures in cpm_generate_pins-nested tests (#724) [Recent nightly failures of the `cpm_generate_pins-nested` test](https://github.com/rapidsai/rapids-cmake/actions/runs/12133373349/attempts/1) are unexpected because neither rapids-cmake, CPM, nor CMake have changed recently. [The error](https://github.com/rapidsai/rapids-cmake/actions/runs/12133373349/job/33828828013#step:8:5066) shows that [the list of projects to verify](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/cpm/verify_generated_pins/CMakeLists.txt#L28) is being completely filtered out because none of them have a `_SOURCE_DIR` variable set. This led me down a rabbit hole of finding a number of different issues in both rapids-cmake itself and in tests. I'll detail them one by one below. ### Problem 1: Our logic for when to download packages for which rapids-cmake provides specialized `rapids_cpm*` commands is wrong The first piece of evidence here is the difference between recent builds of our CI images. [The most recent builds of miniforge-cuda have spdlog in the base environment](https://github.com/rapidsai/ci-imgs/actions/runs/12145567995/job/33867533318#step:8:274), while [previous builds do not](https://github.com/rapidsai/ci-imgs/actions/runs/12036072167/job/33556500014#step:8:307). That should not affect our tests [because we set `CPM_DOWNLOAD_ALL` when populating the CMake cache](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/utils/fill_cache/CMakeLists.txt#L36), but it turns out that #348 introduced a bug where [the `always_download` variable gets defaulted to OFF](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/package_details.cmake#L95) and because of [how it propagates to `CPM_DOWNLOAD_ALL` in parent scope](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/detail/package_details.cmake#L118) we wind up with `CPM_DOWNLOAD_ALL` always being false. As a result, we have actually been ignoring `CPM_DOWNLOAD_ALL` for any packages for which `rapids_cpm_package_details` is called (the more specific `CPM_DOWNLOAD_` should still have been working since that takes precedence). Solution: We unset the `always_download` variable initially instead of setting it to off so that the check is handled correctly. ### Problem 2: Even with the above logic fixed, the actual test will use the spdlog from the environment The above fix ensures that when we populate the CPM source cache in our tests we download spdlog instead of finding it in the environment. However, when tests are subsequently run they will still prefer the package unless `CPM_DOWNLOAD_ALL` (or `CPM_DOWNLOAD_`) is set because of how CPM prioritizes. Solution: To resolve this, for all rapids-cpm tests we are now setting `CPM_DOWNLOAD_` for all of the packages that have been added to our CPM cache. This fix is safe since it effectively just avoids finding local copies of the built package; the source will always come from the cache by design. This change is sufficient for tests to pass, but on closer inspection we see that only spdlog is verified by the verification script and not rmm or fmt. That's because of the next problems. ### Problem 3: All of our tests of pinning are incorrectly checking for rmm with the wrong case [The `project` call in rmm uses the uppercase name "RMM"](https://github.com/rapidsai/rmm/blob/branch-25.02/CMakeLists.txt#L25), but [the entry in rapids-cmake's versions.json](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/rapids-cmake/cpm/versions.json#L66) uses the lowercase name. As a result, our current logic for verifying generate pins can never test rmm because the project [is initially filtered based on `_SOURCE_DIR` being defined](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/cpm/verify_generated_pins.cmake#L33), which will use the uppercase name from the `project` call, but when actually verifying we use `rapids_cpm_package_details` to get the versions.json data _using the same string_, which will now have the wrong case if we passed the uppercase version originally. ### Problem 4: All of our tests of pinning are incorrectly checking for fmt with the wrong case This is the same problem as the above. [fmt uses `project(FMT)`](https://github.com/fmtlib/fmt/blob/master/CMakeLists.txt#L151). Solution for 3 and 4: We now use the `CPM_PACKAGE__SOURCE_DIR` variables instead of the `_SOURCE_DIR` variables. The former are guaranteed to be using the export names of the package and will therefore always be case-consistent with the values in versions.json. ### Other changes in this PR - [This gtest test](https://github.com/rapidsai/rapids-cmake/blob/branch-25.02/testing/cpm/cpm_find-gtest-no-gmock/CMakeLists.txt) is explicitly based on finding a mocked up version of the package locally, so with the changes in this PR we now have to turn of `CPM_DOWNLOAD_GTest` explicitly inside the test. - I removed scikit-build from the testing CPM cache since as of #614 we no longer support its usage via the legacy rapids-cython module. The change to `verify_generated_pins.cmake` ensures that all of the above changes are actually having the desired effect, since now rather than silently passing when only a subset of projects requesting verification are downloaded we will error. All projects must be pulled correctly from the CPM cache during the test (which implicitly checks all four problems above) in order for tests to pass. --- rapids-cmake/cpm/detail/package_details.cmake | 4 +++- .../cpm/cpm_find-gtest-no-gmock/CMakeLists.txt | 5 ++++- testing/cpm/verify_generated_pins.cmake | 7 +++---- testing/utils/cmake_test.cmake | 7 +++++++ testing/utils/fill_cache/CMakeLists.txt | 15 +++++++++++---- testing/utils/setup_cpm_cache.cmake | 9 +++++++++ 6 files changed, 37 insertions(+), 10 deletions(-) diff --git a/rapids-cmake/cpm/detail/package_details.cmake b/rapids-cmake/cpm/detail/package_details.cmake index cca327a4..cc364ea1 100644 --- a/rapids-cmake/cpm/detail/package_details.cmake +++ b/rapids-cmake/cpm/detail/package_details.cmake @@ -92,7 +92,9 @@ function(rapids_cpm_package_details package_name version_var url_var tag_var sha set(exclude_from_all OFF) rapids_cpm_json_get_value(exclude_from_all) - set(always_download OFF) + # Ensure that always_download is not set by default so that the if(DEFINED always_download) check + # below works as expected in the default case. + unset(always_download) if(override_json_data AND json_data AND git_details_overridden) # `always_download` default value requires the package to exist in both the default and override # and that the git url / git tag have been modified. diff --git a/testing/cpm/cpm_find-gtest-no-gmock/CMakeLists.txt b/testing/cpm/cpm_find-gtest-no-gmock/CMakeLists.txt index 8102b1d9..6cdb49ed 100644 --- a/testing/cpm/cpm_find-gtest-no-gmock/CMakeLists.txt +++ b/testing/cpm/cpm_find-gtest-no-gmock/CMakeLists.txt @@ -1,5 +1,5 @@ #============================================================================= -# Copyright (c) 2022, NVIDIA CORPORATION. +# Copyright (c) 2022-2024, NVIDIA CORPORATION. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -20,6 +20,9 @@ include(${rapids-cmake-dir}/cpm/init.cmake) include(${rapids-cmake-dir}/cpm/gtest.cmake) set(CMAKE_PREFIX_PATH "${CMAKE_CURRENT_SOURCE_DIR}/mock_installed_gtest") +# Downloading is turned on by default for all packages in the CPM cache when +# testing, but in this case we specifically want it off. +set(CPM_DOWNLOAD_GTest OFF) rapids_cpm_init() rapids_cpm_gtest() diff --git a/testing/cpm/verify_generated_pins.cmake b/testing/cpm/verify_generated_pins.cmake index c75e788b..014f1234 100644 --- a/testing/cpm/verify_generated_pins.cmake +++ b/testing/cpm/verify_generated_pins.cmake @@ -28,10 +28,9 @@ function(verify_generated_pins target_name) set(_RAPIDS_PIN_FILE "${CMAKE_CURRENT_BINARY_DIR}/rapids-cmake/pinned_versions.json") endif() - # only check projects that were downloaded by CPM (ignore those already in the build environment) foreach(proj IN LISTS _RAPIDS_PROJECTS) - if(${proj}_SOURCE_DIR) - list(APPEND projects-to-verify ${proj}) + if(NOT CPM_PACKAGE_${proj}_SOURCE_DIR) + message(FATAL_ERROR "Attempting to verify a project that was not cloned as part of this build") endif() endforeach() @@ -39,7 +38,7 @@ function(verify_generated_pins target_name) COMMAND ${CMAKE_COMMAND} "-S${CMAKE_CURRENT_FUNCTION_LIST_DIR}/verify_generated_pins/" "-B${CMAKE_BINARY_DIR}/${target_name}_verify_build" "-Drapids-cmake-dir=${rapids-cmake-dir}" "-Dpinned_versions_file=${_RAPIDS_PIN_FILE}" - "-Dprojects-to-verify=${projects-to-verify}" + "-Dprojects-to-verify=${_RAPIDS_PROJECTS}" "-Dprojects-not-in-list=${_RAPIDS_PROJECTS_NOT_EXIST}" VERBATIM ) diff --git a/testing/utils/cmake_test.cmake b/testing/utils/cmake_test.cmake index e679ae43..4089ff47 100644 --- a/testing/utils/cmake_test.cmake +++ b/testing/utils/cmake_test.cmake @@ -96,6 +96,13 @@ function(add_cmake_test mode source_or_dir) if(DEFINED CPM_DOWNLOAD_LOCATION) list(APPEND extra_configure_flags "-DCPM_DOWNLOAD_LOCATION=${CPM_DOWNLOAD_LOCATION}") endif() + if(PACKAGES_IN_CPM_CACHE) + # Prevent ever finding preexisting built packages for those that we have in + # the cache. + foreach(pkg ${PACKAGES_IN_CPM_CACHE}) + list(APPEND extra_configure_flags "-DCPM_DOWNLOAD_${pkg}=ON") + endforeach() + endif() foreach(generator gen_name IN ZIP_LISTS supported_generators nice_gen_names) diff --git a/testing/utils/fill_cache/CMakeLists.txt b/testing/utils/fill_cache/CMakeLists.txt index 746d1e23..945a6da5 100644 --- a/testing/utils/fill_cache/CMakeLists.txt +++ b/testing/utils/fill_cache/CMakeLists.txt @@ -43,11 +43,18 @@ rapids_cpm_nvtx3(DOWNLOAD_ONLY ON) rapids_cpm_rmm(DOWNLOAD_ONLY ON) rapids_cpm_spdlog(DOWNLOAD_ONLY ON) rapids_cpm_fmt(DOWNLOAD_ONLY ON) -rapids_cpm_find(skbuild 0.14.1 - GIT_REPOSITORY https://github.com/scikit-build/scikit-build.git - GIT_TAG 0.14.1 - ) # Download all binary packages set(CPM_DOWNLOAD_ALL "OFF") rapids_cpm_nvcomp(USE_PROPRIETARY_BINARY ON DOWNLOAD_ONLY ON) + +# Print out all the packages in the cache +get_cmake_property(cpm_packages CACHE_VARIABLES) +set(packages) +foreach(p ${cpm_packages}) + if ("${p}" MATCHES "^CPM_PACKAGE_(.*)_SOURCE_DIR") + list(APPEND packages ${CMAKE_MATCH_1}) + endif() +endforeach() +string(REPLACE ";" ", " packages "${packages}") +message(STATUS "CPM packages in cache: {${packages}}") diff --git a/testing/utils/setup_cpm_cache.cmake b/testing/utils/setup_cpm_cache.cmake index 2272523d..c9a4b419 100644 --- a/testing/utils/setup_cpm_cache.cmake +++ b/testing/utils/setup_cpm_cache.cmake @@ -30,8 +30,17 @@ function(setup_cpm_cache ) -DCPM_SOURCE_CACHE=${CPM_SOURCE_CACHE} -DCPM_DOWNLOAD_LOCATION=${CPM_DOWNLOAD_LOCATION} WORKING_DIRECTORY ${src_dir} + OUTPUT_VARIABLE out_var ) + # Find the line in out_var that contains "CPM packages in cache + set(packages) + foreach(line IN LISTS out_var) + if("${line}" MATCHES "CPM packages in cache: {(.*)}") + string(REPLACE ", " ";" packages "${CMAKE_MATCH_1}") + endif() + endforeach() + set(PACKAGES_IN_CPM_CACHE ${packages} PARENT_SCOPE) set(CPM_SOURCE_CACHE "${CPM_SOURCE_CACHE}" PARENT_SCOPE) set(CPM_DOWNLOAD_LOCATION "${CPM_DOWNLOAD_LOCATION}" PARENT_SCOPE) endfunction()