From f61d12e9e4f940810efbaf244911a94830ba6c05 Mon Sep 17 00:00:00 2001 From: Cal Peyser Date: Tue, 26 Jul 2016 14:50:52 +0000 Subject: Refactor CppLinkAction to construct its command line using the crosstool instead of a hardcoded switch. Add action configs to CppConfiguration for each link type. -- MOS_MIGRATED_REVID=128470872 --- .../build/lib/rules/cpp/CppLinkActionTest.java | 120 ++++++++++++++++-- .../lib/rules/cpp/LinkBuildVariablesTest.java | 134 +++++++++++++++++++++ 2 files changed, 242 insertions(+), 12 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java (limited to 'src/test/java/com/google/devtools/build') diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index c4ea2be93f..7531632e8a 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java @@ -17,8 +17,8 @@ package com.google.devtools.build.lib.rules.cpp; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; @@ -34,18 +34,17 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; +import com.google.devtools.build.lib.rules.cpp.CppLinkActionConfigs.CppLinkPlatform; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink; import com.google.devtools.build.lib.vfs.PathFragment; - +import java.io.IOException; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Tests for {@link CppLinkAction}. - */ +/** Tests for {@link CppLinkAction}. */ @RunWith(JUnit4.class) public class CppLinkActionTest extends BuildViewTestCase { private RuleContext createDummyRuleContext() throws Exception { @@ -66,6 +65,18 @@ public class CppLinkActionTest extends BuildViewTestCase { } }, masterConfig); } + + private final FeatureConfiguration getMockFeatureConfiguration() throws Exception { + return CcToolchainFeaturesTest.buildFeatures( + CppLinkActionConfigs.getCppLinkActionConfigs(CppLinkPlatform.LINUX)) + .getFeatureConfiguration( + Link.LinkTargetType.EXECUTABLE.getActionName(), + Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(), + Link.LinkTargetType.STATIC_LIBRARY.getActionName(), + Link.LinkTargetType.PIC_STATIC_LIBRARY.getActionName(), + Link.LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY.getActionName(), + Link.LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY.getActionName()); + } @Test public void testToolchainFeatureFlags() throws Exception { @@ -93,9 +104,9 @@ public class CppLinkActionTest extends BuildViewTestCase { "out", ImmutableList.of(), ImmutableList.of(), - featureConfiguration) + featureConfiguration, + false) .build(); - assertThat(Joiner.on(" ").join(linkAction.getArgv())).contains("mock_tool"); assertThat(linkAction.getArgv()).contains("some_flag"); } @@ -118,7 +129,8 @@ public class CppLinkActionTest extends BuildViewTestCase { "out", ImmutableList.of(), ImmutableList.of(), - featureConfiguration) + featureConfiguration, + false) .build(); assertThat(linkAction.getEnvironment()).containsEntry("foo", "bar"); } @@ -135,6 +147,8 @@ public class CppLinkActionTest extends BuildViewTestCase { final Artifact oFile = getSourceArtifact("cc/a.o"); final Artifact oFile2 = getSourceArtifact("cc/a2.o"); final Artifact interfaceSoBuilder = getBinArtifactWithNoOwner("foo/build_interface_so"); + final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration(); + ActionTester.runTest( 128, new ActionCombinationFactory() { @@ -158,7 +172,7 @@ public class CppLinkActionTest extends BuildViewTestCase { builder.setWholeArchive((i & 16) == 0); builder.setFake((i & 32) == 0); builder.setRuntimeSolibDir((i & 64) == 0 ? null : new PathFragment("so1")); - builder.setFeatureConfiguration(new FeatureConfiguration()); + builder.setFeatureConfiguration(featureConfiguration); return builder.build(); } @@ -177,6 +191,8 @@ public class CppLinkActionTest extends BuildViewTestCase { final Artifact oFile = getSourceArtifact("cc/a.o"); final Artifact oFile2 = getSourceArtifact("cc/a2.o"); final Artifact interfaceSoBuilder = getBinArtifactWithNoOwner("foo/build_interface_so"); + final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration(); + ActionTester.runTest( 4, new ActionCombinationFactory() { @@ -194,6 +210,7 @@ public class CppLinkActionTest extends BuildViewTestCase { (i & 1) == 0 ? ImmutableList.of(oFile) : ImmutableList.of(oFile2)); builder.setLinkType( (i & 2) == 0 ? LinkTargetType.STATIC_LIBRARY : LinkTargetType.DYNAMIC_LIBRARY); + builder.setFeatureConfiguration(featureConfiguration); return builder.build(); } }); @@ -254,7 +271,8 @@ public class CppLinkActionTest extends BuildViewTestCase { "binary2", objects.build(), ImmutableList.of(), - new FeatureConfiguration()) + new FeatureConfiguration(), + false) .setFake(true) .build(); @@ -285,7 +303,8 @@ public class CppLinkActionTest extends BuildViewTestCase { String outputPath, Iterable nonLibraryInputs, ImmutableList libraryInputs, - FeatureConfiguration featureConfiguration) + FeatureConfiguration featureConfiguration, + boolean shouldIncludeToolchain) throws Exception { RuleContext ruleContext = createDummyRuleContext(); CppLinkActionBuilder builder = @@ -294,7 +313,7 @@ public class CppLinkActionTest extends BuildViewTestCase { new Artifact( new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()), ruleContext.getConfiguration(), - null) + shouldIncludeToolchain ? CppHelper.getToolchain(ruleContext) : null) .addNonLibraryInputs(nonLibraryInputs) .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs)) .setLinkType(type) @@ -306,6 +325,17 @@ public class CppLinkActionTest extends BuildViewTestCase { .setFeatureConfiguration(featureConfiguration); return builder; } + + private CppLinkActionBuilder createLinkBuilder(Link.LinkTargetType type) throws Exception { + PathFragment output = new PathFragment("dummyRuleContext/output/path.xyz"); + return createLinkBuilder( + type, + output.getPathString(), + ImmutableList.of(), + ImmutableList.of(), + new FeatureConfiguration(), + true); + } public Artifact getOutputArtifact(String relpath) { return new Artifact( @@ -313,4 +343,70 @@ public class CppLinkActionTest extends BuildViewTestCase { getTargetConfiguration().getBinDirectory(), getTargetConfiguration().getBinFragment().getRelative(relpath)); } + + private Artifact scratchArtifact(String s) { + try { + return new Artifact( + scratch.overwriteFile(outputBase.getRelative("WORKSPACE").getRelative(s).toString()), + Root.asDerivedRoot(scratch.dir(outputBase.getRelative("WORKSPACE").toString()))); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private void assertError(String expectedSubstring, CppLinkActionBuilder builder) { + try { + builder.build(); + fail(); + } catch (RuntimeException e) { + assertThat(e.getMessage()).contains(expectedSubstring); + } + } + + @Test + public void testInterfaceOutputWithoutBuildingDynamicLibraryIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.EXECUTABLE) + .setInterfaceOutput(scratchArtifact("FakeInterfaceOutput")); + + assertError("Interface output can only be used with non-fake DYNAMIC_LIBRARY targets", builder); + } + + @Test + public void testStaticLinkWithDynamicIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY).setLinkStaticness(LinkStaticness.DYNAMIC); + + assertError("static library link must be static", builder); + } + + @Test + public void testStaticLinkWithSymbolsCountOutputIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY) + .setLinkStaticness(LinkStaticness.FULLY_STATIC) + .setSymbolCountsOutput(scratchArtifact("dummySymbolCounts")); + + assertError("the symbol counts output must be null for static links", builder); + } + + @Test + public void testStaticLinkWithNativeDepsIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY) + .setLinkStaticness(LinkStaticness.FULLY_STATIC) + .setNativeDeps(true); + + assertError("the native deps flag must be false for static links", builder); + } + + @Test + public void testStaticLinkWithWholeArchiveIsError() throws Exception { + CppLinkActionBuilder builder = + createLinkBuilder(LinkTargetType.STATIC_LIBRARY) + .setLinkStaticness(LinkStaticness.FULLY_STATIC) + .setWholeArchive(true); + + assertError("the need whole archive flag must be false for static links", builder); + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java new file mode 100644 index 0000000000..50fdebad0f --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java @@ -0,0 +1,134 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.rules.cpp; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests that {@code CppLinkAction} is populated with the correct build variables. */ +@RunWith(JUnit4.class) +public class LinkBuildVariablesTest extends BuildViewTestCase { + + private CppLinkAction getCppLinkAction(ConfiguredTarget target, Link.LinkTargetType type) { + Artifact linkerOutput = null; + switch (type) { + case STATIC_LIBRARY: + case ALWAYS_LINK_STATIC_LIBRARY: + linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + ".a", target); + break; + case PIC_STATIC_LIBRARY: + case ALWAYS_LINK_PIC_STATIC_LIBRARY: + linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + "pic.a", target); + break; + case DYNAMIC_LIBRARY: + linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + ".so", target); + break; + case EXECUTABLE: + linkerOutput = getExecutable(target); + break; + default: + throw new IllegalArgumentException( + String.format("Cannot get CppLinkAction for link type %s", type)); + } + return (CppLinkAction) getGeneratingAction(linkerOutput); + } + + private Variables getLinkBuildVariables(ConfiguredTarget target, Link.LinkTargetType type) { + return getCppLinkAction(target, type).getLinkCommandLine().getBuildVariables(); + } + + private List getVariableValue(Variables variables, String variable) throws Exception { + FeatureConfiguration mockFeatureConfiguration = + CcToolchainFeaturesTest.buildFeatures( + "feature {", + " name: 'a'", + " flag_set {", + " action: 'foo'", + " flag_group {", + " flag: '%{" + variable + "}'", + " }", + " }", + "}") + .getFeatureConfiguration("a"); + return mockFeatureConfiguration.getCommandLine("foo", variables); + } + + @Test + public void testLinkstampBuildVariable() throws Exception { + scratch.file( + "x/BUILD", + "cc_binary(", + " name = 'bin',", + " srcs = ['a.cc'],", + " deps = [':lib'],", + ")", + "cc_library(", + " name = 'lib',", + " srcs = ['b.cc'],", + " linkstamp = 'c.cc',", + ")"); + scratch.file("x/a.cc"); + scratch.file("x/b.cc"); + scratch.file("x/c.cc"); + + ConfiguredTarget target = getConfiguredTarget("//x:bin"); + Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); + List variableValue = + getVariableValue(variables, CppLinkActionBuilder.LINKSTAMP_PATHS_VARIABLE); + assertThat(Iterables.getOnlyElement(variableValue)).contains("c.o"); + } + + @Test + public void testForcePicBuildVariable() throws Exception { + useConfiguration("--force_pic"); + scratch.file("x/BUILD", "cc_binary(", " name = 'bin',", " srcs = ['a.cc'],", ")"); + scratch.file("x/a.cc"); + + ConfiguredTarget target = getConfiguredTarget("//x:bin"); + Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); + List variableValue = + getVariableValue(variables, CppLinkActionBuilder.FORCE_PIC_VARIABLE); + assertThat(variableValue).contains(""); + } + + @Test + public void testWholeArchiveBuildVariables() throws Exception { + scratch.file( + "x/BUILD", + "cc_binary(", + " name = 'bin.so',", + " srcs = ['a.cc'],", + " linkopts = ['-shared'],", + " linkstatic = 1", + ")"); + scratch.file("x/a.cc"); + + ConfiguredTarget target = getConfiguredTarget("//x:bin.so"); + Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE); + List variableValue = + getVariableValue(variables, CppLinkActionBuilder.GLOBAL_WHOLE_ARCHIVE_VARIABLE); + assertThat(variableValue).contains(""); + } +} -- cgit v1.2.3