Skip to content

Use includes instead of system_includes for includes attr #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+74 −30
Open

Use includes instead of system_includes for includes attr #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compile.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ if [[ $PLATFORM == "darwin" ]] && \
fi

if [[ $PLATFORM == "windows" ]]; then
EXTRA_BAZEL_ARGS="${EXTRA_BAZEL_ARGS-} --cxxopt=/std:c++17 --host_cxxopt=/std:c++17"
EXTRA_BAZEL_ARGS="${EXTRA_BAZEL_ARGS-} --cxxopt=/std:c++17 --host_cxxopt=/std:c++17 --copt=/external:W0 --host_copt=/external:W0"
fi

source scripts/bootstrap/bootstrap.sh
Expand Down
1 change: 1 addition & 0 deletions scripts/bootstrap/bootstrap.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ _BAZEL_ARGS="--spawn_strategy=standalone \
--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} \
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,7 +1133,10 @@ public void validateInclusions(

private Iterable<PathFragment> getValidationIgnoredDirs() {
List<PathFragment> cxxSystemIncludeDirs = getBuiltInIncludeDirectories();
return Iterables.concat(cxxSystemIncludeDirs, ccCompilationContext.getSystemIncludeDirs());
return Iterables.concat(
cxxSystemIncludeDirs,
ccCompilationContext.getSystemIncludeDirs(),
ccCompilationContext.getExternalIncludeDirs());
}

@VisibleForTesting
Expand Down
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False):
cxx_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "cxxopts"),
defines = cc_helper.defines(ctx, additional_make_variable_substitutions),
local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps),
system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions),
includes = cc_helper.include_dirs(ctx, additional_make_variable_substitutions),
private_hdrs = cc_helper.get_private_hdrs(ctx),
public_hdrs = cc_helper.get_public_hdrs(ctx),
copts_filter = cc_helper.copts_filter(ctx, additional_make_variable_substitutions),
Expand Down
5 changes: 3 additions & 2 deletions src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ def _package_source_root(repository, package, sibling_repository_layout):
repository = repository[1:]
return paths.get_relative(paths.get_relative("external", repository), package)

def _system_include_dirs(ctx, additional_make_variable_substitutions):
def _include_dirs(ctx, additional_make_variable_substitutions):
result = []
sibling_repository_layout = ctx.configuration.is_sibling_repository_layout()
package = ctx.label.package
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

high

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,

Comment on lines +1267 to +1268
Copy link

Choose a reason for hiding this comment

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

💡 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 bzl

Length 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.

