feat: implement the external_include_paths feature - #242
Open
vadikmironov wants to merge 1 commit into
Open
vadikmironov wants to merge 1 commit into
vadikmironov wants to merge 1 commit into
Conversation
external_include_paths is not a Bazel built-in; each cc_toolchain_config has to implement it. rules_cc defines it in unix_cc_toolchain_config.bzl, and toolchains_llvm delegates to that config, but this toolchain has its own config and never defined the feature. The flag is silently a no-op: Bazel does not warn when you request a feature a toolchain lacks. External headers arrive as -I/-iquote instead of -isystem, which breaks #include <...> lookups and, in repos that build first-party code with -Werror, any warning raised inside a vendored header. The feature mirrors rules_cc's, restricted to the action names this toolchain already uses in include_paths_feature. It is not enabled by default and the flag group is guarded by expand_if_available, so nothing changes unless it is requested. Fixes f0rmiga#215
This was referenced Aug 29, 2026
Closed
vadikmironov
added a commit
to vadikmironov/omniglot-bazel-starter
that referenced
this pull request
Aug 29, 2026
Recovers the Windows CI leg, `if: false` since the initial public release, and re-enables the hermetic GCC leg. Both green. Thirteen CI rounds, each surfacing one real defect. Ours: - credential-helper was a shebang script; Bazel launches it with CreateProcessW, which cannot run one. Now PowerShell plus a .bat. - `/std:c++23` is not an MSVC option; it silently fell back to C++14. MSVC spells it `/std:c++23preview`. Missing `/utf-8` too, which fmt asserts on. - python_toolchain_resolver.cpp had four POSIX layout assumptions, including a runfiles lookup asking Rlocation for a directory. - test_utils.h used POSIX setenv/unsetenv, absent from MSVC's CRT, and built only POSIX Python layouts, so ten resolver tests failed and five passed for the wrong reason. - call_in_subinterpreter_test aborted the process whenever SetUp asserted, burying the real error. Platform-independent. - the OCI tar layer was unconstrained while both consumers were Linux-only. - cpp_app_with_cmake_dep hardcoded libfmt.a; MSVC produces fmt.lib and fmt appends "d" in Debug, so `-c dbg` was already broken on Linux. Six dependency patches via single_version_override, all fixes that exist upstream but were never released. toolchains_llvm 1.8.0 (#791, #780) plus LLVM 22.1.8, superseding #114 and #10; rules_foreign_cc 0.15.1 (#1570, #1458, #1459); gcc_toolchain 0.12.0 (external_include_paths, sent upstream as f0rmiga/gcc-toolchain#242). Tracked for removal in #9. The GCC leg needed a second fix beyond its stated blocker: rustc appends a crate's link_deps after --linkopt, so -l:libstdc++.a was consumed before the tcmalloc objects needing it. Windows covers //modules/... — //tools/... resolves clang-tidy, clang-format and llvm-symbolizer from the hermetic LLVM, which has no Windows build. go_app_with_cgo_dep is incompatible there (rules_go denylists msvc-cl for cgo), as are the profiling workloads by tag (gperftools and memray are Linux/macOS only). Also salvages renovate.json labels and .claude/settings.json permissions from #10, and removes references to a tools/buildifier.bat that never existed.
Author
|
@f0rmiga, when you will have a chance to get to this - here are some details behind this PR: I've been running this exact change as a downstream patch since 30 August (hermetic GCC 15.2.0, |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #215.
external_include_pathsis not a Bazel built-in; eachcc_toolchain_confighas to implement it. rules_cc implements it inunix_cc_toolchain_config.bzl, and toolchains_llvm delegates to that config, which is why the LLVM toolchain honours--features=external_include_paths.This toolchain has its own
cc_toolchain_config.bzland never defined the feature, so the flag is silently a no-op — Bazel does not warn when you request a feature a toolchain lacks. External headers arrive as-I/-iquoteinstead of-isystem, which breaks#include <...>lookups (the symptom in #215) and, in repos that build first-party code with-Werror, any warning inside a vendored header turns into an error.The feature added here mirrors rules_cc's, restricted to the action names this toolchain already uses in
include_paths_feature. The flag group carriesexpand_if_available = "external_include_paths", so it stays inert unless the feature is requested and the variable is set — no existing behaviour changes when it is off.Verification
On a repo that builds C++ with
-Werroragainst a vendored googletest, same target and same flag, before and after:Before the change that target failed to compile on
-Werror=sign-compareraised insidegtest.h; after it, it builds. The LLVM build is unaffected either way, since it never depended on this toolchain's config.