Stop input and output of cc_library from clobbering each other.
Before this change:
Any given cc_library can only contribute one library with a given name to
targets which depend on it. If an input library has the same name as the
cc_library which it is an input to, the decision of which to use is based
on the link mode. e.g.,
cc_library(
name = "foo",
srcs = ["foo.c", "libfoo.so"],
)
will only contribute libfoo.a (a static library containing foo.o) in static mode,
while it will only contribute libfoo.so (the precompiled shared library) in dynamic
mode.
This change alters cc_library's behavior in this case:
* If libfoo.a would be empty (i.e., there are no linkable sources), then
this is allowed. The libfoo.so from srcs is simply passed through. (Previously,
the empty libfoo.a would be forwarded.)
* Otherwise, this is an error.
In the case where there are multiple libraries in the srcs with the same
library identifier (lib[name].[a|so|lo]), cc_library will still choose one
based on the link mode. This behavior has not changed.
Similarly, cc_library will still choose one of its own outputs based on the
link mode. That behavior has not changed either.
RELNOTES[INC]: It is now an error to include a precompiled library (.a, .lo, .so)
in a cc_library which would generate a library with the same name
(e.g., libfoo.so in cc_library foo) if that library also contains other linkable
sources.
--
MOS_MIGRATED_REVID=127569615
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index d6b6a2b..c43404e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.rules.cpp;
import com.google.common.base.Function;
+import com.google.common.base.Joiner;
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -913,14 +914,50 @@
CcLinkingOutputs originalLinkingOutputs = ccLinkingOutputs;
if (!(
staticLibraries.isEmpty() && picStaticLibraries.isEmpty() && dynamicLibraries.isEmpty())) {
+
+ CcLinkingOutputs.Builder newOutputsBuilder = new CcLinkingOutputs.Builder();
+ if (!ccOutputs.isEmpty()) {
+ // Add the linked outputs of this rule iff we had anything to put in them, but then
+ // make sure we're not colliding with some library added from the inputs.
+ newOutputsBuilder.merge(originalLinkingOutputs);
+ Iterable<LibraryToLink> allLibraries =
+ Iterables.concat(staticLibraries, picStaticLibraries, dynamicLibraries);
+ for (LibraryToLink precompiledLibrary : allLibraries) {
+ List<LibraryToLink> matchingLibs =
+ originalLinkingOutputs.getLibrariesWithSameIdentifierAs(precompiledLibrary);
+ if (!matchingLibs.isEmpty()) {
+ Iterable<String> matchingLibArtifactNames =
+ Iterables.transform(
+ matchingLibs,
+ new Function<LibraryToLink, String>() {
+ @Override
+ public String apply(LibraryToLink input) {
+ return input.getOriginalLibraryArtifact().getFilename();
+ }
+ });
+ ruleContext.ruleError(
+ "Can't put "
+ + precompiledLibrary.getArtifact().getFilename()
+ + " into the srcs of a "
+ + ruleContext.getRuleClassNameForLogging()
+ + " with the same name ("
+ + ruleContext.getRule().getName()
+ + ") which also contains other code or objects to link; it shares a name with "
+ + Joiner.on(", ").join(matchingLibArtifactNames)
+ + " (output compiled and linked from the non-library sources of this rule), "
+ + "which could cause confusion");
+ }
+ }
+ }
+
// Merge the pre-compiled libraries (static & dynamic) into the linker outputs.
- ccLinkingOutputs = new CcLinkingOutputs.Builder()
- .merge(ccLinkingOutputs)
- .addStaticLibraries(staticLibraries)
- .addPicStaticLibraries(picStaticLibraries)
- .addDynamicLibraries(dynamicLibraries)
- .addExecutionDynamicLibraries(dynamicLibraries)
- .build();
+ ccLinkingOutputs =
+ newOutputsBuilder
+ .addStaticLibraries(staticLibraries)
+ .addPicStaticLibraries(picStaticLibraries)
+ .addDynamicLibraries(dynamicLibraries)
+ .addExecutionDynamicLibraries(dynamicLibraries)
+ .build();
}
DwoArtifactsCollector dwoArtifacts =
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingOutputs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingOutputs.java
index 9d2ef27..aab4599 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingOutputs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingOutputs.java
@@ -69,6 +69,24 @@
}
/**
+ * Returns all libraries in this CcLinkingOutputs with the same library identifier - i.e., those
+ * which would be considered different forms of the same library by getPreferredLibrary.
+ */
+ public List<LibraryToLink> getLibrariesWithSameIdentifierAs(LibraryToLink input) {
+ Iterable<LibraryToLink> allLibraries =
+ Iterables.concat(
+ staticLibraries, picStaticLibraries, dynamicLibraries, executionDynamicLibraries);
+ ImmutableList.Builder<LibraryToLink> result = new ImmutableList.Builder<>();
+ for (LibraryToLink library : allLibraries) {
+ if (libraryIdentifierOf(library.getOriginalLibraryArtifact())
+ .equals(libraryIdentifierOf(input.getOriginalLibraryArtifact()))) {
+ result.add(library);
+ }
+ }
+ return result.build();
+ }
+
+ /**
* Add the ".a", ".pic.a" and/or ".so" files in appropriate order of preference depending on the
* link preferences.
*
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
index b585dc8..ace2d9d 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
@@ -623,19 +623,8 @@
Iterable<Artifact> libraries = getLinkerInputs(wrapsophos);
- // The "libsavi.a" below is the empty ".a" file created by Blaze for the
- // "savi" cc_library rule (empty since it has no ".cc" files in "srcs").
- // The "libsavi.so" below is the "lib/libsavi.so" file from "srcs".
- //
- // TODO(blaze-team): (2009) the order here is a bit odd; it would make more sense
- // to put the library for the rule ("libsavi.a") before the ".so" file
- // from "srcs" ("libsavi.so"). I think this is because we currently
- // list all the .so files for a rule before all the .a files for the rule.
assertThat(baseArtifactNames(libraries))
.containsAllOf("wrapsophos.pic.o", "libsophosengine.a", "libsavi.so");
- if (emptyShouldOutputStaticLibrary()) {
- assertThat(baseArtifactNames(libraries)).contains("libsavi.a");
- }
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 08bbc5d..45c7bd5 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -36,6 +36,7 @@
import com.google.devtools.build.lib.analysis.OutputGroupProvider;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
import com.google.devtools.build.lib.packages.util.MockCcSupport;
import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
import com.google.devtools.build.lib.testutil.TestConstants;
@@ -1009,4 +1010,86 @@
Artifact soOutput = getBinArtifact("libfoo.so", target);
assertThat(getGeneratingAction(soOutput)).isInstanceOf(FailAction.class);
}
+
+ @Test
+ public void alwaysAddStaticAndDynamicLibraryToFilesToBuildWhenBuilding() throws Exception {
+ ConfiguredTarget target =
+ scratchConfiguredTarget("a", "b", "cc_library(name = 'b', srcs = ['source.cc'])");
+
+ assertThat(artifactsToStrings(getFilesToBuild(target)))
+ .containsExactly("bin a/libb.a", "bin a/libb.ifso", "bin a/libb.so");
+ }
+
+ @Test
+ public void addOnlyStaticLibraryToFilesToBuildWhenWrappingIffImplicitOutput() throws Exception {
+ // This shared library has the same name as the archive generated by this rule, so it should
+ // override said archive. However, said archive should still be put in files to build.
+ ConfiguredTarget target =
+ scratchConfiguredTarget("a", "b", "cc_library(name = 'b', srcs = ['libb.so'])");
+
+ if (target.getTarget().getAssociatedRule().getRuleClassObject().getImplicitOutputsFunction()
+ != ImplicitOutputsFunction.NONE) {
+ assertThat(artifactsToStrings(getFilesToBuild(target))).containsExactly("bin a/libb.a");
+ } else {
+ assertThat(artifactsToStrings(getFilesToBuild(target))).isEmpty();
+ }
+ }
+
+ @Test
+ public void addStaticLibraryToStaticSharedLinkParamsWhenBuilding() throws Exception {
+ ConfiguredTarget target =
+ scratchConfiguredTarget("a", "foo", "cc_library(name = 'foo', srcs = ['foo.cc'])");
+
+ Iterable<Artifact> libraries =
+ LinkerInputs.toNonSolibArtifacts(
+ target
+ .getProvider(CcLinkParamsProvider.class)
+ .getCcLinkParams(true, true)
+ .getLibraries());
+ assertThat(artifactsToStrings(libraries)).contains("bin a/libfoo.a");
+ }
+
+ @Test
+ public void dontAddStaticLibraryToStaticSharedLinkParamsWhenWrappingSameLibraryIdentifier()
+ throws Exception {
+ ConfiguredTarget target =
+ scratchConfiguredTarget("a", "foo", "cc_library(name = 'foo', srcs = ['libfoo.so'])");
+
+ Iterable<Artifact> libraries =
+ LinkerInputs.toNonSolibArtifacts(
+ target
+ .getProvider(CcLinkParamsProvider.class)
+ .getCcLinkParams(true, true)
+ .getLibraries());
+ assertThat(artifactsToStrings(libraries)).doesNotContain("bin a/libfoo.a");
+ assertThat(artifactsToStrings(libraries)).contains("src a/libfoo.so");
+ }
+
+ @Test
+ public void onlyAddOneWrappedLibraryWithSameLibraryIdentifierToLinkParams() throws Exception {
+ ConfiguredTarget target =
+ scratchConfiguredTarget(
+ "a", "foo", "cc_library(name = 'foo', srcs = ['libfoo.lo', 'libfoo.so'])");
+
+ Iterable<Artifact> libraries =
+ LinkerInputs.toNonSolibArtifacts(
+ target
+ .getProvider(CcLinkParamsProvider.class)
+ .getCcLinkParams(true, true)
+ .getLibraries());
+ assertThat(artifactsToStrings(libraries)).doesNotContain("src a/libfoo.so");
+ assertThat(artifactsToStrings(libraries)).contains("src a/libfoo.lo");
+ }
+
+ @Test
+ public void forbidBuildingAndWrappingSameLibraryIdentifier() throws Exception {
+ checkError(
+ "a",
+ "foo",
+ "in cc_library rule //a:foo: Can't put libfoo.lo into the srcs of a cc_library with the "
+ + "same name (foo) which also contains other code or objects to link; it shares a name "
+ + "with libfoo.a, libfoo.ifso, libfoo.so (output compiled and linked from the "
+ + "non-library sources of this rule), which could cause confusion",
+ "cc_library(name = 'foo', srcs = ['foo.cc', 'libfoo.lo'])");
+ }
}
