Use includes instead of system_includes for includes attr
#4
Conversation
Previously even though the attribute was named `includes` it was passed through the `system_includes` field of the compilation context. This resulted in toolchains passing these include paths with `-isystem`, which is unexpected if you use this for first party code. Many non bazel-first projects have header directory structures that require custom include paths be propagated throughout the graph, the alternative to `includes` is to use `strip_include_prefix`. The downside of `strip_include_prefix` is that you add 1 include path per `cc_library`, even if the libraries are in the same package. With `includes` these are deduplicated. In the case of LLVM using `includes` reduced the number of search paths on the order of hundreds. If users want to use `-isystem` for third party code that uses `includes`, they can pass `--features=external_include_paths --host_features=external_include_paths` If there are first party libraries users want to use `-isystem` with, they can use `features = ["system_include_paths"]` Fixes bazelbuild#20267 RELNOTES[INC]: Use `-I` instead of `-isystem` for `cc_library` / `cc_binary` `includes` attr. To use `-isystem` for only external repositories, you can pass `--features=external_include_paths --host_features=external_include_paths`. To use `-isystem` for a single `cc_library` / `cc_binary` `includes`, you can set `features = ["system_include_paths"],` on the target
|
this requires a toolchain change in order to add a test, I can do that if folks are happy with this direction |
|
see the alternative here bazelbuild#25751 |
|
Selfishly, we have on the order of a dozen+ cc_toolchains that will need to add the I'm not opposed in principle, but can we add an incompatible flag for this? Or keep the current behavior if a feature is unset? Open to suggestions, but I'd like to have a way to disable this globally (for our monorepo) without having to modify many toolchains. |
|
totally fair, I can add this behind an incompatible flag, before I do that on this one, should we consider bazelbuild#25751 or should I close that and move forward with this approach? |
|
it looks like there is another implementation option, we can use |
|
I've updated to the |
|
ok the CI failures here lead to some interesting discoveries. for one the BUILD files of both grpc and c-ares have undeclared inclusion errors lurking that only hit if you use the change here is that previously those were ignored because The fix here is to pass I've added a release notes blurb as well |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis set of changes updates the handling of include directories in Bazel's C++ and Objective-C build rules. The code and build scripts are modified to shift from using "system include" directories to more general "include" directories in both the build logic and associated tests. Several scripts and Starlark files now use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bazel
participant StarlarkRule
participant CppCompileAction
User->>Bazel: Starts build with updated flags
Bazel->>StarlarkRule: Invokes cc_binary/cc_library/objc rules
StarlarkRule->>StarlarkRule: Calls cc_helper.include_dirs(...)
StarlarkRule->>CppCompileAction: Passes include dirs as 'includes'
CppCompileAction->>CppCompileAction: Ignores built-in, system, and external include dirs during validation
CppCompileAction-->>Bazel: Compilation proceeds with updated include handling
Assessment against linked issues
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request aims to replace system_includes with includes for the includes attribute in Bazel's C++ rules. This change intends to use -I instead of -isystem for include paths, providing more control to users over how include paths are propagated. The changes look good overall.
Summary of Findings
- External Include Directories Validation: The change in
CppCompileAction.javaintroduces the validation of external include directories. It's important to ensure that this validation doesn't inadvertently break existing builds or introduce performance regressions. Consider adding more robust testing to cover various scenarios with external include paths. - TODO comment in cc_helper.bzl: The
cc_helper.bzlfile contains aTODOcomment regarding the removal of the old namesystem_include_dirs. It's important to address thisTODOin a timely manner to avoid confusion and maintain code clarity. Consider adding a follow-up issue or task to track the removal of the deprecated name. - Feature flag for external include paths: The pull request introduces a feature flag (
external_include_paths) to control the use of-isystemfor external repositories. It's important to document this feature flag clearly and provide guidance on when and how to use it. Consider adding a section to the Bazel documentation or a dedicated README file explaining the purpose and usage of this flag.
Merge Readiness
The pull request appears to be well-structured and addresses the intended issue. However, it's important to address the TODO comment in cc_helper.bzl and ensure that the validation of external include directories doesn't introduce any regressions. Additionally, clear documentation for the external_include_paths feature flag is crucial for users to understand and utilize this functionality effectively. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. I would recommend addressing the high severity issue before merging.
| @@ -1264,7 +1264,8 @@ cc_helper = struct( | |||
| get_private_hdrs = _get_private_hdrs, | |||
| get_public_hdrs = _get_public_hdrs, | |||
| report_invalid_options = _report_invalid_options, | |||
| system_include_dirs = _system_include_dirs, | |||
| include_dirs = _include_dirs, | |||
| system_include_dirs = _include_dirs, # TODO: Remove uses of old name | |||
There was a problem hiding this comment.
There is a TODO comment here to remove the old name system_include_dirs. It's important to address this in a timely manner to avoid confusion and maintain code clarity. Should we create a follow-up issue or task to track the removal of this deprecated name?
include_dirs = _include_dirs,
# TODO: Remove uses of old name
# system_include_dirs = _include_dirs,
| .doesNotContain("foo/implementation_dep"); | ||
| assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs())) | ||
| .doesNotContain("foo/implementation_dep"); |
There was a problem hiding this comment.
Why is getSystemIncludeDirs() still being called here, even though it's intended to be deprecated? Should this be updated to use getIncludeDirs() instead for consistency?
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java (1)
1908-1913: Correctly verifies implementation dependencies are not propagatedThe test now checks
getIncludeDirs()instead ofgetSystemIncludeDirs()to verify that implementation dependency include directories are not propagated transitively, consistent with the change to use regular includes instead of system includes.
🧹 Nitpick comments (2)
scripts/bootstrap/bootstrap.sh (1)
47-47: Enable external include paths for bootstrap invocationsThe addition of
--features=external_include_pathsand--host_features=external_include_pathsto_BAZEL_ARGSensures the bootstrap process respects the new include-path semantics for both target and host compilations. Please verify that the script still completes successfully on all supported platforms after these flags are applied.Suggestion: Refactor
_BAZEL_ARGSto a Bash arrayTo improve readability and prevent potential word-splitting pitfalls when appending future flags, consider defining
_BAZEL_ARGSas an array instead of a single quoted string. For example:_BAZEL_ARGS=( --spawn_strategy=standalone --nojava_header_compilation --strategy=Javac=worker --worker_quit_after_build --ignore_unsupported_sandboxing --compilation_mode=opt --repository_cache=derived/repository_cache --repo_env=BAZEL_HTTP_RULES_URLS_AS_DEFAULT_CANONICAL_ID=0 --extra_toolchains=//scripts/bootstrap:all --extra_toolchains=@rules_python//python/runtime_env_toolchains:all --enable_bzlmod --check_direct_dependencies=error --lockfile_mode=update --features=external_include_paths --host_features=external_include_paths --override_repository=$(cat derived/maven/MAVEN_CANONICAL_REPO_NAME)=derived/maven --java_runtime_version=${JAVA_VERSION} --java_language_version=${JAVA_VERSION} --tool_java_runtime_version=${JAVA_VERSION} --tool_java_language_version=${JAVA_VERSION} --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 ${DIST_BOOTSTRAP_ARGS:-} ${EXTRA_BAZEL_ARGS:-} )Then invoke Bazel with
"${_BAZEL_ARGS[@]}"to preserve each element as a discrete argument.src/main/starlark/builtins_bzl/common/cc/compile/cc_compilation_helper.bzl (1)
230-233: Added conditional logic forsystem_include_pathsfeatureThis change adds support for the
system_include_pathsfeature flag that allows users to opt into the previous behavior (using-isystemfor includes). When the feature is enabled, regular include directories are merged with system include directories.Consider adding a brief comment explaining the purpose of the
system_include_pathsfeature to make the code more self-documenting.+ # When system_include_paths feature is enabled, treat include_dirs as system includes + # to maintain backward compatibility with previous behavior if not external and feature_configuration.is_requested("system_include_paths"):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (12)
compile.sh(1 hunks)scripts/bootstrap/bootstrap.sh(1 hunks)src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java(1 hunks)src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl(1 hunks)src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl(2 hunks)src/main/starlark/builtins_bzl/common/cc/cc_library.bzl(1 hunks)src/main/starlark/builtins_bzl/common/cc/compile/cc_compilation_helper.bzl(1 hunks)src/main/starlark/builtins_bzl/common/objc/compilation_support.bzl(1 hunks)src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java(4 hunks)src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java(2 hunks)src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java(1 hunks)src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java(1 hunks)
🔇 Additional comments (14)
src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java (2)
1401-1402: Verified includes attribute behavior change.The test now only checks for "foo/bar" being in the merged system includes, omitting "a/dep1/baz" and "a/dep2/qux" which have been moved to regular includes. This correctly validates the change described in the PR where the
includesattribute now uses-Iflags instead of-isystem.
1404-1405: Correctly updates test to match the new behavior for includes.Now testing that "a/dep1/baz" and "a/dep2/qux" are present in merged_includes rather than in system_includes, confirming that the
includesattribute now correctly places paths in regular includes rather than system includes. This is the key verification for the PR behavior change.compile.sh (1)
64-64: Added compiler flags to suppress warnings for external includes on Windows.This change adds two MSVC-specific compiler flags (
/external:W0) that disable warnings from headers in external include directories on Windows. This complements the PR's main change where includes are now passed with-Iinstead of-isystem, ensuring consistent warning behavior between platforms.src/main/starlark/builtins_bzl/common/cc/cc_library.bzl (1)
66-66: Changed parameter fromsystem_includestoincludesreflecting the new include path handling approach.This is the core change implementing the PR objective. The function now uses
cc_helper.include_dirs()and passes the result as theincludesparameter instead ofsystem_includes, which means these include paths will be passed with-Iflags rather than-isystem. This aligns with the expectation that first-party code should use regular include paths.src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java (1)
1135-1139: Added external include directories to validation ignored dirs.This change properly extends the set of directories ignored during include validation by adding external include directories. Even though these paths will now be passed with
-Iflags rather than-isystem, they should still be exempt from the requirement that all included files must be explicitly declared in the build files.src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java (1)
1400-1400: Test correctly updated to validate new include flag behaviorThis change properly updates the test to verify that include directories from transitive C++ dependencies are now passed to compile actions with
-Iflags rather than-isystemflags, confirming the core functionality of this PR.src/main/starlark/builtins_bzl/common/objc/compilation_support.bzl (1)
108-108: Core implementation change for include paths handlingThis change is the key implementation that shifts include directories specified in the
includesattribute from being treated as system includes (-isystem) to regular includes (-I). This correctly implements the main objective of the PR to use-Iflags for first-party code includes.src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl (1)
513-513: Consistent implementation of include path handling in cc_binary ruleThis change correctly updates the cc_binary implementation to use regular include flags (
-I) instead of system include flags (-isystem) for include directories, maintaining consistency with the changes in other files and properly implementing the PR's objective.src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java (1)
1197-1204: Changes correctly use-Iinstead of-isystemfor include pathsThe test now expects include directories to be passed with
-Iflags rather than-isystem, aligning with the change to use regular includes for theincludesattribute instead of system includes.src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl (1)
1065-1065: Renamed function to better reflect its purposeFunction renamed from
_system_include_dirsto_include_dirsto accurately reflect that it's now used for regular include directories, not just system includes.src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java (4)
576-577: Test assertion updated to check include dirs instead of system include dirsThis change correctly updates the test assertion to check
getIncludeDirs()instead ofgetSystemIncludeDirs(), aligning with the PR's objective to make theincludesattribute use-Iflags instead of-isystemflags for include paths.
599-600: Updated assertions for testing includes with merge_genfiles_directory flagThese assertion changes properly test that regular include directories (not system include directories) are affected by the
--incompatible_merge_genfiles_directoryflag, consistent with the PR's change to how include directories are handled.Also applies to: 604-605
629-629: Added system_include_paths feature flag to testThis addition correctly implements the new opt-in mechanism for the old behavior. By adding the
system_include_pathsfeature, this test now explicitly requests that include paths should be treated as system includes (using-isystem), which tests the backward compatibility feature mentioned in the PR objectives.
648-685: Added new test for-Iinclude behaviorThis new test method complements the existing
testUseIsystemForIncludes()method by verifying the new default behavior where include paths use the-Iflag. The test properly checks that paths from theincludesattribute are added to theIncludeDirsrather thanSystemIncludeDirs, which is the key change in this PR.
| include_dirs = _include_dirs, | ||
| system_include_dirs = _include_dirs, # TODO: Remove uses of old name |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Updated struct to reference the renamed function
Both include_dirs and system_include_dirs keys now reference the renamed _include_dirs function, with a TODO to eventually remove the old name.
The TODO comment indicates the system_include_dirs name should be removed in the future. Consider creating a follow-up issue to track this cleanup task to avoid technical debt accumulation.
🏁 Script executed:
#!/bin/bash
# Search for usages of system_include_dirs to identify code that needs to be updated
rg "system_include_dirs" --type bzlLength of output: 63
🏁 Script executed:
#!/bin/bash
# Search for any usage of system_include_dirs in .bzl files
rg "system_include_dirs" -n --color never -g '*.bzl'Length of output: 1324
Track and plan removal of system_include_dirs alias
There are still multiple references to system_include_dirs that will need updating once the alias is removed. To avoid accumulating technical debt, please create a follow-up issue to track this cleanup and update these locations when ready:
- src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl (line 1268)
- src/main/starlark/builtins_bzl/common/cc/compile/cc_compilation_helper.bzl (lines 186, 231, 234, 237–238, 245, 401, 430)
No immediate code changes required—just open an issue to consolidate and schedule the refactor.
Previously even though the attribute was named
includesit was passedthrough the
system_includesfield of the compilation context. Thisresulted in toolchains passing these include paths with
-isystem,which is unexpected if you use this for first party code.
Many non bazel-first projects have header directory structures that
require custom include paths be propagated throughout the graph, the
alternative to
includesis to usestrip_include_prefix. The downsideof
strip_include_prefixis that you add 1 include path percc_library, even if the libraries are in the same package. Withincludesthese are deduplicated. In the case of LLVM usingincludesreduced the number of search paths on the order of hundreds.
If users want to use
-isystemfor third party code that usesincludes, they can pass--features=external_include_paths --host_features=external_include_pathsIf there are first party libraries users want to use
-isystemwith,they can use
features = ["system_include_paths"]Fixes bazelbuild#20267
RELNOTES[INC]: Use
-Iinstead of-isystemforcc_library/cc_binaryincludesattr. To use-isystemfor only external repositories, you can pass--features=external_include_paths --host_features=external_include_paths. To use-isystemfor a singlecc_library/cc_binaryincludes, you can setfeatures = ["system_include_paths"],on the targetSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores