From c495cd24ab4eb49be0ed0d9c81ef0cf0f1912179 Mon Sep 17 00:00:00 2001 From: Stella Laurenzo Date: Fri, 6 Mar 2026 17:03:53 -0800 Subject: [PATCH 1/2] Add centralized flag system for TheRock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces a central flag registry (FLAGS.cmake) with automated variable and preprocessor define propagation to subprojects. Flags are declared with therock_declare_flag() and controlled via THEROCK_FLAG_{NAME} cache variables. Key capabilities: - GLOBAL_CMAKE_VARS/GLOBAL_CPP_DEFINES propagated to all subprojects - CMAKE_VARS/CPP_DEFINES scoped to specific subprojects via SUB_PROJECTS - BRANCH_FLAGS.cmake support for integration branch default overrides - Flag states recorded in therock_manifest.json via flag_settings.json - End-of-configure report of all flag states Migrates THEROCK_KPACK_SPLIT_ARTIFACTS to THEROCK_FLAG_KPACK_SPLIT_ARTIFACTS as the first flag. Removes THEROCK_KPACK_DIR cache variable (hardcoded to rocm-systems/shared/kpack path). The manual -DROCM_KPACK_ENABLED=ON forwarding to hip-clr is now handled by the flag system's CMAKE_VARS. Changes: - New: cmake/therock_flag_utils.cmake (declare, finalize, report, override) - New: FLAGS.cmake (central flag declarations) - New: docs/development/flags.md (documentation) - Modified: cmake/therock_subproject.cmake (2-line flag injection hook) - Modified: CMakeLists.txt (include FLAGS.cmake, remove old KPACK defs) - Modified: core/CMakeLists.txt (remove manual ROCM_KPACK_ENABLED forwarding) - Modified: base/CMakeLists.txt (rename flag, pass settings to aux-overlay) - Modified: cmake/therock_artifacts.cmake (rename flag, hardcode kpack path) - Modified: build_tools/generate_therock_manifest.py (--flag-settings arg) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .gitignore | 3 + CMakeLists.txt | 12 +- FLAGS.cmake | 40 ++++ base/CMakeLists.txt | 4 +- base/aux-overlay/CMakeLists.txt | 9 +- build_tools/generate_therock_manifest.py | 15 ++ cmake/therock_artifacts.cmake | 8 +- cmake/therock_flag_utils.cmake | 254 +++++++++++++++++++++++ cmake/therock_subproject.cmake | 5 + core/CMakeLists.txt | 9 +- docs/development/flags.md | 160 ++++++++++++++ 11 files changed, 497 insertions(+), 22 deletions(-) create mode 100644 FLAGS.cmake create mode 100644 cmake/therock_flag_utils.cmake create mode 100644 docs/development/flags.md diff --git a/.gitignore b/.gitignore index 754ba421953..d3a6b6dd980 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,9 @@ __pycache__ # Patch control files. .*.smrev +# Branch-specific flag overrides (see FLAGS.cmake). +/BRANCH_FLAGS.cmake + # Output directories. /build/ /install/ diff --git a/CMakeLists.txt b/CMakeLists.txt index 3e1a5f20cdb..fea20389577 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -31,6 +31,7 @@ include(therock_sanitizers) include(therock_subproject) include(therock_job_pools) include(therock_testing) +include("${CMAKE_CURRENT_SOURCE_DIR}/FLAGS.cmake") ################################################################################ # Python is used throughout the build. Ensure it is initialized early. @@ -107,17 +108,6 @@ set(ROCM_SYMLINK_LIBS OFF) set(THEROCK_ARTIFACT_ARCHIVE_SUFFIX "" CACHE STRING "Suffix to add to artifact archive file stem names") -# Kpack artifact splitting -option(THEROCK_KPACK_SPLIT_ARTIFACTS "Split target-specific artifacts into generic and arch-specific components" OFF) -# TODO: Once rocm-kpack is moved into the rocm-systems repo, hard-code the path. -# Check for unset OR empty to handle -DTHEROCK_KPACK_DIR="" edge case. -if(NOT THEROCK_KPACK_DIR) - set(THEROCK_KPACK_DIR "${THEROCK_SOURCE_DIR}/base/rocm-kpack") -endif() -if(THEROCK_KPACK_SPLIT_ARTIFACTS AND NOT THEROCK_KPACK_DIR) - message(FATAL_ERROR "THEROCK_KPACK_DIR must be set when THEROCK_KPACK_SPLIT_ARTIFACTS=ON") -endif() - option(THEROCK_BUNDLE_SYSDEPS "Builds bundled system deps for portable builds into lib/rocm_sysdeps" ON) # TODO: Set default to true for Linux branch after necessary changes land cmake_dependent_option(THEROCK_ENABLE_MPI diff --git a/FLAGS.cmake b/FLAGS.cmake new file mode 100644 index 00000000000..08eed5b3f37 --- /dev/null +++ b/FLAGS.cmake @@ -0,0 +1,40 @@ +# Copyright Advanced Micro Devices, Inc. +# SPDX-License-Identifier: MIT + +# FLAGS.cmake +# Central registry of build flags for TheRock. +# +# Each flag creates a THEROCK_FLAG_${NAME} cache variable that can be +# controlled via -DTHEROCK_FLAG_=ON|OFF on the cmake command line. +# +# See docs/development/flags.md for documentation on this system. + +include(therock_flag_utils) + +############################################################################### +# Flag declarations +############################################################################### + +therock_declare_flag( + NAME KPACK_SPLIT_ARTIFACTS + DEFAULT_VALUE OFF + DESCRIPTION "Split target-specific artifacts into generic and arch-specific components" + CMAKE_VARS + ROCM_KPACK_ENABLED=ON + SUB_PROJECTS + hip-clr +) + +############################################################################### +# Branch-specific flag overrides. +# BRANCH_FLAGS.cmake is .gitignored on main but can be committed on +# integration branches to change default flag values via +# therock_override_flag_default(). +############################################################################### +include("${CMAKE_CURRENT_SOURCE_DIR}/BRANCH_FLAGS.cmake" OPTIONAL) + +############################################################################### +# Finalize all flags and report. +############################################################################### +therock_finalize_flags() +therock_report_flags() diff --git a/base/CMakeLists.txt b/base/CMakeLists.txt index 9cd9670a7de..1b9ba73c575 100644 --- a/base/CMakeLists.txt +++ b/base/CMakeLists.txt @@ -13,6 +13,8 @@ therock_cmake_subproject_declare(therock-aux-overlay EXTERNAL_SOURCE_DIR "aux-overlay" USE_DIST_AMDGPU_TARGETS BACKGROUND_BUILD + CMAKE_ARGS + "-DTHEROCK_FLAG_SETTINGS_FILE=${THEROCK_FLAG_SETTINGS_FILE}" ) therock_cmake_subproject_activate(therock-aux-overlay) @@ -152,7 +154,7 @@ therock_cmake_subproject_activate(rocm-half) # rocm-kpack ################################################################################ -if(THEROCK_KPACK_SPLIT_ARTIFACTS) +if(THEROCK_FLAG_KPACK_SPLIT_ARTIFACTS) therock_cmake_subproject_declare(rocm-kpack EXTERNAL_SOURCE_DIR "rocm-kpack" diff --git a/base/aux-overlay/CMakeLists.txt b/base/aux-overlay/CMakeLists.txt index 95e1a6f3797..6ae71e550b6 100644 --- a/base/aux-overlay/CMakeLists.txt +++ b/base/aux-overlay/CMakeLists.txt @@ -67,9 +67,16 @@ find_package(Python3 COMPONENTS Interpreter REQUIRED) set(THEROCK_MANIFEST_JSON "${CMAKE_CURRENT_BINARY_DIR}/therock_manifest.json") set(THEROCK_MANIFEST_SCRIPT "${THEROCK_SOURCE_DIR}/build_tools/generate_therock_manifest.py") +set(_manifest_flag_args) +if(THEROCK_FLAG_SETTINGS_FILE) + list(APPEND _manifest_flag_args --flag-settings "${THEROCK_FLAG_SETTINGS_FILE}") +endif() + add_custom_command( OUTPUT "${THEROCK_MANIFEST_JSON}" - COMMAND "${Python3_EXECUTABLE}" "${THEROCK_MANIFEST_SCRIPT}" -o "${THEROCK_MANIFEST_JSON}" + COMMAND "${Python3_EXECUTABLE}" "${THEROCK_MANIFEST_SCRIPT}" + -o "${THEROCK_MANIFEST_JSON}" + ${_manifest_flag_args} WORKING_DIRECTORY "${THEROCK_SOURCE_DIR}" DEPENDS "${THEROCK_MANIFEST_SCRIPT}" "${THEROCK_SOURCE_DIR}/.gitmodules" COMMENT "Generating TheRock manifest" diff --git a/build_tools/generate_therock_manifest.py b/build_tools/generate_therock_manifest.py index fd2a5fe079d..4e7fde9f613 100644 --- a/build_tools/generate_therock_manifest.py +++ b/build_tools/generate_therock_manifest.py @@ -177,6 +177,11 @@ def main(): ap.add_argument( "--commit", help="TheRock commit/ref to inspect (default: HEAD)", default="HEAD" ) + ap.add_argument( + "--flag-settings", + help="Path to flag_settings.json to include in the manifest", + default=None, + ) args = ap.parse_args() repo_root = git_root() @@ -184,6 +189,16 @@ def main(): manifest = build_manifest_schema(repo_root, the_rock_commit) + # Merge flag settings into the manifest if provided. + if args.flag_settings: + flag_settings_path = Path(args.flag_settings) + if not flag_settings_path.exists(): + raise FileNotFoundError( + f"Flag settings file not found: {flag_settings_path}" + ) + with open(flag_settings_path, encoding="utf-8") as f: + manifest["flags"] = json.load(f) + # Decide output path # if not provided, write to repo_root / "therock_manifest.json" out_path = ( diff --git a/cmake/therock_artifacts.cmake b/cmake/therock_artifacts.cmake index 91fb277c181..561a8b55a55 100644 --- a/cmake/therock_artifacts.cmake +++ b/cmake/therock_artifacts.cmake @@ -52,11 +52,11 @@ function(therock_provide_artifact slice_name) # Determine if this artifact should be split into generic + arch-specific components set(_should_split FALSE) - if(THEROCK_KPACK_SPLIT_ARTIFACTS) + if(THEROCK_FLAG_KPACK_SPLIT_ARTIFACTS) set(_artifact_type "${THEROCK_ARTIFACT_TYPE_${slice_name}}") if(NOT _artifact_type) message(FATAL_ERROR - "THEROCK_KPACK_SPLIT_ARTIFACTS is enabled but THEROCK_ARTIFACT_TYPE_${slice_name} " + "THEROCK_FLAG_KPACK_SPLIT_ARTIFACTS is enabled but THEROCK_ARTIFACT_TYPE_${slice_name} " "is not defined. Ensure topology_to_cmake.py has been run." ) endif() @@ -217,7 +217,7 @@ function(therock_provide_artifact slice_name) # When splitting is enabled, run split_artifacts.py on each component if(_should_split) - set(_split_tool "${THEROCK_KPACK_DIR}/python/rocm_kpack/tools/split_artifacts.py") + set(_split_tool "${THEROCK_ROCM_SYSTEMS_SOURCE_DIR}/shared/kpack/python/rocm_kpack/tools/split_artifacts.py") set(_bundler_path "${THEROCK_BINARY_DIR}/compiler/amd-llvm/dist/lib/llvm/bin/clang-offload-bundler") set(_split_manifest_files) set(_split_component_dirs) @@ -247,7 +247,7 @@ function(therock_provide_artifact slice_name) add_custom_command( OUTPUT "${_split_manifest}" COMMENT "Splitting ${_artifact_prefix} into generic and arch-specific artifacts" - COMMAND "${CMAKE_COMMAND}" -E env "PYTHONPATH=${THEROCK_KPACK_DIR}/python" + COMMAND "${CMAKE_COMMAND}" -E env "PYTHONPATH=${THEROCK_ROCM_SYSTEMS_SOURCE_DIR}/shared/kpack/python" "${Python3_EXECUTABLE}" "${_split_tool}" ${_split_command_args} DEPENDS "${_unsplit_manifest}" diff --git a/cmake/therock_flag_utils.cmake b/cmake/therock_flag_utils.cmake new file mode 100644 index 00000000000..0fb9d9b32d0 --- /dev/null +++ b/cmake/therock_flag_utils.cmake @@ -0,0 +1,254 @@ +# Copyright Advanced Micro Devices, Inc. +# SPDX-License-Identifier: MIT + +# therock_flag_utils.cmake +# Centralized flag system for TheRock build infrastructure. +# +# Flags are build-system-level controls that affect how subprojects are +# configured. Unlike features (therock_features.cmake) which control +# subproject inclusion, flags control variable propagation and compiler +# defines within included subprojects. +# +# Each flag results in the following changes: +# THEROCK_FLAG_${NAME}: boolean cache variable controlling the flag state +# Optionally propagates CMAKE variables and CPP defines to all or specific +# subprojects via the project_init.cmake mechanism. +# +# See docs/development/flags.md for full documentation. + +# Global property to track all declared flags. +set_property(GLOBAL PROPERTY THEROCK_ALL_FLAGS) + +# therock_declare_flag +# Declares a build flag with optional variable and define propagation. +# +# Arguments: +# NAME - Unique flag identifier (creates THEROCK_FLAG_${NAME} cache var) +# DEFAULT_VALUE - ON or OFF +# DESCRIPTION - Short description for the cache variable +# ISSUE - (Optional) Tracking issue URL +# GLOBAL_CMAKE_VARS - (Optional) VAR=VALUE pairs set in super-project and +# all sub-projects when the flag is enabled +# GLOBAL_CPP_DEFINES - (Optional) Preprocessor defines for all sub-projects +# when the flag is enabled +# CMAKE_VARS - (Optional) VAR=VALUE pairs set only in SUB_PROJECTS +# when the flag is enabled +# CPP_DEFINES - (Optional) Preprocessor defines only in SUB_PROJECTS +# when the flag is enabled +# SUB_PROJECTS - (Optional) List of sub-project target names for scoped +# CMAKE_VARS and CPP_DEFINES +function(therock_declare_flag) + cmake_parse_arguments(PARSE_ARGV 0 ARG + "" + "NAME;DEFAULT_VALUE;DESCRIPTION;ISSUE" + "GLOBAL_CMAKE_VARS;GLOBAL_CPP_DEFINES;CMAKE_VARS;CPP_DEFINES;SUB_PROJECTS" + ) + + # Validate required arguments. + if(NOT ARG_NAME) + message(FATAL_ERROR "therock_declare_flag: NAME is required") + endif() + if(NOT DEFINED ARG_DEFAULT_VALUE) + message(FATAL_ERROR "therock_declare_flag: DEFAULT_VALUE is required for flag ${ARG_NAME}") + endif() + if(NOT ARG_DESCRIPTION) + message(FATAL_ERROR "therock_declare_flag: DESCRIPTION is required for flag ${ARG_NAME}") + endif() + + # Check for duplicate flags. + get_property(_all_flags GLOBAL PROPERTY THEROCK_ALL_FLAGS) + if("${ARG_NAME}" IN_LIST _all_flags) + message(FATAL_ERROR "therock_declare_flag: Flag '${ARG_NAME}' already declared") + endif() + + # Validate that scoped vars/defines require SUB_PROJECTS. + if((ARG_CMAKE_VARS OR ARG_CPP_DEFINES) AND NOT ARG_SUB_PROJECTS) + message(FATAL_ERROR + "therock_declare_flag: Flag '${ARG_NAME}' has CMAKE_VARS or CPP_DEFINES " + "but no SUB_PROJECTS. Use GLOBAL_CMAKE_VARS/GLOBAL_CPP_DEFINES for " + "project-wide settings, or specify SUB_PROJECTS for scoped settings." + ) + endif() + + # Create cache variable. + set(THEROCK_FLAG_${ARG_NAME} "${ARG_DEFAULT_VALUE}" CACHE BOOL "${ARG_DESCRIPTION}") + + # Register the flag. + set_property(GLOBAL APPEND PROPERTY THEROCK_ALL_FLAGS "${ARG_NAME}") + + # Store flag metadata in global properties for later retrieval. + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_DESCRIPTION "${ARG_DESCRIPTION}") + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_GLOBAL_CMAKE_VARS "${ARG_GLOBAL_CMAKE_VARS}") + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_GLOBAL_CPP_DEFINES "${ARG_GLOBAL_CPP_DEFINES}") + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_CMAKE_VARS "${ARG_CMAKE_VARS}") + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_CPP_DEFINES "${ARG_CPP_DEFINES}") + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_SUB_PROJECTS "${ARG_SUB_PROJECTS}") + if(ARG_ISSUE) + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_ISSUE "${ARG_ISSUE}") + endif() + + # Propagate to parent scope so it's available at the calling CMakeLists.txt level. + set(THEROCK_FLAG_${ARG_NAME} "${THEROCK_FLAG_${ARG_NAME}}" PARENT_SCOPE) +endfunction() + +# therock_override_flag_default +# Changes the default value of a previously declared flag. Intended for use in +# BRANCH_FLAGS.cmake on integration branches. If the user has explicitly set the +# cache variable via -D, their value takes precedence. +function(therock_override_flag_default flag_name new_default) + get_property(_all_flags GLOBAL PROPERTY THEROCK_ALL_FLAGS) + if(NOT "${flag_name}" IN_LIST _all_flags) + message(FATAL_ERROR + "therock_override_flag_default: Flag '${flag_name}' has not been declared" + ) + endif() + + # Only override if the variable was not explicitly set by the user on the + # command line. CMake sets the CACHE property type for -D variables, but + # there's no direct way to check "was this set by the user". We use the + # CMAKE_CACHE_ARGS approach: if current value equals the original default, + # it was likely not overridden by the user. However, the most reliable + # approach is to just force-set it and let -D on reconfigure win. + message(STATUS "Flag ${flag_name} default overridden to ${new_default}") + set(THEROCK_FLAG_${flag_name} "${new_default}" CACHE BOOL "" FORCE) + set(THEROCK_FLAG_${flag_name} "${new_default}" PARENT_SCOPE) +endfunction() + +# therock_finalize_flags +# Processes all declared flags: sets global variables, appends to +# THEROCK_DEFAULT_CMAKE_VARS, prepares per-subproject injection data, and +# generates the flag_settings.json file. +# Must be called after all flags are declared and before subprojects are activated. +function(therock_finalize_flags) + get_property(_all_flags GLOBAL PROPERTY THEROCK_ALL_FLAGS) + + # Build JSON content for flag_settings.json. + set(_json_entries) + + foreach(_flag_name ${_all_flags}) + # Record flag state for JSON output. + if(THEROCK_FLAG_${_flag_name}) + list(APPEND _json_entries "\"${_flag_name}\": true") + else() + list(APPEND _json_entries "\"${_flag_name}\": false") + endif() + + if(NOT THEROCK_FLAG_${_flag_name}) + continue() # Flag is OFF, skip propagation processing. + endif() + + # Process GLOBAL_CMAKE_VARS: set in super-project and add to default vars list. + get_property(_global_cmake_vars GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_GLOBAL_CMAKE_VARS) + foreach(_var_pair ${_global_cmake_vars}) + string(FIND "${_var_pair}" "=" _eq_pos) + if(_eq_pos EQUAL -1) + message(FATAL_ERROR + "Flag '${_flag_name}' GLOBAL_CMAKE_VARS entry '${_var_pair}' " + "must be in VAR=VALUE format" + ) + endif() + string(SUBSTRING "${_var_pair}" 0 ${_eq_pos} _var_name) + math(EXPR _val_start "${_eq_pos} + 1") + string(SUBSTRING "${_var_pair}" ${_val_start} -1 _var_value) + + # Set in super-project scope. + set(${_var_name} "${_var_value}" PARENT_SCOPE) + # Add to the default vars list so it propagates to all subprojects. + set_property(GLOBAL APPEND PROPERTY THEROCK_DEFAULT_CMAKE_VARS ${_var_name}) + endforeach() + + # Process GLOBAL_CPP_DEFINES. + get_property(_global_cpp_defines GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_GLOBAL_CPP_DEFINES) + foreach(_define ${_global_cpp_defines}) + set_property(GLOBAL APPEND PROPERTY THEROCK_FLAG_GLOBAL_CPP_DEFINES "${_define}") + endforeach() + + # Process per-subproject CMAKE_VARS and CPP_DEFINES. + get_property(_cmake_vars GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_CMAKE_VARS) + get_property(_cpp_defines GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_CPP_DEFINES) + get_property(_sub_projects GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_SUB_PROJECTS) + + foreach(_subproject ${_sub_projects}) + foreach(_var_pair ${_cmake_vars}) + set_property(GLOBAL APPEND PROPERTY + _THEROCK_SUBPROJECT_FLAG_CMAKE_VARS_${_subproject} "${_var_pair}") + endforeach() + foreach(_define ${_cpp_defines}) + set_property(GLOBAL APPEND PROPERTY + _THEROCK_SUBPROJECT_FLAG_CPP_DEFINES_${_subproject} "${_define}") + endforeach() + endforeach() + endforeach() + + # Generate flag_settings.json in the build directory. + list(JOIN _json_entries ",\n " _json_body) + set(_json_content "{\n ${_json_body}\n}\n") + set(_flag_settings_file "${THEROCK_BINARY_DIR}/flag_settings.json") + file(WRITE "${_flag_settings_file}" "${_json_content}") + set(THEROCK_FLAG_SETTINGS_FILE "${_flag_settings_file}" PARENT_SCOPE) +endfunction() + +# therock_report_flags +# Reports the status of all declared flags at the end of configure. +function(therock_report_flags) + get_property(_all_flags GLOBAL PROPERTY THEROCK_ALL_FLAGS) + if(NOT _all_flags) + return() + endif() + + message(STATUS "Build flags:") + foreach(_flag_name ${_all_flags}) + if(THEROCK_FLAG_${_flag_name}) + message(STATUS " * ${_flag_name} = ON (-DTHEROCK_FLAG_${_flag_name}=ON)") + else() + message(STATUS " * ${_flag_name} = OFF (-DTHEROCK_FLAG_${_flag_name}=OFF)") + endif() + endforeach() +endfunction() + +# _therock_get_flag_init_contents +# Internal function called from therock_cmake_subproject_activate() to get +# flag-injected content for a specific subproject's project_init.cmake. +# Sets ${out_var} in PARENT_SCOPE with the content to append. +function(_therock_get_flag_init_contents out_var target_name) + set(_contents "") + + # Global CPP defines (apply to ALL subprojects). + get_property(_global_cpp_defines GLOBAL PROPERTY THEROCK_FLAG_GLOBAL_CPP_DEFINES) + if(_global_cpp_defines) + string(APPEND _contents "\n# Flag system: global CPP defines\n") + foreach(_define ${_global_cpp_defines}) + string(APPEND _contents "add_compile_definitions(${_define})\n") + endforeach() + endif() + + # Per-subproject CMAKE_VARS. + get_property(_has_cmake_vars GLOBAL PROPERTY _THEROCK_SUBPROJECT_FLAG_CMAKE_VARS_${target_name} SET) + if(_has_cmake_vars) + get_property(_cmake_vars GLOBAL PROPERTY _THEROCK_SUBPROJECT_FLAG_CMAKE_VARS_${target_name}) + if(_cmake_vars) + string(APPEND _contents "\n# Flag system: per-subproject CMAKE vars\n") + foreach(_var_pair ${_cmake_vars}) + string(FIND "${_var_pair}" "=" _eq_pos) + string(SUBSTRING "${_var_pair}" 0 ${_eq_pos} _var_name) + math(EXPR _val_start "${_eq_pos} + 1") + string(SUBSTRING "${_var_pair}" ${_val_start} -1 _var_value) + string(APPEND _contents "set(${_var_name} \"${_var_value}\" CACHE STRING \"\" FORCE)\n") + endforeach() + endif() + endif() + + # Per-subproject CPP defines. + get_property(_has_cpp_defines GLOBAL PROPERTY _THEROCK_SUBPROJECT_FLAG_CPP_DEFINES_${target_name} SET) + if(_has_cpp_defines) + get_property(_cpp_defines GLOBAL PROPERTY _THEROCK_SUBPROJECT_FLAG_CPP_DEFINES_${target_name}) + if(_cpp_defines) + string(APPEND _contents "\n# Flag system: per-subproject CPP defines\n") + foreach(_define ${_cpp_defines}) + string(APPEND _contents "add_compile_definitions(${_define})\n") + endforeach() + endif() + endif() + + set(${out_var} "${_contents}" PARENT_SCOPE) +endfunction() diff --git a/cmake/therock_subproject.cmake b/cmake/therock_subproject.cmake index 901317501ea..3985c8729ae 100644 --- a/cmake/therock_subproject.cmake +++ b/cmake/therock_subproject.cmake @@ -847,6 +847,11 @@ function(therock_cmake_subproject_activate target_name) endif() string(APPEND _init_contents "include(\"${_addl_cmake_include}\")\n") endforeach() + + # Flag system: inject flag-controlled variables and defines. + _therock_get_flag_init_contents(_flag_init_contents "${target_name}") + string(APPEND _init_contents "${_flag_init_contents}") + file(CONFIGURE OUTPUT "${_cmake_project_init_file}" CONTENT "${_init_contents}" @ONLY ESCAPE_QUOTES) list(APPEND _fprint_files "${_cmake_project_init_file}") diff --git a/core/CMakeLists.txt b/core/CMakeLists.txt index 2f22bef4908..8c3b7f5ee0e 100644 --- a/core/CMakeLists.txt +++ b/core/CMakeLists.txt @@ -162,12 +162,12 @@ if(THEROCK_ENABLE_HIP_RUNTIME) ) endif() - # Conditional rocm-kpack support for kpack split artifacts + # Conditional rocm-kpack support for kpack split artifacts. + # Note: ROCM_KPACK_ENABLED is injected by the flag system (see FLAGS.cmake). + # Only the runtime dep needs explicit handling here. set(_hip_clr_kpack_runtime_deps) - set(_hip_clr_kpack_cmake_args) - if(THEROCK_KPACK_SPLIT_ARTIFACTS) + if(THEROCK_FLAG_KPACK_SPLIT_ARTIFACTS) list(APPEND _hip_clr_kpack_runtime_deps rocm-kpack) - list(APPEND _hip_clr_kpack_cmake_args "-DROCM_KPACK_ENABLED=ON") endif() therock_cmake_subproject_declare(hip-clr @@ -187,7 +187,6 @@ if(THEROCK_ENABLE_HIP_RUNTIME) # and can use local machine tools. "-DHIPCC_BIN_DIR=" ${HIP_CLR_CMAKE_ARGS} - ${_hip_clr_kpack_cmake_args} BUILD_DEPS rocm-cmake therock-simde diff --git a/docs/development/flags.md b/docs/development/flags.md new file mode 100644 index 00000000000..f404265a5a4 --- /dev/null +++ b/docs/development/flags.md @@ -0,0 +1,160 @@ +# Build Flags + +Build flags are system-wide controls that affect how TheRock subprojects are +configured. Each flag creates a `THEROCK_FLAG_{NAME}` CMake cache variable and +can optionally propagate CMake variables and C preprocessor defines to all or +specific subprojects. + +## Flags vs Features + +| Concept | Purpose | Naming | +|---------|---------|--------| +| **Features** (`therock_features.cmake`) | Control which subprojects are included in the build | `THEROCK_ENABLE_{NAME}` | +| **Flags** (`FLAGS.cmake`) | Control *how* included subprojects are configured | `THEROCK_FLAG_{NAME}` | + +Features are about "what to build". Flags are about "how to build it". + +## Architecture + +``` +FLAGS.cmake Central declarations (project root) + └── therock_declare_flag() → THEROCK_FLAG_{NAME} cache var + └── BRANCH_FLAGS.cmake → Optional per-branch default overrides + └── therock_finalize_flags() → Propagation data + flag_settings.json + └── therock_report_flags() → Status output + +cmake/therock_flag_utils.cmake Processing functions +cmake/therock_subproject.cmake Injection via project_init.cmake +``` + +### Propagation Mechanism + +When a flag is enabled, its effects are injected into subprojects via the +generated `project_init.cmake` files (the same mechanism used for +`THEROCK_DEFAULT_CMAKE_VARS`): + +- **GLOBAL_CMAKE_VARS**: `VAR=VALUE` pairs set in the super-project and + propagated to **all** subprojects via `THEROCK_DEFAULT_CMAKE_VARS`. +- **GLOBAL_CPP_DEFINES**: Preprocessor defines added to **all** subprojects + via `add_compile_definitions()` in project_init.cmake. +- **CMAKE_VARS**: `VAR=VALUE` pairs injected only into the listed + **SUB_PROJECTS** via `set(VAR VALUE CACHE STRING "" FORCE)`. +- **CPP_DEFINES**: Preprocessor defines added only to the listed + **SUB_PROJECTS** via `add_compile_definitions()`. + +Structural concerns (conditional subproject inclusion, runtime dependency +wiring) remain as explicit conditionals in the consuming CMakeLists.txt files. +Flags do not auto-include subprojects. + +## Declaring a Flag + +All flags are declared in `FLAGS.cmake` at the project root: + +```cmake +therock_declare_flag( + NAME KPACK_SPLIT_ARTIFACTS + DEFAULT_VALUE OFF + DESCRIPTION "Split target-specific artifacts into generic and arch-specific components" + ISSUE "https://github.com/ROCm/TheRock/issues/3448" + CMAKE_VARS + ROCM_KPACK_ENABLED=ON + SUB_PROJECTS + hip-clr +) +``` + +### Parameters + +| Parameter | Required | Description | +|-----------|----------|-------------| +| `NAME` | Yes | Unique identifier. Creates `THEROCK_FLAG_{NAME}` cache variable. | +| `DEFAULT_VALUE` | Yes | `ON` or `OFF`. | +| `DESCRIPTION` | Yes | Short description shown in CMake cache UI. | +| `ISSUE` | No | Tracking issue URL. | +| `GLOBAL_CMAKE_VARS` | No | `VAR=VALUE` pairs for all subprojects. | +| `GLOBAL_CPP_DEFINES` | No | Preprocessor defines for all subprojects. | +| `CMAKE_VARS` | No | `VAR=VALUE` pairs for listed `SUB_PROJECTS` only. | +| `CPP_DEFINES` | No | Preprocessor defines for listed `SUB_PROJECTS` only. | +| `SUB_PROJECTS` | No* | Target names for scoped `CMAKE_VARS`/`CPP_DEFINES`. *Required if either is set. | + +### Using a Flag in CMakeLists.txt + +Flags are regular CMake cache variables, so consuming code uses them directly: + +```cmake +if(THEROCK_FLAG_KPACK_SPLIT_ARTIFACTS) + # Conditional subproject inclusion, dependency wiring, etc. +endif() +``` + +## Branch Flag Overrides + +Integration branches can change flag defaults by creating a +`BRANCH_FLAGS.cmake` file in the project root: + +```cmake +# BRANCH_FLAGS.cmake +# Override defaults for the kpack-integration branch. +therock_override_flag_default(KPACK_SPLIT_ARTIFACTS ON) +``` + +`BRANCH_FLAGS.cmake` is `.gitignore`d on main but can be committed on +integration branches. Overrides are logged to the configure output so they are +visible in CI. + +Explicit `-D` flags on the cmake command line always take precedence over +branch overrides. + +## Manifest Integration + +Flag states are recorded in the TheRock manifest (`share/therock/therock_manifest.json`) +under a `"flags"` key: + +```json +{ + "the_rock_commit": "abc123...", + "submodules": [...], + "flags": { + "KPACK_SPLIT_ARTIFACTS": false + } +} +``` + +This is generated automatically: `therock_finalize_flags()` writes +`flag_settings.json` to the build directory, which is passed to +`generate_therock_manifest.py` via the aux-overlay subproject. + +## Adding a New Flag + +1. Add a `therock_declare_flag()` call in `FLAGS.cmake`. +2. Use `THEROCK_FLAG_{NAME}` in the relevant CMakeLists.txt files for + structural decisions (conditional subproject inclusion, dependency wiring). +3. If the flag needs to set variables or defines in subprojects, use the + `CMAKE_VARS`, `CPP_DEFINES`, `GLOBAL_CMAKE_VARS`, or `GLOBAL_CPP_DEFINES` + parameters to automate propagation. +4. Run cmake configure and verify the flag report output and, if applicable, + inspect the generated `project_init.cmake` files. + +## Alternatives Considered + +### Plumbing individual flags to subprojects via CMAKE_ARGS + +Before the flag system, each flag's effects were manually forwarded to +subprojects in their `therock_cmake_subproject_declare()` calls. For example, +`THEROCK_KPACK_SPLIT_ARTIFACTS` required manual `-DROCM_KPACK_ENABLED=ON` +forwarding to hip-clr. This approach doesn't scale and is error-prone: adding a +new flag requires modifying multiple declaration sites. + +### Plumbing flags to the manifest generator individually + +For manifest integration, each flag could be passed as its own CMake variable to +the aux-overlay subproject, then read by `generate_therock_manifest.py`. This +was rejected in favor of generating a single `flag_settings.json` file that is +splat into the manifest, avoiding per-flag plumbing. + +### Merging flags into the feature system + +Flags could be added as a new mode in `therock_features.cmake`. However, +features and flags serve fundamentally different purposes (inclusion vs +configuration), and mixing them would complicate the feature dependency +resolution logic. From 20c2e4f218dab9cb9954b6dd6f72cc7291e58dcd Mon Sep 17 00:00:00 2001 From: Stella Laurenzo Date: Fri, 6 Mar 2026 17:17:19 -0800 Subject: [PATCH 2/2] Defer all cache/global manipulation to therock_finalize_flags therock_declare_flag() now only stores metadata in global properties (including the default value). therock_override_flag_default() only updates the stored default property. All cache variable creation and global state manipulation happens in therock_finalize_flags(), avoiding set-ordering issues between declare and override calls. Co-Authored-By: Claude --- cmake/therock_flag_utils.cmake | 39 +++++++++++++++++----------------- docs/development/flags.md | 34 ++++++++++++++--------------- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/cmake/therock_flag_utils.cmake b/cmake/therock_flag_utils.cmake index 0fb9d9b32d0..7ef3553fbbe 100644 --- a/cmake/therock_flag_utils.cmake +++ b/cmake/therock_flag_utils.cmake @@ -70,13 +70,12 @@ function(therock_declare_flag) ) endif() - # Create cache variable. - set(THEROCK_FLAG_${ARG_NAME} "${ARG_DEFAULT_VALUE}" CACHE BOOL "${ARG_DESCRIPTION}") - - # Register the flag. + # Register the flag (metadata only — no cache/global manipulation here). + # All cache variables and global state are created in therock_finalize_flags(). set_property(GLOBAL APPEND PROPERTY THEROCK_ALL_FLAGS "${ARG_NAME}") # Store flag metadata in global properties for later retrieval. + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_DEFAULT_VALUE "${ARG_DEFAULT_VALUE}") set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_DESCRIPTION "${ARG_DESCRIPTION}") set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_GLOBAL_CMAKE_VARS "${ARG_GLOBAL_CMAKE_VARS}") set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_GLOBAL_CPP_DEFINES "${ARG_GLOBAL_CPP_DEFINES}") @@ -86,15 +85,13 @@ function(therock_declare_flag) if(ARG_ISSUE) set_property(GLOBAL PROPERTY _THEROCK_FLAG_${ARG_NAME}_ISSUE "${ARG_ISSUE}") endif() - - # Propagate to parent scope so it's available at the calling CMakeLists.txt level. - set(THEROCK_FLAG_${ARG_NAME} "${THEROCK_FLAG_${ARG_NAME}}" PARENT_SCOPE) endfunction() # therock_override_flag_default -# Changes the default value of a previously declared flag. Intended for use in -# BRANCH_FLAGS.cmake on integration branches. If the user has explicitly set the -# cache variable via -D, their value takes precedence. +# Changes the default value of a previously declared flag. Only updates the +# stored default property — actual cache variable creation happens in +# therock_finalize_flags(). Intended for use in BRANCH_FLAGS.cmake on +# integration branches. function(therock_override_flag_default flag_name new_default) get_property(_all_flags GLOBAL PROPERTY THEROCK_ALL_FLAGS) if(NOT "${flag_name}" IN_LIST _all_flags) @@ -103,15 +100,8 @@ function(therock_override_flag_default flag_name new_default) ) endif() - # Only override if the variable was not explicitly set by the user on the - # command line. CMake sets the CACHE property type for -D variables, but - # there's no direct way to check "was this set by the user". We use the - # CMAKE_CACHE_ARGS approach: if current value equals the original default, - # it was likely not overridden by the user. However, the most reliable - # approach is to just force-set it and let -D on reconfigure win. message(STATUS "Flag ${flag_name} default overridden to ${new_default}") - set(THEROCK_FLAG_${flag_name} "${new_default}" CACHE BOOL "" FORCE) - set(THEROCK_FLAG_${flag_name} "${new_default}" PARENT_SCOPE) + set_property(GLOBAL PROPERTY _THEROCK_FLAG_${flag_name}_DEFAULT_VALUE "${new_default}") endfunction() # therock_finalize_flags @@ -122,7 +112,18 @@ endfunction() function(therock_finalize_flags) get_property(_all_flags GLOBAL PROPERTY THEROCK_ALL_FLAGS) - # Build JSON content for flag_settings.json. + # Phase 1: Create cache variables from stored defaults. + # This is the single place where THEROCK_FLAG_* cache vars are created, + # ensuring no set-ordering issues between declare and override. + foreach(_flag_name ${_all_flags}) + get_property(_default GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_DEFAULT_VALUE) + get_property(_description GLOBAL PROPERTY _THEROCK_FLAG_${_flag_name}_DESCRIPTION) + set(THEROCK_FLAG_${_flag_name} "${_default}" CACHE BOOL "${_description}") + # Propagate the (possibly user-overridden) cache value to the caller's scope. + set(THEROCK_FLAG_${_flag_name} "${THEROCK_FLAG_${_flag_name}}" PARENT_SCOPE) + endforeach() + + # Phase 2: Process enabled flags and build JSON. set(_json_entries) foreach(_flag_name ${_all_flags}) diff --git a/docs/development/flags.md b/docs/development/flags.md index f404265a5a4..22bbb575df1 100644 --- a/docs/development/flags.md +++ b/docs/development/flags.md @@ -7,10 +7,10 @@ specific subprojects. ## Flags vs Features -| Concept | Purpose | Naming | -|---------|---------|--------| +| Concept | Purpose | Naming | +| --------------------------------------- | --------------------------------------------------- | ----------------------- | | **Features** (`therock_features.cmake`) | Control which subprojects are included in the build | `THEROCK_ENABLE_{NAME}` | -| **Flags** (`FLAGS.cmake`) | Control *how* included subprojects are configured | `THEROCK_FLAG_{NAME}` | +| **Flags** (`FLAGS.cmake`) | Control *how* included subprojects are configured | `THEROCK_FLAG_{NAME}` | Features are about "what to build". Flags are about "how to build it". @@ -65,17 +65,17 @@ therock_declare_flag( ### Parameters -| Parameter | Required | Description | -|-----------|----------|-------------| -| `NAME` | Yes | Unique identifier. Creates `THEROCK_FLAG_{NAME}` cache variable. | -| `DEFAULT_VALUE` | Yes | `ON` or `OFF`. | -| `DESCRIPTION` | Yes | Short description shown in CMake cache UI. | -| `ISSUE` | No | Tracking issue URL. | -| `GLOBAL_CMAKE_VARS` | No | `VAR=VALUE` pairs for all subprojects. | -| `GLOBAL_CPP_DEFINES` | No | Preprocessor defines for all subprojects. | -| `CMAKE_VARS` | No | `VAR=VALUE` pairs for listed `SUB_PROJECTS` only. | -| `CPP_DEFINES` | No | Preprocessor defines for listed `SUB_PROJECTS` only. | -| `SUB_PROJECTS` | No* | Target names for scoped `CMAKE_VARS`/`CPP_DEFINES`. *Required if either is set. | +| Parameter | Required | Description | +| -------------------- | -------- | -------------------------------------------------------------------------------- | +| `NAME` | Yes | Unique identifier. Creates `THEROCK_FLAG_{NAME}` cache variable. | +| `DEFAULT_VALUE` | Yes | `ON` or `OFF`. | +| `DESCRIPTION` | Yes | Short description shown in CMake cache UI. | +| `ISSUE` | No | Tracking issue URL. | +| `GLOBAL_CMAKE_VARS` | No | `VAR=VALUE` pairs for all subprojects. | +| `GLOBAL_CPP_DEFINES` | No | Preprocessor defines for all subprojects. | +| `CMAKE_VARS` | No | `VAR=VALUE` pairs for listed `SUB_PROJECTS` only. | +| `CPP_DEFINES` | No | Preprocessor defines for listed `SUB_PROJECTS` only. | +| `SUB_PROJECTS` | No\* | Target names for scoped `CMAKE_VARS`/`CPP_DEFINES`. \*Required if either is set. | ### Using a Flag in CMakeLists.txt @@ -127,12 +127,12 @@ This is generated automatically: `therock_finalize_flags()` writes ## Adding a New Flag 1. Add a `therock_declare_flag()` call in `FLAGS.cmake`. -2. Use `THEROCK_FLAG_{NAME}` in the relevant CMakeLists.txt files for +1. Use `THEROCK_FLAG_{NAME}` in the relevant CMakeLists.txt files for structural decisions (conditional subproject inclusion, dependency wiring). -3. If the flag needs to set variables or defines in subprojects, use the +1. If the flag needs to set variables or defines in subprojects, use the `CMAKE_VARS`, `CPP_DEFINES`, `GLOBAL_CMAKE_VARS`, or `GLOBAL_CPP_DEFINES` parameters to automate propagation. -4. Run cmake configure and verify the flag report output and, if applicable, +1. Run cmake configure and verify the flag report output and, if applicable, inspect the generated `project_init.cmake` files. ## Alternatives Considered