rapidsai / rapids-cmake

https://docs.rapids.ai/api/rapids-cmake/stable/
Apache License 2.0
27 stars 45 forks source link

[BUG] `CPM_ARGS GIT_TAG <something>` Does Not Override `versions.json` #575

Closed Jacobfaib closed 4 months ago

Jacobfaib commented 5 months ago

Describe the bug If you have a git_tag field in versions.json, then rapids-cmake will insert a call to FetchContent_Declare(... GIT_TAG <something>). Internally, FetchContent will cache the results of this call in __FetchContent_declareDetails whose docstring states:

#=======================================================================
# Recording and retrieving content details for later population
#=======================================================================

# Internal use, projects must not call this directly. It is
# intended for use by FetchContent_Declare() only.
#
# Sets a content-specific global property (not meant for use
# outside of functions defined here in this file) which can later
# be retrieved using __FetchContent_getSavedDetails() with just the
# same content name. If there is already a value stored in the
# property, it is left unchanged and this call has no effect.
# This allows parent projects to define the content details,
# overriding anything a child project may try to set (properties
# are not cached between runs, so the first thing to set it in a
# build will be in control).

Note (emphasis mine)

Sets a content-specific global property [...] If there is already a value stored in the property, it is left unchanged and this call has no effect

This means that next time CPM goes to try and override these (i.e. in rapids_cpm_find()), FetchContent_Declare() they seemingly have no effect.

Steps/Code to reproduce bug versions.json:

{
  "packages" : {
    "Legion": {
      "version" : "24.1.0",
      "git_url" : "https://gitlab.com/StanfordLegion/legion.git",
      "git_tag" : "1a5e1d237b4f07a39ddfde0bd43f30018045cd10",
      "git_shallow" : false
    }
}

CMakeLists.txt:

cmake_minimum_required(VERSION 3.22.1 FATAL_ERROR)

# <install rapids somehow>

project(Example VERSION 1.0 LANGUAGES C CXX)

rapids_cpm_init(OVERRIDE /path/to/versions.json)

rapids_cpm_find(Legion 24.1.0
  GLOBAL_TARGETS Legion::Realm Legion::Regent Legion::Legion Legion::RealmRuntime Legion::LegionRuntime 
  BUILD_EXPORT_SET legate-core-exports 
  INSTALL_EXPORT_SET legate-core-exports 
  CPM_ARGS 
    GIT_REPOSITORY https://gitlab.com/StanfordLegion/legion.git
    GIT_TAG master # NOTE: we want this to override versions.json!
    GIT_SHALLOW FALSE 
    URL https://gitlab.com/StanfordLegion/legion/-/archive/master//legion-master.zip
)
$ cmake -S . -B build

Expected behavior The above should download the master branch of Legion, not 1a5e1d237b4f07a39ddfde0bd43f30018045cd10.

Environment details (please complete the following information):

robertmaynard commented 4 months ago

This unforunately can't be supported. The behavior you are seeing is the explicit use case that rapids_cpm_package_override was added to solve ( https://docs.rapids.ai/api/rapids-cmake/nightly/command/rapids_cpm_package_override/ ) . To be functional, it needs to override the behavior of rapids_cpm_find calls.

You have a couple of solutions:

  1. Not have Legion in your override file at all. Use the rapids_cpm_find as you currently have it and it will use the master tag. If you want to force no local searches via find_package before building from source, you can add set(CPM_DOWNLOAD_Legion ON) before the rapids_cpm_find.
  2. Not have Legion in your override file at all. Have N separate legion_${tag}_versions.json files that you load after with rapids_cpm_package_override. Those files would have the different tags/sha1's/urls of legion that need to support and you would load only one of them based on the requested build information.
Jacobfaib commented 4 months ago

Hmm the use-case we are trying to support is: have a default git hash/commit for Legion then allow the user to (possibly) override at runtime. Since the override is a user-option it's not feasible for us to predict what it might be (so couldn't e.g. do the legion_${tag}_versions.json).

Do you have any recommendations on how to support this? Or is this just not feasible?

robertmaynard commented 4 months ago

I opened https://github.com/rapidsai/rapids-cmake/pull/578 so that users better understand what exactly the OVERRIDE option is doing.

robertmaynard commented 4 months ago

Hmm the use-case we are trying to support is: have a default git hash/commit for Legion then allow the user to (possibly) override at runtime. Do you have any recommendations on how to support this? Or is this just not feasible?

@Jacobfaib I expect you want something like this:

if(NOT LEGION_GIT_TAG)
  set(LEGION_GIT_TAG 1a5e1d237b4f07a39ddfde0bd43f30018045cd10)
endif()

if(NOT LEGION_GIT_REPO)
  set(LEGION_GIT_REPO https://gitlab.com/StanfordLegion/legion.git)
endif()

rapids_cpm_find(Legion 24.1.0
  GLOBAL_TARGETS Legion::Realm Legion::Regent Legion::Legion Legion::RealmRuntime Legion::LegionRuntime 
  BUILD_EXPORT_SET legate-core-exports 
  INSTALL_EXPORT_SET legate-core-exports 
  CPM_ARGS 
    GIT_REPOSITORY ${LEGION_GIT_REPO}
    GIT_TAG ${LEGION_GIT_TAG}
    GIT_SHALLOW FALSE 
    URL https://gitlab.com/StanfordLegion/legion/-/archive/master//legion-master.zip
)

This kind of pattern is used by projects like raft ( https://github.com/rapidsai/raft/blob/branch-24.06/cpp/cmake/thirdparty/get_hnswlib.cmake ). So that users can specify cmake -DLEGION_GIT_TAG=master to try out a new branch

Jacobfaib commented 4 months ago

so couldn't e.g. do the legion_${tag}_versions.json

Well... actually maybe we could. We could generate this at runtime if needs be. Are you allowed to call rapids_cpm_override() multiple times? I.e. would the following work:

rapids_cpm_package_override(version_except_legion.json)
if(Use_Default_Legion_Version)
  rapids_cpm_package_override(version_legion_only.json)
else()
  generate_version_json(User_Legion_Version)
  rapids_cpm_package_override(runtime_generated_legion_version.json)
endif()
robertmaynard commented 4 months ago

Are you allowed to call rapids_cpm_override() multiple times? I.e. would the following work

Yes, that will absolutely work.