Skip to content

Commit

Permalink
Fix a bug where repository names were not properly matched while enfo…
Browse files Browse the repository at this point in the history 
…rcing builtin rules allowlist.

PiperOrigin-RevId: 527285106
Change-Id: I6019c3b1dba83379982e375d3041daa5622f29ad
  • Loading branch information
@buildbreaker2021 @copybara-github
buildbreaker2021 authored and copybara-github committed Apr 26, 2023
1 parent eadeb73 commit c06fe5c
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 32 deletions.
2 changes: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:actions/parameter_file_write_action",
"//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_collection",
"//src/main/java/com/google/devtools/build/lib/analysis:buildinfo/build_info_key",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
Expand Down Expand Up @@ -102,7 +101,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/core",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
"//src/main/java/com/google/devtools/build/lib/util:filetype",
Expand Down
33 changes: 21 additions & 12 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ public abstract class CcModule
ImmutableList.of("executable", "dynamic_library", "archive");

private static final ImmutableList<PackageIdentifier> PRIVATE_STARLARKIFICATION_ALLOWLIST =
// Repo names in the allowlist are non canonical repo names and package names are package
// prefixes under which we allow restricted API usage.
ImmutableList.of(
PackageIdentifier.createUnchecked("_builtins", ""),
PackageIdentifier.createInMainRepo("bazel_internal/test_rules/cc"),
Expand Down Expand Up @@ -1968,23 +1970,31 @@ public CcDebugInfoContext mergeCcDebugInfoFromStarlark(
}

private static void checkPrivateStarlarkificationAllowlistByLabel(
Label label, ImmutableList<PackageIdentifier> privateStarlarkificationAllowlist)
BazelModuleContext bazelModuleContext,
Label label,
ImmutableList<PackageIdentifier> privateStarlarkificationAllowlist)
throws EvalException {
if (privateStarlarkificationAllowlist.stream()
.noneMatch(
allowedPrefix ->
label.getRepository().equals(allowedPrefix.getRepository())
label
.getRepository()
.equals(
bazelModuleContext
.repoMapping()
.get(allowedPrefix.getRepository().getName()))
&& label.getPackageFragment().startsWith(allowedPrefix.getPackageFragment()))) {
throw Starlark.errorf("Rule in '%s' cannot use private API", label.getPackageName());
throw Starlark.errorf("Rule in '%s' cannot use private API", label);
}
}

public static void checkPrivateStarlarkificationAllowlist(StarlarkThread thread)
throws EvalException {
Label label =
((BazelModuleContext) Module.ofInnermostEnclosingStarlarkFunction(thread).getClientData())
.label();
checkPrivateStarlarkificationAllowlistByLabel(label, PRIVATE_STARLARKIFICATION_ALLOWLIST);
BazelModuleContext bazelModuleContext =
(BazelModuleContext) Module.ofInnermostEnclosingStarlarkFunction(thread).getClientData();
Label label = bazelModuleContext.label();
checkPrivateStarlarkificationAllowlistByLabel(
bazelModuleContext, label, PRIVATE_STARLARKIFICATION_ALLOWLIST);
}

