Add layering_check support for macOS #22259
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
team-Rules-CPP
fmeum
approved these changes
May 6, 2024
There was a problem hiding this comment.
You need to run this target to update the lockfile:
bazel/src/test/tools/bzlmod/BUILD
Line 12 in 3bc70b2
Looks good! The faster find wouldn't work on Linux, so the amount of "fork"ing looks just right.
keith
force-pushed
the
ks/add-layering_check-support-for-macos
branch
6 times, most recently
from
67c225a
to
d0cdc1d
Compare
keith
force-pushed
the
ks/add-layering_check-support-for-macos
branch
from
d0cdc1d
to
e59f370
Compare
|
@pzembrod could you take a look? 🙏🏻 |
|
@keith I left a few comments on your push, because apparently GitHub's UI succeeded in confusing me. ;-) |
comius
added
awaiting-user-response
pzembrod
reviewed
May 15, 2024
| @@ -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 | |||
There was a problem hiding this comment.
This was disabled before on MacOS because it was too slow?
| @@ -144,11 +144,6 @@ EOF | |||
| } | |||
|
|
|||
| function test_bazel_layering_check() { | |||
There was a problem hiding this comment.
Just checking: the related test/feature test_bazel_layering_check_header_only() still doesn't work on MacOS?
There was a problem hiding this comment.
i didn't want to change it in this PR too since it's unrelated, but I think we can enable it afterwards #22370
|
I moved my comments over to the PR |
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.
keith
force-pushed
the
ks/add-layering_check-support-for-macos
branch
from
e59f370
to
0286a53
Compare
pzembrod
approved these changes
May 17, 2024
| @@ -144,11 +144,6 @@ EOF | |||
| } | |||
|
|
|||
| function test_bazel_layering_check() { | |||
pzembrod
added
awaiting-PR-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were 2 things with the previous implementation that needed to be improved here:
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.