Do not retain a name string for generated module maps Copybara Import from https://github.com/bazelbuild/rules_cc/pull/789 BEGIN_PUBLIC Do not retain a name string for generated module maps (#789) Module maps generated for a target are named after the target's label. Since the generated module map file is declared by that very target, the name can be derived on demand from the file's owner label, which the `File` object already retains. The `name` field of the module map struct becomes optional, with `None` meaning "named after the label of the target that generated the module map file", and two central helpers (`get_module_map_name`, `get_module_map_label`) derive the string form and the `Label` form where needed. This fixes the round-trip bug originally addressed in https://github.com/bazelbuild/rules_cc/pull/633, but avoids the memory regression that caused it to be rolled back in b81b6e4 (because retaining `Label` objects increased memory usage overall). Deriving the name on demand retains nothing extra as the owner label is already retained by the `File` and all name materializations are transient only (such as in `map_each` callbacks). Names are now the unambiguous canonical label string, except that main repository labels drop the leading `@@` (`//pkg:lib`), keeping the traditional module name format. Special module maps (the crosstool module map, ObjC internal module maps) keep their explicitly provided names. Closes #789 END_PUBLIC Startblock: * // Put other blockers before this line to avoid churning. * has lgtm * is approved * and then * all comments are resolved * and then *[] allows unknown commit COPYBARA_INTEGRATE_REVIEW=https://github.com/bazelbuild/rules_cc/pull/789 from fmeum:module-map-label 68848659e21ef3b4d223a92dc19b4711b604b82e PiperOrigin-RevId: 964972527 Change-Id: If97a7c4491d670e825b621e80541a064ffe8769f
diff --git a/cc/private/cc_info.bzl b/cc/private/cc_info.bzl index a51d0f4..3ce9824 100644 --- a/cc/private/cc_info.bzl +++ b/cc/private/cc_info.bzl
@@ -152,23 +152,91 @@ "ModuleMapInfo", fields = { "file": "The module map file.", - "name": "The name of the module.", + "name": "The name of the module or None if the module is named after the label of the target that generated the module map file (see get_module_map_name).", }, ) -def create_module_map(*, file, name): +def create_module_map(*, file, name = None): """ Creates a module map struct. Args: file: The module map file. - name: The name of the module. + name: The name of the module. If None (the default), the module is + named after the label of the target that generated the module map + file. Prefer the default: it avoids retaining a name string per + module map as the name is derived on demand from `file.owner`. Returns: A module map struct. """ check_private_api() return _ModuleMapInfo(file = file, name = name) +def module_map_name_for_label(label): + """ + Returns the module name for a module map named after the given label. + + The name is the label's unambiguous canonical form, except that + main-repository labels do not carry the leading double-at prefix (see + get_module_map_name). + + Args: + label: The Label the module is named after. + Returns: + The name of the module as a string. + """ + label_string = str(label) + + # buildifier: disable=canonical-repository + if label_string.startswith("@@//"): + return label_string[2:] + return label_string + +def get_module_map_name(module_map): + """ + Returns the name of the module described by a module map struct. + + Module maps generated for a target are named after the target's label, + rendered in its unambiguous canonical form, except that main-repository + labels do not carry the leading double-at prefix (for example "//pkg:lib" + for a main-repository target). Names of targets in external repositories + are thus valid label strings, while main-repository names match the + traditional module name format. + + Args: + module_map: The module map struct. + Returns: + The name of the module as a string. + """ + if module_map.name != None: + return module_map.name + return module_map_name_for_label(module_map.file.owner) + +def get_module_map_label(module_map): + """ + Returns the label a module map struct is named after. + + This is only valid for module maps whose name is (derived from) a label, + which is the case for all module maps generated for a target, including + separate module maps. It is not valid for module maps with special names + such as the crosstool module map. + + Args: + module_map: The module map struct. + Returns: + The Label the module's name refers to. + """ + if module_map.name == None: + return module_map.file.owner + + # Module map names of main-repository targets lack the leading "@@" (see + # get_module_map_name), which has to be restored for parsing to ensure + # that the name is not resolved relative to this file's repository. + name = module_map.name + + # buildifier: disable=canonical-repository + return Label(name if name.startswith("@") else "@@" + name) + def create_separate_module_map(module_map): """ Creates a separate module map struct. @@ -178,7 +246,7 @@ Returns: A module map struct. """ - return _ModuleMapInfo(file = module_map.file, name = module_map.name + ".sep") + return _ModuleMapInfo(file = module_map.file, name = get_module_map_name(module_map) + ".sep") def create_linking_context( *,
diff --git a/cc/private/compile/cc_compilation_helper.bzl b/cc/private/compile/cc_compilation_helper.bzl index a30397d..5dc19cc 100644 --- a/cc/private/compile/cc_compilation_helper.bzl +++ b/cc/private/compile/cc_compilation_helper.bzl
@@ -21,7 +21,7 @@ "repository_exec_path", ) load("//cc/common:semantics.bzl", "STRIP_INCLUDE_PREFIX_APPLIES_TO_TEXTUAL_HEADERS", "USE_EXEC_ROOT_FOR_VIRTUAL_INCLUDES_SYMLINKS") -load("//cc/private:cc_info.bzl", "create_compilation_context", "create_module_map") +load("//cc/private:cc_info.bzl", "create_compilation_context", "create_module_map", "get_module_map_name", "module_map_name_for_label") load("//cc/private:cc_internal.bzl", _cc_internal = "cc_internal") _VIRTUAL_INCLUDES_DIR = "_virtual_includes" @@ -254,7 +254,8 @@ def _module_map_struct_to_module_map_content(parameters, tree_expander): lines = [] module_map = parameters.module_map - lines.append("module \"%s\" {" % module_map.name) + module_name = get_module_map_name(module_map) + lines.append("module \"%s\" {" % module_name) lines.append(" export *") def expanded(artifacts): @@ -326,10 +327,10 @@ dependency_module_maps = parameters.dependency_module_maps.to_list() for dep in dependency_module_maps: - lines.append(" use \"" + dep.name + "\"") + lines.append(" use \"" + get_module_map_name(dep) + "\"") if parameters.separate_module_headers: - separate_name = module_map.name + ".sep" + separate_name = module_name + ".sep" lines.append(" use \"" + separate_name + "\"") lines.append("}") lines.append("module \"" + separate_name + "\" {") @@ -343,14 +344,14 @@ added_paths.add(header.path) for dep in dependency_module_maps: - lines.append(" use \"" + dep.name + "\"") + lines.append(" use \"" + get_module_map_name(dep) + "\"") lines.append("}") if parameters.extern_dependencies: for dep in dependency_module_maps: lines.append( - "extern module \"" + dep.name + "\" \"" + + "extern module \"" + get_module_map_name(dep) + "\" \"" + parameters.leading_periods + dep.file.path + "\"", ) @@ -566,10 +567,14 @@ header_module = None if _enabled(feature_configuration, "module_maps"): if not module_map: - module_map = create_module_map( - file = actions.declare_file(label.name + ".cppmap"), - name = label.workspace_name + "//" + label.package + ":" + label.name, - ) + file = actions.declare_file(label.name + ".cppmap") + + # If compile() is called with a name that differs from the + # declaring target's name (e.g. by an aspect), the module name + # cannot be derived from the file's owner and has to be stored + # explicitly. + name = None if file.owner == label else module_map_name_for_label(label) + module_map = create_module_map(file = file, name = name) # There are different modes for module compilation: # 1. We create the module map and compile the module so that libraries depending on us can
diff --git a/cc/private/compile/compile.bzl b/cc/private/compile/compile.bzl index 55b9a69..5ea1727 100644 --- a/cc/private/compile/compile.bzl +++ b/cc/private/compile/compile.bzl
@@ -36,6 +36,8 @@ "create_cc_compilation_context_with_cpp20_modules", "create_compilation_context_with_extra_header_tokens", "create_separate_module_map", + "get_module_map_label", + "get_module_map_name", ) load("//cc/private:cc_internal.bzl", _cc_internal = "cc_internal") load("//cc/private/compile:cc_compilation_helper.bzl", "cc_compilation_helper", "dotd_files_enabled", "serialized_diagnostics_file_enabled") @@ -545,13 +547,10 @@ ), ) specific_compile_build_variables = get_specific_compile_build_variables( - feature_configuration, use_pic = use_pic, source_file = source_artifact, output_file = ddi_file, dotd_file = dotd_file, - cpp_module_map = cc_compilation_context._module_map, - direct_module_maps = cc_compilation_context._direct_module_maps, user_compile_flags = get_copts( language = language, cpp_configuration = cpp_configuration, @@ -848,7 +847,6 @@ common_compile_variables = common_compile_build_variables, fdo_build_variables = fdo_build_variables, output_category = artifact_category.CLIF_OUTPUT_PROTO if cpp_source.type == CPP_SOURCE_TYPE_CLIF_INPUT_PROTO else artifact_category.OBJECT_FILE, - cpp_module_map = cc_compilation_context._module_map, add_object = True, enable_coverage = is_code_coverage_enabled, generate_dwo = should_create_per_object_debug_info(feature_configuration, cpp_configuration), @@ -996,7 +994,6 @@ common_compile_variables = common_compile_build_variables, fdo_build_variables = fdo_build_variables, output_category = artifact_category.CLIF_OUTPUT_PROTO if cpp_source.type == CPP_SOURCE_TYPE_CLIF_INPUT_PROTO else artifact_category.OBJECT_FILE, - cpp_module_map = cc_compilation_context._module_map, add_object = True, enable_coverage = is_code_coverage_enabled, generate_dwo = should_create_per_object_debug_info(feature_configuration, cpp_configuration), @@ -1167,7 +1164,7 @@ if _should_provide_header_modules(feature_configuration, private_headers, public_headers): cpp_module_map = cc_compilation_context._module_map - module_map_label = Label(cpp_module_map.name) + module_map_label = get_module_map_label(cpp_module_map) modules = _create_module_action( action_construction_context = action_construction_context, cc_compilation_context = cc_compilation_context, @@ -1213,6 +1210,7 @@ fdo_build_variables = fdo_build_variables, outputs = outputs, cpp_module_map = separate_cpp_module_map, + module_name = get_module_map_name(separate_cpp_module_map), language = language, additional_compilation_inputs = [], additional_include_scanning_roots = [], @@ -1279,7 +1277,6 @@ common_compile_variables = common_compile_build_variables, fdo_build_variables = fdo_build_variables, output_category = (artifact_category.CLIF_OUTPUT_PROTO if source_type == CPP_SOURCE_TYPE_CLIF_INPUT_PROTO else artifact_category.OBJECT_FILE), - cpp_module_map = cc_compilation_context._module_map, add_object = True, enable_coverage = is_code_coverage_enabled, generate_dwo = should_create_per_object_debug_info(feature_configuration, cpp_configuration), @@ -1366,14 +1363,11 @@ ), ) if serialized_diagnostics_file_enabled(feature_configuration) else None specific_compile_build_variables = get_specific_compile_build_variables( - feature_configuration, use_pic = generate_pic_action, source_file = source_file, output_file = output_file, dotd_file = dotd_file, diagnostics_file = diagnostics_file, - cpp_module_map = cc_compilation_context._module_map, - direct_module_maps = cc_compilation_context._direct_module_maps, user_compile_flags = get_copts( language = language, cpp_configuration = cpp_configuration, @@ -1431,7 +1425,6 @@ common_compile_variables, fdo_build_variables, output_category, - cpp_module_map, add_object, enable_coverage, generate_dwo, @@ -1443,7 +1436,8 @@ generate_pic_action, generate_no_pic_action, enable_dotd_files, - progress_message_prefix): + progress_message_prefix, + module_name = None): results = [] if generate_pic_action: pic_object = _create_compile_source_action( @@ -1465,7 +1459,7 @@ common_compile_variables = common_compile_variables, fdo_build_variables = fdo_build_variables, output_category = output_category, - cpp_module_map = cpp_module_map, + module_name = module_name, add_object = add_object, enable_coverage = enable_coverage, generate_dwo = generate_dwo, @@ -1502,7 +1496,7 @@ common_compile_variables = common_compile_variables, fdo_build_variables = fdo_build_variables, output_category = output_category, - cpp_module_map = cpp_module_map, + module_name = module_name, add_object = add_object, enable_coverage = enable_coverage, generate_dwo = generate_dwo, @@ -1540,7 +1534,6 @@ common_compile_variables, fdo_build_variables, output_category, - cpp_module_map, add_object, enable_coverage, generate_dwo, @@ -1551,6 +1544,7 @@ additional_include_scanning_roots, use_pic, enable_dotd_files, + module_name = None, additional_build_variables = {}, action_name = None, additional_outputs = [], @@ -1661,9 +1655,7 @@ dotd_file = dotd_file, diagnostics_file = diagnostics_file, use_pic = use_pic, - cpp_module_map = cpp_module_map, - feature_configuration = feature_configuration, - direct_module_maps = cc_compilation_context._direct_module_maps, + module_name = module_name, fdo_build_variables = fdo_build_variables, additional_build_variables = additional_build_variables, ) @@ -1885,9 +1877,6 @@ dotd_file = preprocess_dotd_file, diagnostics_file = preprocess_diagnostics_file, use_pic = use_pic, - cpp_module_map = cc_compilation_context._module_map, - direct_module_maps = cc_compilation_context._direct_module_maps, - feature_configuration = feature_configuration, fdo_build_variables = fdo_build_variables, additional_build_variables = {"output_preprocess_file": preprocess_object_file.path}, ) @@ -1904,9 +1893,6 @@ dotd_file = assembly_dotd_file, diagnostics_file = assembly_diagnostics_file, use_pic = use_pic, - cpp_module_map = cc_compilation_context._module_map, - direct_module_maps = cc_compilation_context._direct_module_maps, - feature_configuration = feature_configuration, fdo_build_variables = fdo_build_variables, additional_build_variables = {"output_assembly_file": assembly_object_file.path}, ) @@ -2069,9 +2055,6 @@ dotd_file = dotd_file, diagnostics_file = diagnostics_file, use_pic = use_pic, - cpp_module_map = cc_compilation_context._module_map, - feature_configuration = feature_configuration, - direct_module_maps = cc_compilation_context._direct_module_maps, fdo_build_variables = fdo_build_variables, additional_build_variables = {}, ) @@ -2142,8 +2125,9 @@ additional_compilation_inputs, additional_include_scanning_roots, outputs, - progress_message_prefix): - module_map_label = Label(cpp_module_map.name) + progress_message_prefix, + module_name = None): + module_map_label = get_module_map_label(cpp_module_map) return _create_pic_nopic_compile_source_actions( action_construction_context = action_construction_context, cc_compilation_context = cc_compilation_context, @@ -2167,7 +2151,7 @@ source_artifact = cpp_module_map.file, language = language, output_category = artifact_category.CPP_MODULE, - cpp_module_map = cpp_module_map, + module_name = module_name, add_object = False, enable_coverage = False, generate_dwo = False,
diff --git a/cc/private/compile/compile_action_templates.bzl b/cc/private/compile/compile_action_templates.bzl index b31efda..94d65cd 100644 --- a/cc/private/compile/compile_action_templates.bzl +++ b/cc/private/compile/compile_action_templates.bzl
@@ -170,12 +170,9 @@ use_pic = use_pic, ) specific_compile_build_variables = get_specific_compile_build_variables( - feature_configuration, use_pic = use_pic, source_file = source_dir, output_file = output_dir, - cpp_module_map = cc_compilation_context._module_map, - direct_module_maps = cc_compilation_context._direct_module_maps, user_compile_flags = all_copts, ) dotd_tree_artifact = _maybe_declare_dotd_tree_artifact(
diff --git a/cc/private/compile/compile_build_variables.bzl b/cc/private/compile/compile_build_variables.bzl index 4b3b8cf..7ca6959 100644 --- a/cc/private/compile/compile_build_variables.bzl +++ b/cc/private/compile/compile_build_variables.bzl
@@ -16,6 +16,7 @@ """ load("//cc/common:cc_helper_internal.bzl", "extensions", "get_fdo_build_stamp", "get_linkstamp_stamps", _PRIVATE_STARLARKIFICATION_ALLOWLIST = "PRIVATE_STARLARKIFICATION_ALLOWLIST") +load("//cc/private:cc_info.bzl", "get_module_map_name") load("//cc/private:cc_internal.bzl", _cc_internal = "cc_internal") load("//cc/private/rules_impl:native_cc_common.bzl", _cc_common_internal = "native_cc_common") @@ -173,7 +174,6 @@ additional_build_variables["input_file"] = input_file variables = get_specific_compile_build_variables( - feature_configuration, use_pic = use_pic, source_file = source_file, output_file = output_file, @@ -207,6 +207,8 @@ defines = cc_compilation_context.defines, local_defines = cc_compilation_context.local_defines, external_include_dirs = cc_compilation_context.external_includes, + cpp_module_map = cc_compilation_context._module_map if feature_configuration.is_enabled("module_maps") else None, + direct_module_maps = cc_compilation_context._direct_module_maps, ) return _cc_internal.combine_cc_toolchain_variables(cc_toolchain._build_variables, common_vars) @@ -225,9 +227,16 @@ framework_include_dirs = depset(), defines = depset(), local_defines = depset(), - external_include_dirs = depset()): + external_include_dirs = depset(), + cpp_module_map = None, + direct_module_maps = depset()): result = {} + if cpp_module_map: + result[_VARS.MODULE_NAME] = get_module_map_name(cpp_module_map) + result[_VARS.MODULE_MAP_FILE] = cpp_module_map.file + result[_VARS.DEPENDENT_MODULE_MAP_FILES] = direct_module_maps + if feature_configuration.is_enabled("use_header_modules"): result[_VARS.MODULE_FILES] = () if feature_configuration.is_requested("system_include_paths"): @@ -271,7 +280,6 @@ # Note: this method is side-effect free, callers should add fdo inputs to # cc_compile_action_builder themselves def get_specific_compile_build_variables( - feature_configuration, use_pic, source_file = None, output_file = None, @@ -285,15 +293,13 @@ thinlto_output_object_file = None, using_fission = False, code_coverage_enabled = False, - cpp_module_map = None, - direct_module_maps = depset(), + module_name = None, user_compile_flags = [], additional_build_variables = {}, fdo_build_variables = {}): """Creates a CcToolchainVariables instance Args: - feature_configuration: (FeatureConfiguration) use_pic: (bool) source_file: (File) output_file: (File) @@ -307,8 +313,8 @@ thinlto_output_object_file: (File) using_fission: (bool) code_coverage_enabled: (bool) - cpp_module_map: (File) - direct_module_maps: (depset[File]) + module_name: (str) Overrides the module name set by + setup_common_compile_build_variables. user_compile_flags: (list[str]) additional_build_variables: (dict{str,str}) fdo_build_variables: (dict{str,str}) @@ -318,10 +324,8 @@ """ result = {} - if feature_configuration.is_enabled("module_maps") and cpp_module_map: - result[_VARS.MODULE_NAME] = cpp_module_map.name - result[_VARS.MODULE_MAP_FILE] = cpp_module_map.file - result[_VARS.DEPENDENT_MODULE_MAP_FILES] = direct_module_maps + if module_name: + result[_VARS.MODULE_NAME] = module_name result[_VARS.USER_COMPILE_FLAGS] = _cc_internal.intern_string_sequence_variable_value(user_compile_flags) if source_file: