Skip to content

Commit

Permalink
Add layering_check support for macOS
Browse files Browse the repository at this point in the history
There were 2 things with the previous implementation that needed to be improved here:

1. Apple Clang has a bug where it doesn't pass module compiler flags to the underlying -cc1 invocation, so we have to manually pass them directly to that invocation with -Xclang
2. The previous search script was too aggressive and slow for macOS. The macOS SDK has tons of files that aren't headers, and tons of symlinks pointing to other files within the SDK. This adds a fork in the script to run a version that works with Apple SDKs. The time difference on my machine is 41s->6s. 6s is still pretty long so if desired we can put this behavior behind an env var for users to opt in with.

I've added a hermetic version of this to the apple_support toolchain, but similar to the Linux setup here the modulemap file includes absolute paths.

Closes bazelbuild#22259.

This reverts commit 1f1b4fd.

Closes bazelbuild#22475.

PiperOrigin-RevId: 637969838
Change-Id: I7d4940a820e3741836239493222ba8d06c4d70e4
  • Loading branch information
keith authored and bazel-io committed May 28, 2024
1 parent 2f2d23d commit 7f7918f
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 46 deletions.
1 change: 0 additions & 1 deletion .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ tasks:
# To run any of the following test in presubmit, just comment out the corresponding line.
# TODO(pcloudy): Disable the android tests after enabling them on Apple Silicon platform.
- "-//src/test/shell/bazel:bazel_bootstrap_distfile_test"
- "-//src/test/shell/bazel:bazel_bootstrap_distfile_tar_test"
# - "-//src/test/shell/bazel/android:android_integration_test"
# - "-//src/test/shell/bazel/android:android_integration_test_with_head_android_tools"
- "-//src/test/shell/bazel:bazel_proto_library_test"
Expand Down
8 changes: 8 additions & 0 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ local_path_override(
path = "./third_party/googleapis",
)

single_version_override(
module_name = "grpc",
patch_strip = 1,
patches = [
"//third_party/grpc:00_disable_layering_check.patch",
],
)

# The following Bazel modules are not direct dependencies for building Bazel,
# but are required for visibility from DIST_ARCHIVE_REPOS in repositories.bzl
bazel_dep(name = "apple_support", version = "1.8.1")
Expand Down
65 changes: 36 additions & 29 deletions MODULE.bazel.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 0 additions & 5 deletions src/test/shell/bazel/bazel_layering_check_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,6 @@ EOF
}

function test_bazel_layering_check() {
if is_darwin; then
echo "This test doesn't run on Darwin. Skipping."
return
fi

local -r clang_tool=$(which clang)
if [[ ! -x ${clang_tool:-/usr/bin/clang_tool} ]]; then
echo "clang not installed. Skipping test."
Expand Down
19 changes: 14 additions & 5 deletions tools/cpp/generate_system_module_map.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/bash
# Copyright 2020 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -17,10 +17,19 @@ set -eu

echo 'module "crosstool" [system] {'

for dir in $@; do
find -L "${dir}" -type f 2>/dev/null | LANG=C sort | uniq | while read header; do
echo " textual header \"${header}\""
if [[ "$OSTYPE" == darwin* ]]; then
for dir in $@; do
find "$dir" -type f \( -name "*.h" -o -name "*.def" -o -path "*/c++/*" \) \
| LANG=C sort -u | while read -r header; do
echo " textual header \"${header}\""
done
done
done
else
for dir in $@; do
find -L "${dir}" -type f 2>/dev/null | LANG=C sort -u | while read -r header; do
echo " textual header \"${header}\""
done
done
fi

echo "}"
2 changes: 1 addition & 1 deletion tools/cpp/unix_cc_configure.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools):
["/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk"],
)

generate_modulemap = is_clang and not darwin
generate_modulemap = is_clang
if generate_modulemap:
repository_ctx.file("module.modulemap", _generate_system_module_map(
repository_ctx,
Expand Down
12 changes: 7 additions & 5 deletions tools/cpp/unix_cc_toolchain_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def _target_os_version(ctx):
xcode_config = ctx.attr._xcode_config[apple_common.XcodeVersionConfig]
return xcode_config.minimum_os_for_platform_type(platform_type)

def layering_check_features(compiler):
def layering_check_features(compiler, is_macos):
if compiler != "clang":
return []
return [
Expand All @@ -53,8 +53,10 @@ def layering_check_features(compiler):
],
flag_groups = [
flag_group(
flags = [
# macOS requires -Xclang because of a bug in Apple Clang
flags = (["-Xclang"] if is_macos else []) + [
"-fmodule-name=%{module_name}",
] + (["-Xclang"] if is_macos else []) + [
"-fmodule-map-file=%{module_map_file}",
],
),
Expand Down Expand Up @@ -86,7 +88,7 @@ def layering_check_features(compiler):
]),
flag_group(
iterate_over = "dependent_module_map_files",
flags = [
flags = (["-Xclang"] if is_macos else []) + [
"-fmodule-map-file=%{dependent_module_map_files}",
],
),
Expand Down Expand Up @@ -1484,7 +1486,7 @@ def _impl(ctx):
unfiltered_compile_flags_feature,
treat_warnings_as_errors_feature,
archive_param_file_feature,
] + layering_check_features(ctx.attr.compiler)
] + layering_check_features(ctx.attr.compiler, is_macos = False)
else:
# macOS artifact name patterns differ from the defaults only for dynamic
# libraries.
Expand Down Expand Up @@ -1525,7 +1527,7 @@ def _impl(ctx):
treat_warnings_as_errors_feature,
archive_param_file_feature,
generate_linkmap_feature,
]
] + layering_check_features(ctx.attr.compiler, is_macos = True)

parse_headers_action_configs, parse_headers_features = parse_headers_support(
parse_headers_tool_path = ctx.attr.tool_paths.get("parse_headers"),
Expand Down

0 comments on commit 7f7918f

Please sign in to comment.