get_coverage_environment = _get_coverage_environment,
create_cc_instrumented_files_info = _create_cc_instrumented_files_info,
linkopts = _linkopts,
Expand Down
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def _cc_library_impl(ctx):
cxx_flags = cc_helper.get_copts(ctx, feature_configuration, additional_make_variable_substitutions, attr = "cxxopts"),
defines = cc_helper.defines(ctx, additional_make_variable_substitutions),
local_defines = cc_helper.local_defines(ctx, additional_make_variable_substitutions) + cc_helper.get_local_defines_for_runfiles_lookup(ctx, ctx.attr.deps + ctx.attr.implementation_deps),
system_includes = cc_helper.system_include_dirs(ctx, additional_make_variable_substitutions),
includes = cc_helper.include_dirs(ctx, additional_make_variable_substitutions),
copts_filter = cc_helper.copts_filter(ctx, additional_make_variable_substitutions),
purpose = "cc_library-compile",
srcs = cc_helper.get_srcs(ctx),
Expand Down
5 changes: 4 additions & 1 deletion src/main/starlark/builtins_bzl/common/cc/compile/cc_compilation_helper.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,10 @@ def _init_cc_compilation_context(
external_include_dirs = []
declared_include_srcs = []

if not external:
if not external and feature_configuration.is_requested("system_include_paths"):
system_include_dirs_for_context = system_include_dirs + include_dirs
include_dirs_for_context = []
elif not external:
system_include_dirs_for_context = list(system_include_dirs)
include_dirs_for_context = list(include_dirs)
else:
Expand Down
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/objc/compilation_support.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def _build_common_variables(
has_module_map = has_module_map,
attr_linkopts = attr_linkopts,
direct_cc_compilation_contexts = direct_cc_compilation_contexts,
includes = cc_helper.system_include_dirs(ctx, {}) if hasattr(ctx.attr, "includes") else [],
includes = cc_helper.include_dirs(ctx, {}) if hasattr(ctx.attr, "includes") else [],
)

return struct(
Expand Down
46 changes: 43 additions & 3 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ public void testIsolatedIncludes() throws Exception {
ConfiguredTarget foo = getConfiguredTarget("//bang:bang");

String includesRoot = "bang/bang_includes";
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
.containsAtLeast(
PathFragment.create(includesRoot),
targetConfig.getGenfilesFragment(RepositoryName.MAIN).getRelative(includesRoot));
Expand All @@ -596,12 +596,12 @@ public void testDisabledGenfilesDontShowUpInSystemIncludePaths() throws Exceptio
ConfiguredTarget foo = getConfiguredTarget("//bang:bang");
PathFragment genfilesDir =
targetConfig.getGenfilesFragment(RepositoryName.MAIN).getRelative(includesRoot);
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
.contains(genfilesDir);

useConfiguration("--incompatible_merge_genfiles_directory");
foo = getConfiguredTarget("//bang:bang");
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getSystemIncludeDirs())
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
.doesNotContain(genfilesDir);
}

Expand All @@ -626,6 +626,7 @@ public void testUseIsystemForIncludes() throws Exception {
name = "bang",
srcs = ["bang.cc"],
includes = ["bang_includes"],
features = ["system_include_paths"],
)
""");

Expand All @@ -644,6 +645,45 @@ public void testUseIsystemForIncludes() throws Exception {
.containsExactlyElementsIn(expected);
}

@Test
public void testUseIForIncludes() throws Exception {
// Tests the effect of --use_isystem_for_includes.
useConfiguration("--incompatible_merge_genfiles_directory=false");
scratch.file(
"no_includes/BUILD",
"""
cc_library(
name = "no_includes",
srcs = ["no_includes.cc"],
)
""");
ConfiguredTarget noIncludes = getConfiguredTarget("//no_includes:no_includes");

scratch.file(
"bang/BUILD",
"""
cc_library(
name = "bang",
srcs = ["bang.cc"],
includes = ["bang_includes"],
)
""");

ConfiguredTarget foo = getConfiguredTarget("//bang:bang");

String includesRoot = "bang/bang_includes";
List<PathFragment> expected =
new ImmutableList.Builder<PathFragment>()
.addAll(
noIncludes.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
.add(PathFragment.create(includesRoot))
.add(targetConfig.getGenfilesFragment(RepositoryName.MAIN).getRelative(includesRoot))
.add(targetConfig.getBinFragment(RepositoryName.MAIN).getRelative(includesRoot))
.build();
assertThat(foo.get(CcInfo.PROVIDER).getCcCompilationContext().getIncludeDirs())
.containsExactlyElementsIn(expected);
}

@Test
public void testCcTestDisallowsAlwaysLink() throws Exception {
scratch.file(
Expand Down
28 changes: 12 additions & 16 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,18 +1196,12 @@ public void testIncludePathOrder() throws Exception {
assertContainsSublist(
action.getCompilerOptions(),
ImmutableList.of(
"-isystem",
"foo/foo",
"-isystem",
genfilesDir + "/foo/foo",
"-isystem",
binDir + "/foo/foo",
"-isystem",
"foo/bar",
"-isystem",
genfilesDir + "/foo/bar",
"-isystem",
binDir + "/foo/bar"));
"-Ifoo/foo",
"-I" + genfilesDir + "/foo/foo",
"-I" + binDir + "/foo/foo",
"-Ifoo/bar",
"-I" + genfilesDir + "/foo/bar",
"-I" + binDir + "/foo/bar"));
}

@Test
Expand Down Expand Up @@ -1911,22 +1905,24 @@ public void testImplementationDepsCompilationContextIsNotPropagated() throws Exc
assertThat(artifactsToStrings(libCompilationContext.getDeclaredIncludeSrcs()))
.doesNotContain("src foo/implementation_dep.h");

assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs()))
assertThat(pathfragmentsToStrings(libCompilationContext.getIncludeDirs()))
.contains("foo/public_dep");
assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs()))
assertThat(pathfragmentsToStrings(libCompilationContext.getIncludeDirs()))
.contains("foo/interface_dep");
assertThat(pathfragmentsToStrings(libCompilationContext.getIncludeDirs()))
.doesNotContain("foo/implementation_dep");
assertThat(pathfragmentsToStrings(libCompilationContext.getSystemIncludeDirs()))
.doesNotContain("foo/implementation_dep");
Comment on lines +1913 to 1915
Copy link

Choose a reason for hiding this comment

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

medium

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?


CcCompilationContext publicDepCompilationContext =
getCppCompileAction("//foo:public_dep").getCcCompilationContext();
assertThat(artifactsToStrings(publicDepCompilationContext.getDeclaredIncludeSrcs()))
.contains("src foo/interface_dep.h");
assertThat(pathfragmentsToStrings(publicDepCompilationContext.getSystemIncludeDirs()))
assertThat(pathfragmentsToStrings(publicDepCompilationContext.getIncludeDirs()))
.contains("foo/interface_dep");
assertThat(artifactsToStrings(publicDepCompilationContext.getDeclaredIncludeSrcs()))
.contains("src foo/implementation_dep.h");
assertThat(pathfragmentsToStrings(publicDepCompilationContext.getSystemIncludeDirs()))
assertThat(pathfragmentsToStrings(publicDepCompilationContext.getIncludeDirs()))
.contains("foo/implementation_dep");
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1398,11 +1398,11 @@ def _impl(ctx):

List<String> mergedSystemIncludes =
((Depset) myInfo.getValue("merged_system_includes")).getSet(String.class).toList();
assertThat(mergedSystemIncludes).containsAtLeast("foo/bar", "a/dep1/baz", "a/dep2/qux");
assertThat(mergedSystemIncludes).contains("foo/bar");

List<String> mergedIncludes =
((Depset) myInfo.getValue("merged_includes")).getSet(String.class).toList();
assertThat(mergedIncludes).contains("baz/qux");
assertThat(mergedIncludes).containsAtLeast("baz/qux", "a/dep1/baz", "a/dep2/qux");

List<String> mergedQuoteIncludes =
((Depset) myInfo.getValue("merged_quote_includes")).getSet(String.class).toList();
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,7 +1397,7 @@ public void testIncludesDirsOfTransitiveCcDepsGetPassedToCompileAction() throws
Iterables.concat(
Iterables.transform(
rootedIncludePaths("package/foo/bar"),
element -> ImmutableList.of("-isystem", element)))));
element -> ImmutableList.of("-I" + element)))));
}

@Test
Expand Down