Skip to content

fix: pass -isysroot to clang-tidy for macOS platform SDK headers#779

Open
vadikmironov wants to merge 9 commits into
aspect-build:mainfrom
vadikmironov:fix/clang-tidy-builtin-include-dirs
Open

fix: pass -isysroot to clang-tidy for macOS platform SDK headers#779
vadikmironov wants to merge 9 commits into
aspect-build:mainfrom
vadikmironov:fix/clang-tidy-builtin-include-dirs

Conversation

@vadikmironov

Copy link
Copy Markdown
Contributor

Summary

  • Pass -isysroot to clang-tidy so it can find macOS Xcode SDK platform headers
    (wchar.h, stdlib.h, stdint.h) that libc++ depends on via #include_next
  • Use ctx.target_platform_has_constraint(@platforms//os:macos) to guard the
    macOS-specific sysroot fallback
  • Fix pre-existing //test:clang_tidy failure (forward declaration needed for
    readability-inconsistent-declaration-parameter-name to fire)
  • Add integration test (clang_tidy_stdlib) exercising stdlib includes with -stdlib=libc++

Description

A hermetic clang-tidy (e.g. from toolchains_llvm) resolves its own libc++ and Clang builtins via binary-relative search, but cannot find macOS Xcode SDK platform headers (wchar.h, stdlib.h, stdint.h) that libc++ depends on via #include_next.

Pass -isysroot pointing to the macOS SDK root so the Clang frontend locates platform headers without injecting individual include dirs that risk mixing incompatible header versions.

The sysroot is obtained from cc_toolchain.sysroot when available, or derived from built_in_include_directories by searching for MacOSX.sdk paths. A platform constraint check (@platforms//os:macos) ensures the fallback search only runs when targeting macOS.

Also fix the pre-existing //test:clang_tidy test which expected exit code 1 but got 0: the readability-inconsistent-declaration-parameter-name check requires a forward declaration with a different parameter name to fire, but hello.cpp had only definitions. Add the missing forward declaration.

References:

Test plan

  • Linux: all 3 tests pass (clang_tidy, clang_tidy_stdlib, cppcheck)
  • macOS CI (Bazel 7.x): both clang_tidy and clang_tidy_stdlib pass
  • macOS CI (Bazel 8.x): both clang_tidy and clang_tidy_stdlib pass

Please note that I verified MacOS builds in my fork CI, but not including it into the PR. Let me know if you would like to reenable MacOS CI and I can push it as a separate PR or update this one.

A hermetic clang-tidy (e.g. from toolchains_llvm) resolves its own
libc++ and Clang builtins via binary-relative search, but cannot find
macOS Xcode SDK platform headers (wchar.h, stdlib.h, stdint.h) that
libc++ depends on via #include_next.

Pass -isysroot pointing to the macOS SDK root so the Clang frontend
locates platform headers without injecting individual include dirs
that risk mixing incompatible header versions.

The sysroot is obtained from cc_toolchain.sysroot when available, or
derived from built_in_include_directories by searching for MacOSX.sdk
paths.  A platform constraint check (@platforms//os:macos) ensures
the fallback search only runs when targeting macOS.

Also fix the pre-existing //test:clang_tidy test which expected exit
code 1 but got 0: the readability-inconsistent-declaration-parameter-name
check requires a forward declaration with a different parameter name
to fire, but hello.cpp had only definitions.  Add the missing forward
declaration.

References:
- llvm/llvm-project#63890
- aspect-build#566

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Feb 28, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@aspect-workflows

aspect-workflows Bot commented Feb 28, 2026

Copy link
Copy Markdown

Bazel 7 (Test)

3 test targets passed

Targets
//format/test:custom_args_test [k8-fastbuild] 463ms
//format/test:ls-files_test [k8-fastbuild]    876ms
//tools/sarif:sarif_test [k8-fastbuild]       133ms

Bazel 8 (Test)

All tests were cache hits

3 tests (100.0%) were fully cached saving 969ms.


Bazel 9 (Test)

All tests were cache hits

3 tests (100.0%) were fully cached saving 2s.

@vadikmironov

Copy link
Copy Markdown
Contributor Author

I did check couple failed checks and they all failing with 'download_and_extract: java.io.IOException' which must be some github.com issue? Anyway, would appreciate if someone with appropriate rights could re-trigger checks to get it to green.

@vadikmironov

Copy link
Copy Markdown
Contributor Author

I did check couple failed checks and they all failing with 'download_and_extract: java.io.IOException' which must be some github.com issue? Anyway, would appreciate if someone with appropriate rights could re-trigger checks to get it to green.

Ah, branch merge triggered rebuild and it is all green now. PR is ready for review and any feedback is much appreciated!

@vadikmironov

Copy link
Copy Markdown
Contributor Author

@JesseTatasciore , @jbedard , would you be so kind to take a look and let me know if any further comments? happy to tweak it as per your suggestions if needed.

@vadikmironov

Copy link
Copy Markdown
Contributor Author

@jbedard, @JesseTatasciore — friendly nudge on this one — it's been ready for a while and I'd love to get it in.

Status: currently conflict-free and fully green (CI + CLA), intentionally small (+61/-1) and macOS-scoped — the sysroot fallback is guarded behind a @platforms//os:macos constraint, so non-macOS platforms are unaffected. It also adds an integration test (clang_tidy_stdlib) and fixes the pre-existing //test:clang_tidy failure.

Happy to rebase, split it up, or adjust the approach if that makes review easier — just let me know what would help.

Comment thread lint/clang_tidy.bzl Outdated
# individual include dirs that risk mixing incompatible header
# versions (see https://github.com/llvm/llvm-project/issues/63890).
#
# cc_toolchain.sysroot is often None on macOS (Bazel treats the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a cc expert, but I'm told this statement is incorrect about using the Xcode SDK as the default

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, I did read through llvm issue, but perhaps too quickly. Should've actually looked into rules apple_support (https://github.com/bazelbuild/apple_support/blob/main/crosstool/cc_toolchain_config.bzl#L2424). Anyway, comment amended with the little robot help and a sdk match fixed for versioned folder names.

  - Reword the sysroot derivation comment: Apple toolchains leave
    builtin_sysroot unset and pass the SDK via an -isysroot
    __BAZEL_XCODE_SDKROOT__ flag resolved at action time, so
    cc_toolchain.sysroot is empty. Prior "Bazel treats the Xcode SDK
    as the default sysroot" wording was incorrect (review feedback).
  - Match the ".sdk/" boundary instead of "MacOSX.sdk/" so versioned
    SDK paths (e.g. MacOSX15.2.sdk) still resolve to a sysroot.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: clang-tidy includes violations in llvm_toolchain_llvm

3 participants