From 942f7cf6a0da0a4ecc804615424f039e50963933 Mon Sep 17 00:00:00 2001 From: plf Date: Thu, 9 May 2019 06:26:08 -0700 Subject: [PATCH] C++: Fixes bug in C++ API with external repo aspects Reported in: https://github.com/bazelbuild/bazel/issues/8254 Added test testApiWithAspectsOnTargetsInExternalRepos() RELNOTES:none PiperOrigin-RevId: 247412486 --- .../build/lib/rules/cpp/CcModule.java | 6 +- .../lib/rules/cpp/SkylarkCcCommonTest.java | 142 ++++++++++++------ 2 files changed, 105 insertions(+), 43 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index 2ef48f6cbc2e94..f7a93a153cfc5e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java @@ -1442,7 +1442,11 @@ protected Label getCallerLabel(Location location, SkylarkActionFactory actions, try { label = Label.create( - actions.getActionConstructionContext().getActionOwner().getLabel().getPackageName(), + actions + .getActionConstructionContext() + .getActionOwner() + .getLabel() + .getPackageIdentifier(), name); } catch (LabelSyntaxException e) { throw new EvalException(location, e); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java index 344bbbfe011320..22ec1a0cb5d650 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java @@ -56,6 +56,7 @@ import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.MakeVariable; @@ -5197,6 +5198,71 @@ private static void createFiles(Scratch scratch, String bzlFilePath) throws Exce createFiles(scratch, bzlFilePath, "", ""); } + @Test + public void testTransitiveLinkWithDeps() throws Exception { + setupTestTransitiveLink(scratch, " linking_contexts = dep_linking_contexts"); + ConfiguredTarget target = getConfiguredTarget("//foo:bin"); + assertThat(target).isNotNull(); + Artifact executable = (Artifact) getMyInfoFromTarget(target).getValue("executable"); + assertThat(artifactsToStrings(getGeneratingAction(executable).getInputs())) + .containsAllOf("bin foo/libdep1.a", "bin foo/libdep2.a"); + } + + @Test + public void testTransitiveLinkForDynamicLibrary() throws Exception { + setupTestTransitiveLink(scratch, "output_type = 'dynamic_library'"); + ConfiguredTarget target = getConfiguredTarget("//foo:bin"); + assertThat(target).isNotNull(); + LibraryToLink library = (LibraryToLink) getMyInfoFromTarget(target).getValue("library"); + assertThat(library).isNotNull(); + Object executable = getMyInfoFromTarget(target).getValue("executable"); + assertThat(EvalUtils.isNullOrNone(executable)).isTrue(); + } + + @Test + public void testTransitiveLinkForExecutable() throws Exception { + setupTestTransitiveLink(scratch, "output_type = 'executable'"); + ConfiguredTarget target = getConfiguredTarget("//foo:bin"); + assertThat(target).isNotNull(); + Artifact executable = (Artifact) getMyInfoFromTarget(target).getValue("executable"); + assertThat(executable).isNotNull(); + Object library = getMyInfoFromTarget(target).getValue("library"); + assertThat(EvalUtils.isNullOrNone(library)).isTrue(); + } + + @Test + public void testTransitiveLinkWithCompilationOutputs() throws Exception { + setupTestTransitiveLink(scratch, "compilation_outputs=objects"); + ConfiguredTarget target = getConfiguredTarget("//foo:bin"); + assertThat(target).isNotNull(); + Artifact executable = (Artifact) getMyInfoFromTarget(target).getValue("executable"); + assertThat(artifactsToStrings(getGeneratingAction(executable).getInputs())) + .contains("src foo/file.o"); + } + + @Test + public void testApiWithAspectsOnTargetsInExternalRepos() throws Exception { + if (!AnalysisMock.get().isThisBazel()) { + return; + } + createFilesForTestingCompilation( + scratch, "tools/build_defs/foo", /* compileProviderLines= */ ""); + FileSystemUtils.appendIsoLatin1( + scratch.resolve("WORKSPACE"), "local_repository(name='r', path='/r')"); + scratch.file("/r/WORKSPACE"); + scratch.file("/r/p/BUILD", "cc_library(", " name = 'a',", " srcs = ['a.cc'],", ")"); + invalidatePackages(); + scratch.file( + "b/BUILD", + "load('//tools/build_defs/foo:extension.bzl', 'cc_skylark_library')", + "cc_skylark_library(", + " name = 'b',", + " srcs = ['b.cc'],", + " aspect_deps = ['@r//p:a']", + ")"); + assertThat(getConfiguredTarget("//b:b")).isNotNull(); + } + private static void createFiles( Scratch scratch, String bzlFilePath, String compileProviderLines, String linkProviderLines) throws Exception { @@ -5208,6 +5274,39 @@ private static void createFiles( scratch.file( bzlFilePath + "/extension.bzl", "load('//myinfo:myinfo.bzl', 'MyInfo')", + "def _cc_aspect_impl(target, ctx):", + " toolchain = ctx.attr._cc_toolchain[cc_common.CcToolchainInfo]", + " feature_configuration = cc_common.configure_features(", + " cc_toolchain = toolchain,", + " requested_features = ctx.features,", + " unsupported_features = ctx.disabled_features,", + " )", + " (compilation_context, compilation_outputs) = cc_common.compile(", + " actions = ctx.actions,", + " feature_configuration = feature_configuration,", + " cc_toolchain = toolchain,", + " name = ctx.label.name + '_aspect',", + " srcs = ctx.rule.files.srcs,", + " public_hdrs = ctx.rule.files.hdrs,", + " )", + " (linking_context, linking_outputs) = (", + " cc_common.create_linking_context_from_compilation_outputs(", + " actions = ctx.actions,", + " feature_configuration = feature_configuration,", + " name = ctx.label.name + '_aspect',", + " cc_toolchain = toolchain,", + " compilation_outputs = compilation_outputs,", + " )", + " )", + " return []", + "_cc_aspect = aspect(", + " implementation = _cc_aspect_impl,", + " attrs = {", + " '_cc_toolchain': attr.label(default =" + + " '@bazel_tools//tools/cpp:current_cc_toolchain'),", + " },", + fragments, + ")", "def _cc_skylark_library_impl(ctx):", " dep_compilation_contexts = []", " dep_linking_contexts = []", @@ -5262,6 +5361,7 @@ private static void createFiles( " '_additional_inputs': attr.label_list(allow_files=True," + " default=['//foo:script.lds']),", " '_deps': attr.label_list(default=['//foo:dep1', '//foo:dep2']),", + " 'aspect_deps': attr.label_list(aspects=[_cc_aspect]),", " '_cc_toolchain': attr.label(default =", " configuration_field(fragment = 'cpp', name = 'cc_toolchain'))", " },", @@ -5399,48 +5499,6 @@ private void setupCcOutputsTest() throws Exception { ")"); } - @Test - public void testTransitiveLinkWithDeps() throws Exception { - setupTestTransitiveLink(scratch, " linking_contexts = dep_linking_contexts"); - ConfiguredTarget target = getConfiguredTarget("//foo:bin"); - assertThat(target).isNotNull(); - Artifact executable = (Artifact) getMyInfoFromTarget(target).getValue("executable"); - assertThat(artifactsToStrings(getGeneratingAction(executable).getInputs())) - .containsAllOf("bin foo/libdep1.a", "bin foo/libdep2.a"); - } - - @Test - public void testTransitiveLinkForDynamicLibrary() throws Exception { - setupTestTransitiveLink(scratch, "output_type = 'dynamic_library'"); - ConfiguredTarget target = getConfiguredTarget("//foo:bin"); - assertThat(target).isNotNull(); - LibraryToLink library = (LibraryToLink) getMyInfoFromTarget(target).getValue("library"); - assertThat(library).isNotNull(); - Object executable = getMyInfoFromTarget(target).getValue("executable"); - assertThat(EvalUtils.isNullOrNone(executable)).isTrue(); - } - - @Test - public void testTransitiveLinkForExecutable() throws Exception { - setupTestTransitiveLink(scratch, "output_type = 'executable'"); - ConfiguredTarget target = getConfiguredTarget("//foo:bin"); - assertThat(target).isNotNull(); - Artifact executable = (Artifact) getMyInfoFromTarget(target).getValue("executable"); - assertThat(executable).isNotNull(); - Object library = getMyInfoFromTarget(target).getValue("library"); - assertThat(EvalUtils.isNullOrNone(library)).isTrue(); - } - - @Test - public void testTransitiveLinkWithCompilationOutputs() throws Exception { - setupTestTransitiveLink(scratch, "compilation_outputs=objects"); - ConfiguredTarget target = getConfiguredTarget("//foo:bin"); - assertThat(target).isNotNull(); - Artifact executable = (Artifact) getMyInfoFromTarget(target).getValue("executable"); - assertThat(artifactsToStrings(getGeneratingAction(executable).getInputs())) - .contains("src foo/file.o"); - } - private static void setupTestTransitiveLink(Scratch scratch, String... additionalLines) throws Exception { String fragments = " fragments = ['google_cpp', 'cpp'],";