diff options
4 files changed, 145 insertions, 18 deletions
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 d6b6a2b2f1..c43404e3b9 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 @@ public final class CcLibraryHelper { 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 9d2ef270cf..aab459989c 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 @@ public class CcLinkingOutputs { } /** + * 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 b585dc8655..ace2d9d4cf 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 @@ public class CcCommonTest extends BuildViewTestCase { 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 08bbc5d92e..45c7bd5a74 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.ConfiguredTarget; 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 @@ public class CcLibraryConfiguredTargetTest extends BuildViewTestCase { 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'])"); + } } |