public static boolean isBuiltIn(StarlarkThread thread) {
Expand Down Expand Up @@ -2030,15 +2040,14 @@ protected static void isCalledFromStarlarkCcCommon(StarlarkThread thread) throws
public void checkPrivateApi(Object allowlistObject, StarlarkThread thread) throws EvalException {
// Make sure that check_private_api is called either from builtins or allowlisted packages.
isCalledFromStarlarkCcCommon(thread);
Label label =
((BazelModuleContext)
Module.ofInnermostEnclosingStarlarkFunction(thread, 1).getClientData())
.label();
BazelModuleContext bazelModuleContext =
(BazelModuleContext) Module.ofInnermostEnclosingStarlarkFunction(thread, 1).getClientData();
Label label = bazelModuleContext.label();
ImmutableList<PackageIdentifier> allowlist =
Sequence.cast(allowlistObject, Tuple.class, "allowlist").stream()
.map(p -> PackageIdentifier.createUnchecked((String) p.get(0), (String) p.get(1)))
.collect(ImmutableList.toImmutableList());
checkPrivateStarlarkificationAllowlistByLabel(label, allowlist);
checkPrivateStarlarkificationAllowlistByLabel(bazelModuleContext, label, allowlist);
}

protected Language parseLanguage(String string) throws EvalException {
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,6 @@ public void testDwpFilesIsBlocked() throws Exception {

getConfiguredTarget("//test:target");

assertContainsEvent("Rule in 'test' cannot use private API");
assertContainsEvent("cannot use private API");
}
}
10 changes: 5 additions & 5 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationStarlarkTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,15 @@ public void testExpandedApiBlocked() throws Exception {
"ctx.fragments.cpp.fission_active_for_current_compilation_mode()");
AssertionError e;
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:pic"));
assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use 'force_pic'");
assertThat(e).hasMessageThat().contains("cannot use 'force_pic'");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:lcov"));
assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use 'generate_llvm_lcov'");
assertThat(e).hasMessageThat().contains("cannot use 'generate_llvm_lcov'");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:fdo"));
assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use 'fdo_instrument'");
assertThat(e).hasMessageThat().contains("cannot use 'fdo_instrument'");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:hdr_deps"));
assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:save"));
assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:fission"));
assertThat(e)
.hasMessageThat()
Expand Down
24 changes: 12 additions & 12 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5950,7 +5950,7 @@ public void testCustomNameOutputArtifactRaisesError() throws Exception {
setupTestTransitiveLink(scratch, "output_type = 'dynamic_library'", " main_output=None");

AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:bin"));
assertThat(e).hasMessageThat().contains("Rule in 'tools/build_defs' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}

@Test
Expand Down Expand Up @@ -6906,7 +6906,7 @@ public void testCcDebugContextDisabled() throws Exception {
" },",
")");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:b_lib"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}

@Test
Expand Down Expand Up @@ -7153,13 +7153,13 @@ public void testExpandedCcCompilationContextApiBlocked() throws Exception {
" implementation = _transitive_modules_impl",
")");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:m"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:p2"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:ai"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:tm"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}

@Test
Expand Down Expand Up @@ -7202,7 +7202,7 @@ public void testExpandedLinkApiRaisesError() throws Exception {
")");
initializeSkyframeExecutor();
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:foo"));
assertThat(e).hasMessageThat().contains("Rule in 'b' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}
}

Expand All @@ -7226,7 +7226,7 @@ public void testExpandedCcCompilationOutputsApiRaisesError() throws Exception {
")");
invalidatePackages();
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:foo"));
assertThat(e).hasMessageThat().contains("Rule in 'b' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}
}

Expand Down Expand Up @@ -7354,7 +7354,7 @@ public void testExpandedLibraryToLinkApiRaisesError() throws Exception {
")");
invalidatePackages();
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:foo"));
assertThat(e).hasMessageThat().contains("Rule in 'b' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}
}

Expand Down Expand Up @@ -7437,7 +7437,7 @@ public void testExpandedLinkstampApiRaisesError() throws Exception {
")");
invalidatePackages();
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:foo"));
assertThat(e).hasMessageThat().contains("Rule in 'b' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}
}

Expand Down Expand Up @@ -8093,7 +8093,7 @@ public void testGetBuildInfoArtifactsIsPrivateApi() throws Exception {
AssertionError e =
assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:custom"));

assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}

@Test
Expand Down Expand Up @@ -8165,7 +8165,7 @@ public void testCheckPrivateApiAllowlistBlocksPrivateParameter() throws Exceptio
AssertionError e =
assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:custom"));

assertThat(e).hasMessageThat().contains("Rule in 'foo' cannot use private API");
assertThat(e).hasMessageThat().contains("cannot use private API");
}

@Test
Expand Down

0 comments on commit c06fe5c

Please sign in to comment.