From 8ce95959d19334ce53963c6cbce3ac6eb40cd28c Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Mon, 24 Jan 2022 08:30:10 -0800 Subject: [PATCH] Copy `strict_includes` into `SwiftInfo` provider's Clang module descriptor in `swift_clang_module_aspect`. This makes it so that the `apple_common.Objc` provider no longer needs to be handled separately for compilation by Swift build APIs. (It is still used for linking until that is migrated entirely onto `CcInfo`.) PiperOrigin-RevId: 423822059 --- swift/internal/compiling.bzl | 160 +++++++------------ swift/internal/providers.bzl | 34 ++-- swift/internal/swift_clang_module_aspect.bzl | 27 +++- swift/internal/utils.bzl | 31 ++-- 4 files changed, 112 insertions(+), 140 deletions(-) diff --git a/swift/internal/compiling.bzl b/swift/internal/compiling.bzl index 9e8abcb98..cb5083797 100644 --- a/swift/internal/compiling.bzl +++ b/swift/internal/compiling.bzl @@ -818,17 +818,28 @@ def _c_layering_check_configurator(prerequisites, args): args.add("-Xcc", "-fmodules-strict-decluse") return None +def _clang_module_strict_includes(module_context): + """Returns the strict Clang include paths for a module context.""" + if not module_context.clang: + return None + strict_includes = module_context.clang.strict_includes + if not strict_includes: + return None + return strict_includes.to_list() + def _clang_search_paths_configurator(prerequisites, args): """Adds Clang search paths to the command line.""" args.add_all( - depset(transitive = [ - prerequisites.cc_compilation_context.includes, - # TODO(b/146575101): Replace with `objc_info.include` once this bug - # is fixed. See `_merge_target_providers` below for more details. - prerequisites.objc_include_paths_workaround, - ]), + prerequisites.cc_compilation_context.includes, + before_each = "-Xcc", + format_each = "-I%s", + ) + args.add_all( + prerequisites.transitive_modules, before_each = "-Xcc", format_each = "-I%s", + map_each = _clang_module_strict_includes, + uniquify = True, ) # Add Clang search paths for the workspace root and Bazel output roots. The @@ -876,7 +887,6 @@ def _collect_clang_module_inputs( cc_compilation_context, is_swift, modules, - objc_info, prefer_precompiled_modules): """Collects Clang module-related inputs to pass to an action. @@ -890,8 +900,6 @@ def _collect_clang_module_inputs( `swift_common.create_module`). The precompiled Clang modules or the textual module maps and headers of these modules (depending on the value of `prefer_precompiled_modules`) will be collected as inputs. - objc_info: The `apple_common.Objc` provider of the target being - compiled. prefer_precompiled_modules: If True, precompiled module artifacts should be preferred over textual module map files and headers for modules that have them. If False, textual module map files and headers @@ -905,37 +913,22 @@ def _collect_clang_module_inputs( direct_inputs = [] transitive_inputs = [] - if cc_compilation_context: - # The headers stored in the compilation context differ depending on the - # kind of action we're invoking: - if (is_swift and not prefer_precompiled_modules) or not is_swift: - # If this is a `SwiftCompile` with explicit modules disabled, the - # `headers` field is an already-computed set of the transitive - # headers of all the deps. (For an explicit module build, we skip it - # and will more selectively pick subsets for any individual modules - # that need to fallback to implicit modules in the loop below). - # - # If this is a `SwiftPrecompileCModule`, then by definition we're - # only here in a build with explicit modules enabled. We should only - # need the direct headers of the module being compiled and its - # direct dependencies (the latter because Clang needs them present - # on the file system to map them to the module that contains them.) - # However, we may also need some of the transitive headers, if the - # module has dependencies that aren't recognized as modules (e.g., - # `cc_library` targets without an aspect hint) and the module's - # headers include those. This will likely over-estimate the needed - # inputs, but we can't do better without include scanning in - # Starlark. - transitive_inputs.append(cc_compilation_context.headers) - transitive_inputs.append(depset(cc_compilation_context.direct_textual_headers)) - - # Some rules still use the `umbrella_header` field to propagate a header - # that they don't also include in `cc_compilation_context.headers`, so we - # also need to pull these in for the time being. - # TODO(b/142867898): This can be removed once the Swift rules start - # generating its own module map for these targets. - if objc_info: - transitive_inputs.append(objc_info.umbrella_header) + if cc_compilation_context and not is_swift: + # This is a `SwiftPrecompileCModule` action, so by definition we're + # only here in a build with explicit modules enabled. We should only + # need the direct headers of the module being compiled and its + # direct dependencies (the latter because Clang needs them present + # on the file system to map them to the module that contains them.) + # However, we may also need some of the transitive headers, if the + # module has dependencies that aren't recognized as modules (e.g., + # `cc_library` targets without an aspect hint) and the module's + # headers include those. This will likely over-estimate the needed + # inputs, but we can't do better without include scanning in + # Starlark. + transitive_inputs.append(cc_compilation_context.headers) + transitive_inputs.append( + depset(cc_compilation_context.direct_textual_headers), + ) for module in modules: clang_module = module.clang @@ -946,22 +939,24 @@ def _collect_clang_module_inputs( if not module.is_system and type(module_map) == "File": direct_inputs.append(module_map) - if prefer_precompiled_modules: - precompiled_module = clang_module.precompiled_module - if precompiled_module: - # For builds preferring explicit modules, use it if we have it - # and don't include any headers as inputs. - direct_inputs.append(precompiled_module) - else: - # If we don't have an explicit module, we need the transitive - # headers from the compilation context associated with the - # module. This will likely overestimate the headers that will - # actually be used in the action, but until we can use include - # scanning from Starlark, we can't compute a more precise input - # set. - transitive_inputs.append( - clang_module.compilation_context.headers, - ) + precompiled_module = clang_module.precompiled_module + + if prefer_precompiled_modules and precompiled_module: + # For builds preferring explicit modules, use it if we have it + # and don't include any headers as inputs. + direct_inputs.append(precompiled_module) + else: + # If we don't have an explicit module (or we're not using it), we + # need the transitive headers from the compilation context + # associated with the module. This will likely overestimate the + # headers that will actually be used in the action, but until we can + # use include scanning from Starlark, we can't compute a more + # precise input set. + compilation_context = clang_module.compilation_context + transitive_inputs.append(compilation_context.headers) + transitive_inputs.append( + depset(compilation_context.direct_textual_headers), + ) return swift_toolchain_config.config_result( inputs = direct_inputs, @@ -1046,7 +1041,6 @@ def _dependencies_clang_modulemaps_configurator(prerequisites, args): cc_compilation_context = prerequisites.cc_compilation_context, is_swift = prerequisites.is_swift, modules = modules, - objc_info = prerequisites.objc_info, prefer_precompiled_modules = False, ) @@ -1072,7 +1066,6 @@ def _dependencies_clang_modules_configurator(prerequisites, args): cc_compilation_context = prerequisites.cc_compilation_context, is_swift = prerequisites.is_swift, modules = modules, - objc_info = prerequisites.objc_info, prefer_precompiled_modules = True, ) @@ -1335,8 +1328,7 @@ def compile( deps: Non-private dependencies of the target being compiled. These targets are used as dependencies of both the Swift module being compiled and the Clang module for the generated header. These - targets must propagate one of the following providers: `CcInfo`, - `SwiftInfo`, or `apple_common.Objc`. + targets must propagate `CcInfo` or `SwiftInfo`. feature_configuration: A feature configuration obtained from `swift_common.configure_features`. generated_header_name: The name of the Objective-C generated header that @@ -1348,8 +1340,7 @@ def compile( private_deps: Private (implementation-only) dependencies of the target being compiled. These are only used as dependencies of the Swift module, not of the Clang module for the generated header. These - targets must propagate one of the following providers: `CcInfo`, - `SwiftInfo`, or `apple_common.Objc`. + targets must propagate `CcInfo` or `SwiftInfo`. srcs: The Swift source files to compile. swift_toolchain: The `SwiftToolchainInfo` provider of the toolchain. target_name: The name of the target for which the code is being @@ -1403,9 +1394,9 @@ def compile( ]) + compile_outputs.object_files + other_outputs # Merge the providers from our dependencies so that we have one each for - # `SwiftInfo`, `CcInfo`, and `apple_common.Objc`. Then we can pass these - # into the action prerequisites so that configurators have easy access to - # the full set of values and inputs through a single accessor. + # `SwiftInfo` and `CcInfo`. Then we can pass these into the action + # prerequisites so that configurators have easy access to the full set of + # values and inputs through a single accessor. merged_providers = _merge_targets_providers( implicit_deps_providers = swift_toolchain.implicit_deps_providers, targets = deps + private_deps, @@ -1462,10 +1453,6 @@ def compile( genfiles_dir = feature_configuration._genfiles_dir, is_swift = True, module_name = module_name, - objc_include_paths_workaround = ( - merged_providers.objc_include_paths_workaround - ), - objc_info = merged_providers.objc_info, source_files = srcs, transitive_modules = transitive_modules, transitive_swiftmodules = transitive_swiftmodules, @@ -1691,8 +1678,6 @@ def _precompile_clang_module( is_swift = False, is_swift_generated_header = is_swift_generated_header, module_name = module_name, - objc_include_paths_workaround = depset(), - objc_info = apple_common.new_objc_provider(), pcm_file = precompiled_module, source_files = [module_map_file], transitive_modules = transitive_modules, @@ -1731,8 +1716,7 @@ def _create_cc_compilation_context( deps: Non-private dependencies of the target being compiled. These targets are used as dependencies of both the Swift module being compiled and the Clang module for the generated header. These - targets must propagate one of the following providers: `CcInfo`, - `SwiftInfo`, or `apple_common.Objc`. + targets must propagate `CcInfo` or `SwiftInfo`. feature_configuration: A feature configuration obtained from `swift_common.configure_features`. public_hdrs: Public headers that should be propagated by the new @@ -2059,11 +2043,10 @@ def _declare_validated_generated_header(actions, generated_header_name): def _merge_targets_providers(implicit_deps_providers, targets): """Merges the compilation-related providers for the given targets. - This function merges the `CcInfo`, `SwiftInfo`, and `apple_common.Objc` - providers from the given targets into a single provider for each. These - providers are then meant to be passed as prerequisites to compilation - actions so that configurators can populate command lines and inputs based on - their data. + This function merges the `CcInfo` and `SwiftInfo` providers from the given + targets into a single provider for each. These providers are then meant to + be passed as prerequisites to compilation actions so that configurators can + populate command lines and inputs based on their data. Args: implicit_deps_providers: The implicit deps providers `struct` from the @@ -2074,40 +2057,19 @@ def _merge_targets_providers(implicit_deps_providers, targets): A `struct` containing the following fields: * `cc_info`: The merged `CcInfo` provider of the targets. - * `objc_include_paths_workaround`: A `depset` containing the include - paths from the given targets that should be passed to ClangImporter. - This is a workaround for some currently incorrect propagation - behavior that is being removed in the future. - * `objc_info`: The merged `apple_common.Objc` provider of the targets. * `swift_info`: The merged `SwiftInfo` provider of the targets. """ cc_infos = list(implicit_deps_providers.cc_infos) - objc_infos = list(implicit_deps_providers.objc_infos) swift_infos = list(implicit_deps_providers.swift_infos) - # TODO(b/146575101): This is only being used to preserve the current - # behavior of strict Objective-C include paths being propagated one more - # level than they should be. Once the remaining targets that depend on this - # behavior have been fixed, remove it. - objc_include_paths_workaround_depsets = [] - for target in targets: if CcInfo in target: cc_infos.append(target[CcInfo]) if SwiftInfo in target: swift_infos.append(target[SwiftInfo]) - if apple_common.Objc in target: - objc_infos.append(target[apple_common.Objc]) - objc_include_paths_workaround_depsets.append( - target[apple_common.Objc].strict_include, - ) return struct( cc_info = cc_common.merge_cc_infos(cc_infos = cc_infos), - objc_include_paths_workaround = depset( - transitive = objc_include_paths_workaround_depsets, - ), - objc_info = apple_common.new_objc_provider(providers = objc_infos), swift_info = create_swift_info(swift_infos = swift_infos), ) diff --git a/swift/internal/providers.bzl b/swift/internal/providers.bzl index 95cc4ba43..4500af83b 100644 --- a/swift/internal/providers.bzl +++ b/swift/internal/providers.bzl @@ -291,28 +291,10 @@ def create_clang_module( *, compilation_context, module_map, - precompiled_module = None): + precompiled_module = None, + strict_includes = None): """Creates a value representing a Clang module used as a Swift dependency. - Note: The `compilation_context` argument of this function is primarily - intended to communicate information *to* the Swift build rules, not to - retrieve information *back out.* In most cases, it is better to depend on - the `CcInfo` provider propagated by a Swift target to collect transitive - C/Objective-C compilation information about that target. This is because the - context used when compiling the module itself may not be the same as the - context desired when depending on it. (For example, `apple_common.Objc` - supports "strict include paths" which are only propagated to direct - dependents.) - - One valid exception to the guidance above is retrieving the generated header - associated with a specific Swift module. Since the `CcInfo` provider - propagated by the library will have already merged them transitively (or, - in the case of a hypothetical custom rule that propagates multiple direct - modules, the `direct_public_headers` of the `CcInfo` would also have them - merged), it is acceptable to read the headers from the compilation context - of the module struct itself in order to associate them with the module that - generated them. - Args: compilation_context: A `CcCompilationContext` that contains the header files and other context (such as include paths, preprocessor @@ -328,15 +310,23 @@ def create_clang_module( explicit module was built for the module; in that case, targets that depend on the module will fall back to the text module map and headers. + strict_includes: A `depset` of strings representing additional Clang + include paths that should be passed to the compiler when this module + is a _direct_ dependency of the module being compiled. May be + `None`. **This field only exists to support a specific legacy use + case and should otherwise not be used, as it is fundamentally + incompatible with Swift's import model.** Returns: - A `struct` containing the `compilation_context`, `module_map`, and - `precompiled_module` fields provided as arguments. + A `struct` containing the `compilation_context`, `module_map`, + `precompiled_module`, and `strict_includes` fields provided as + arguments. """ return struct( compilation_context = compilation_context, module_map = module_map, precompiled_module = precompiled_module, + strict_includes = strict_includes, ) def create_swift_module( diff --git a/swift/internal/swift_clang_module_aspect.bzl b/swift/internal/swift_clang_module_aspect.bzl index 2520309e6..040f07810 100644 --- a/swift/internal/swift_clang_module_aspect.bzl +++ b/swift/internal/swift_clang_module_aspect.bzl @@ -505,7 +505,6 @@ def _module_info_for_target( def _handle_module( aspect_ctx, - compilation_context, exclude_headers, feature_configuration, module_map_file, @@ -518,8 +517,6 @@ def _handle_module( Args: aspect_ctx: The aspect's context. - compilation_context: The `CcCompilationContext` containing the target's - headers. exclude_headers: A `list` of `File`s representing header files to exclude, if any, if we are generating the module map. feature_configuration: The current feature configuration. @@ -546,6 +543,11 @@ def _handle_module( direct_swift_infos + swift_infos + swift_toolchain.clang_implicit_deps_providers.swift_infos ) + if CcInfo in target: + compilation_context = target[CcInfo].compilation_context + else: + compilation_context = None + # Collect the names of Clang modules that the module being built directly # depends on. dependent_module_names = [] @@ -584,9 +586,24 @@ def _handle_module( else: return [] + compilation_contexts_to_merge_for_compilation = [compilation_context] + + # Fold the `strict_includes` from `apple_common.Objc` into the Clang module + # descriptor in `SwiftInfo` so that the `Objc` provider doesn't have to be + # passed as a separate input to Swift build APIs. + if apple_common.Objc in target: + strict_includes = target[apple_common.Objc].strict_include + compilation_contexts_to_merge_for_compilation.append( + cc_common.create_compilation_context(includes = strict_includes), + ) + else: + strict_includes = None + compilation_context_to_compile = ( compilation_context_for_explicit_module_compilation( - compilation_contexts = [compilation_context], + compilation_contexts = ( + compilation_contexts_to_merge_for_compilation + ), deps = [ target for attr_name in _MULTIPLE_TARGET_ASPECT_ATTRS @@ -619,6 +636,7 @@ def _handle_module( compilation_context = compilation_context, module_map = module_map_file, precompiled_module = precompiled_module, + strict_includes = strict_includes, ), ), ], @@ -838,7 +856,6 @@ def _swift_clang_module_aspect_impl(target, aspect_ctx): if interop_info or apple_common.Objc in target or CcInfo in target: return _handle_module( aspect_ctx = aspect_ctx, - compilation_context = _compilation_context_for_target(target), exclude_headers = exclude_headers, feature_configuration = feature_configuration, module_map_file = module_map_file, diff --git a/swift/internal/utils.bzl b/swift/internal/utils.bzl index faa30aed4..6f5045da3 100644 --- a/swift/internal/utils.bzl +++ b/swift/internal/utils.bzl @@ -93,20 +93,23 @@ def compilation_context_for_explicit_module_compilation( all_compilation_contexts.append(dep[CcInfo].compilation_context) elif SwiftInfo in dep: # TODO(b/151667396): Remove j2objc-specific knowledge. - # J2ObjC doesn't expose CcInfo directly on the `java_library` targets it process, but we can find the - # compilation context that was synthesized by `swift_clang_module_aspect` within the `SwiftInfo` provider. - all_compilation_contexts.extend([ - module.clang.compilation_context - for module in dep[SwiftInfo].direct_modules - if module.clang - ]) - - if apple_common.Objc in dep: - all_compilation_contexts.append( - cc_common.create_compilation_context( - includes = dep[apple_common.Objc].strict_include, - ), - ) + # J2ObjC doesn't expose `CcInfo` directly on the `java_library` + # targets it processes, but we can find the compilation context that + # was synthesized by `swift_clang_module_aspect` within the + # `SwiftInfo` provider. + for module in dep[SwiftInfo].direct_modules: + clang = module.clang + if not clang: + continue + + if clang.compilation_context: + all_compilation_contexts.append(clang.compilation_context) + if clang.strict_includes: + all_compilation_contexts.append( + cc_common.create_compilation_context( + includes = clang.strict_includes, + ), + ) return cc_common.merge_compilation_contexts( compilation_contexts = all_compilation_contexts,