Skip to content

Commit

Permalink
Warn when override of a default package uses the incorrect case (#708)
Browse files Browse the repository at this point in the history
It is a common mistake to presume that all entries in a package override json should use lower case names, since the vast majority of default packages are all lower case. This causes silent 'failures' when the override doesn't work.

We now normalize all package data internally so that  overrides for `ABC`, `abc`, and `Abc` all map to the same package. In addition we have added information tracking the proper name of bundled packages. This allows us to warn when an override uses the non correct package name, and treat it like the proper name.

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #708
  • Loading branch information
robertmaynard authored Nov 5, 2024
1 parent 03ec7ef commit 8c26daa
Show file tree
Hide file tree
Showing 7 changed files with 199 additions and 33 deletions.
3 changes: 2 additions & 1 deletion rapids-cmake/cpm/detail/get_default_json.cmake
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#=============================================================================
# Copyright (c) 2021, NVIDIA CORPORATION.
# Copyright (c) 2021-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.
Expand All @@ -26,6 +26,7 @@ get_default_json
#]=======================================================================]
function(get_default_json package_name output_variable)
list(APPEND CMAKE_MESSAGE_CONTEXT "rapids.cpm.get_default_json")
string(TOLOWER "${package_name}" package_name)
get_property(json_data GLOBAL PROPERTY rapids_cpm_${package_name}_json)
set(${output_variable} "${json_data}" PARENT_SCOPE)
endfunction()
3 changes: 2 additions & 1 deletion rapids-cmake/cpm/detail/get_override_json.cmake
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#=============================================================================
# Copyright (c) 2021, NVIDIA CORPORATION.
# Copyright (c) 2021-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.
Expand All @@ -26,6 +26,7 @@ get_override_json
#]=======================================================================]
function(get_override_json package_name output_variable)
list(APPEND CMAKE_MESSAGE_CONTEXT "rapids.cpm.get_override_json")
string(TOLOWER "${package_name}" package_name)
get_property(json_data GLOBAL PROPERTY rapids_cpm_${package_name}_override_json)
set(${output_variable} "${json_data}" PARENT_SCOPE)
endfunction()
17 changes: 13 additions & 4 deletions rapids-cmake/cpm/detail/load_preset_versions.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,19 @@ function(rapids_cpm_load_preset_versions)
string(JSON package_name MEMBER "${json_data}" packages ${index})
string(JSON data GET "${json_data}" packages "${package_name}")

get_property(already_exists GLOBAL PROPERTY rapids_cpm_${package_name}_json SET)
if(NOT already_exists)
set_property(GLOBAL PROPERTY rapids_cpm_${package_name}_json "${data}")
set_property(GLOBAL PROPERTY rapids_cpm_${package_name}_json_file "${filepath}")
# Normalize the names all to lower case. This will allow us to better support overrides with
# different package name casing
string(TOLOWER "${package_name}" normalized_pkg_name)
get_property(already_exists GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_json SET)
if(already_exists)
# Warn that we have duplicate entries in the default json file
message(AUTHOR_WARNING "RAPIDS-CMake has detected two entries for ${package_name} in ${_rapids_preset_version_file}. Please ensure a single entry as all names are cased insensitive"
)
else()
set_property(GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_json "${data}")
set_property(GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_json_file "${filepath}")

set_property(GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_proper_name "${package_name}")
endif()
endforeach()

Expand Down
68 changes: 43 additions & 25 deletions rapids-cmake/cpm/package_override.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -94,32 +94,50 @@ function(rapids_cpm_package_override _rapids_override_filepath)
# cmake-lint: disable=E1120
foreach(index RANGE ${package_count})
string(JSON package_name MEMBER "${json_data}" packages ${index})
get_property(override_exists GLOBAL PROPERTY rapids_cpm_${package_name}_override_json DEFINED)
if(NOT (override_exists OR DEFINED CPM_${package_name}_SOURCE))
# only add the first override for a project we encounter
string(JSON data GET "${json_data}" packages "${package_name}")
set_property(GLOBAL PROPERTY rapids_cpm_${package_name}_override_json "${data}")
set_property(GLOBAL PROPERTY rapids_cpm_${package_name}_override_json_file
"${_rapids_override_filepath}")

# establish the fetch content
include(FetchContent)
include("${rapids-cmake-dir}/cpm/detail/package_details.cmake")
rapids_cpm_package_details(${package_name} version repository tag shallow exclude)

include("${rapids-cmake-dir}/cpm/detail/generate_patch_command.cmake")
rapids_cpm_generate_patch_command(${package_name} ${version} patch_command)

unset(exclude_from_all)
if(exclude AND CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0)
set(exclude_from_all EXCLUDE_FROM_ALL)
endif()
FetchContent_Declare(${package_name}
GIT_REPOSITORY ${repository}
GIT_TAG ${tag}
GIT_SHALLOW ${shallow}
${patch_command} ${exclude_from_all})
string(TOLOWER "${package_name}" normalized_pkg_name)
get_property(override_exists GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_override_json
DEFINED)

if(override_exists OR DEFINED CPM_${package_name}_SOURCE)
continue()
endif()

# Warn if our name all lower case matches a default package, but our case sensitive names
# doesn't ( ABC vs abc )
get_property(package_proper_name GLOBAL
PROPERTY rapids_cpm_${normalized_pkg_name}_proper_name)
if(package_proper_name AND NOT package_proper_name STREQUAL package_name)
message(AUTHOR_WARNING "RAPIDS-CMake is assuming the override ${package_name} is meant for the ${package_proper_name} package. For correctness please use the correctly cased name"
)
endif()
if(NOT package_proper_name)
set(package_proper_name ${package_name}) # Required for FetchContent_Declare
endif()

# only add the first override for a project we encounter
string(JSON data GET "${json_data}" packages "${package_name}")
set_property(GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_override_json "${data}")
set_property(GLOBAL PROPERTY rapids_cpm_${normalized_pkg_name}_override_json_file
"${_rapids_override_filepath}")

# establish the fetch content
include(FetchContent)
include("${rapids-cmake-dir}/cpm/detail/package_details.cmake")
rapids_cpm_package_details(${package_name} version repository tag shallow exclude)

include("${rapids-cmake-dir}/cpm/detail/generate_patch_command.cmake")
rapids_cpm_generate_patch_command(${package_name} ${version} patch_command)

unset(exclude_from_all)
if(exclude AND CMAKE_VERSION VERSION_GREATER_EQUAL 3.28.0)
set(exclude_from_all EXCLUDE_FROM_ALL)
endif()
FetchContent_Declare(${package_proper_name}
GIT_REPOSITORY ${repository}
GIT_TAG ${tag}
GIT_SHALLOW ${shallow}
${patch_command} ${exclude_from_all})
unset(package_proper_name)
endforeach()
endif()
endfunction()
6 changes: 4 additions & 2 deletions testing/cpm/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,18 @@ add_cmake_config_test( cpm_init-bad-override-cmake-var.cmake SHOULD_FAIL "rapids
add_cmake_config_test( cpm_init-custom-default-simple.cmake)
add_cmake_config_test( cpm_init-custom-default-multiple.cmake)
add_cmake_config_test( cpm_init-custom-default-via-cmake-var.cmake)
add_cmake_config_test( cpm_init-override-simple.cmake)
add_cmake_config_test( cpm_init-override-multiple.cmake)
add_cmake_config_test( cpm_init-override-simple.cmake)
add_cmake_config_test( cpm_init-override-via-cmake-var.cmake)


add_cmake_config_test( cpm_package_override-add-new-project.cmake )
add_cmake_config_test( cpm_package_override-bad-path.cmake SHOULD_FAIL "rapids_cpm_package_override can't load")
add_cmake_config_test( cpm_package_override-before-init.cmake )
add_cmake_config_test( cpm_package_override-empty.cmake )
add_cmake_config_test( cpm_package_override-defaults-with-different-casing.cmake )
add_cmake_config_test( cpm_package_override-defaults-with-different-casing-warning.cmake)
add_cmake_config_test( cpm_package_override-empty-patches.cmake )
add_cmake_config_test( cpm_package_override-empty.cmake )
add_cmake_config_test( cpm_package_override-env-var-support.cmake )
add_cmake_config_test( cpm_package_override-multiple.cmake )
add_cmake_config_test( cpm_package_override-no-version-value.cmake SHOULD_FAIL "rapids_cmake can't parse")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#=============================================================================
# Copyright (c) 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.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#=============================================================================
# rapids-cmake will output as `AUTHOR_WARNING` so we need to hijack
# message to verify we see output we expect
set(rmm_string "RAPIDS-CMake is assuming the override rMm is meant for the rmm package")
set(gtest_string "RAPIDS-CMake is assuming the override gtest is meant for the GTest package")

function(message mode content )
if(mode STREQUAL "AUTHOR_WARNING")
if(content MATCHES "${rmm_string}")
message(STATUS "==rmm warned==")
set_property(GLOBAL PROPERTY rapids_cmake_rmm_warned TRUE)
elseif(content MATCHES "${gtest_string}")
message(STATUS "==gtest warned==")
set_property(GLOBAL PROPERTY rapids_cmake_gtest_warned TRUE)
endif()
else()
_message(${ARGV})
endif()
endfunction()

include(${rapids-cmake-dir}/cpm/init.cmake)
include(${rapids-cmake-dir}/cpm/package_override.cmake)

rapids_cpm_init()

# Need to write out an override file
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/override.json
[=[
{
"packages": {
"rMm": {
"git_url": "new_rmm_url",
"git_shallow": "OFF",
"exclude_from_all": "ON"
},
"gtest": {
"version": "3.00.A1"
}
}
}
]=])

rapids_cpm_package_override(${CMAKE_CURRENT_BINARY_DIR}/override.json)
include("${rapids-cmake-dir}/cpm/detail/package_details.cmake")

# Verify that we got our warnings
get_property(rmm_warned GLOBAL PROPERTY rapids_cmake_rmm_warned)
get_property(gtest_warned GLOBAL PROPERTY rapids_cmake_gtest_warned)
if(NOT rmm_warned)
message(FATAL_ERROR "Failed to warn about improper rmm casing")
endif()
if(NOT gtest_warned)
message(FATAL_ERROR "Failed to warn about improper gtest casing")
endif()
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#=============================================================================
# Copyright (c) 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.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#=============================================================================

include(${rapids-cmake-dir}/cpm/init.cmake)
include(${rapids-cmake-dir}/cpm/package_override.cmake)

rapids_cpm_init()

# Need to write out an override file
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/override.json
[=[
{
"packages": {
"rMm": {
"git_url": "new_rmm_url",
"git_shallow": "OFF",
"exclude_from_all": "ON"
},
"gtest": {
"version": "3.00.A1"
}
}
}
]=])

rapids_cpm_package_override(${CMAKE_CURRENT_BINARY_DIR}/override.json)

rapids_cpm_package_details(GTest version repository tag shallow exclude)
if(NOT version STREQUAL "3.00.A1")
message(FATAL_ERROR "custom version field was removed. ${version} was found instead")
endif()
if(NOT tag MATCHES "3.00.A1")
message(FATAL_ERROR "custom version field not used when computing git_tag value. ${tag} was found instead")
endif()
if(NOT exclude MATCHES "OFF")
message(FATAL_ERROR "default value of exclude not found. ${exclude} was found instead")
endif()
if(CPM_DOWNLOAD_ALL)
message(FATAL_ERROR "CPM_DOWNLOAD_ALL should be false by default when an override exists that doesn't modify url or tag")
endif()

rapids_cpm_package_details(rmm version repository tag shallow exclude)
if(NOT repository MATCHES "new_rmm_url")
message(FATAL_ERROR "custom url field was not used. ${repository} was found instead")
endif()
if(NOT shallow MATCHES "OFF")
message(FATAL_ERROR "override should not change git_shallow value. ${shallow} was found instead")
endif()
if(NOT exclude MATCHES "ON")
message(FATAL_ERROR "override should have changed exclude value. ${exclude} was found instead")
endif()
if(NOT CPM_DOWNLOAD_ALL)
message(FATAL_ERROR "CPM_DOWNLOAD_ALL should be set to true when an override exists with a custom repository")
endif()

0 comments on commit 8c26daa

Please sign in to comment.