From 8058099184c470c41fce67aa02bbbfa4e098fa87 Mon Sep 17 00:00:00 2001 From: Cal Peyser Date: Thu, 19 May 2016 15:48:50 +0000 Subject: The link command line API can consume a feature configuration to configure flags and environment variables. -- MOS_MIGRATED_REVID=122735641 --- .../build/lib/rules/cpp/CppLinkAction.java | 72 ++++++++++---- .../build/lib/rules/cpp/LinkCommandLine.java | 22 +++-- .../lib/rules/cpp/CcToolchainFeaturesTest.java | 5 +- .../build/lib/rules/cpp/CppLinkActionTest.java | 104 +++++++++++++++++---- 4 files changed, 158 insertions(+), 45 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 33b1306973..3c88c70546 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -112,13 +112,14 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo /** * The name of this action for the purpose of crosstool features/action_configs */ - private static final String ACTION_NAME = "cpp-link"; + private static final String ACTION_NAME = "c++-link"; private final CppConfiguration cppConfiguration; private final LibraryToLink outputLibrary; private final LibraryToLink interfaceOutputLibrary; + private final Map toolchainEnv; private final ImmutableSet executionRequirements; - + private final LinkCommandLine linkCommandLine; /** True for cc_fake_binary targets. */ @@ -164,6 +165,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo boolean isLTOIndexing, Iterable allLTOBackendArtifacts, LinkCommandLine linkCommandLine, + Map toolchainEnv, ImmutableSet executionRequirements) { super(owner, inputs, outputs); this.mandatoryInputs = inputs; @@ -174,6 +176,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo this.isLTOIndexing = isLTOIndexing; this.allLTOBackendArtifacts = allLTOBackendArtifacts; this.linkCommandLine = linkCommandLine; + this.toolchainEnv = toolchainEnv; this.executionRequirements = executionRequirements; } @@ -209,8 +212,12 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo } public ImmutableMap getEnvironment() { + ImmutableMap.Builder result = ImmutableMap.builder(); + + result.putAll(toolchainEnv); + if (OS.getCurrent() == OS.WINDOWS) { - // TODO(bazel-team): Both GCC and clang rely on their execution directories being on + // Both GCC and clang rely on their execution directories being on // PATH, otherwise they fail to find dependent DLLs (and they fail silently...). On // the other hand, Windows documentation says that the directory of the executable // is always searched for DLLs first. Not sure what to make of it. @@ -218,13 +225,15 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo // the crosstool file. // // @see com.google.devtools.build.lib.rules.cpp.CppCompileAction#getEnvironment. - return ImmutableMap.of( + // TODO(b/28791924): Use the crosstool to provide this value. + result.put( "PATH", - cppConfiguration.getToolPathFragment(CppConfiguration.Tool.GCC).getParentDirectory() - .getPathString() - ); + cppConfiguration + .getToolPathFragment(CppConfiguration.Tool.GCC) + .getParentDirectory() + .getPathString()); } - return ImmutableMap.of(); + return result.build(); } /** @@ -424,7 +433,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo f.addString(fake ? FAKE_LINK_GUID : LINK_GUID); f.addString(getCppConfiguration().getLdExecutable().getPathString()); f.addStrings(linkCommandLine.arguments()); - f.addStrings(executionRequirements); + f.addStrings(getExecutionInfo().keySet()); // TODO(bazel-team): For correctness, we need to ensure the invariant that all values accessed // during the execution phase are also covered by the key. Above, we add the argv to the key, @@ -540,6 +549,8 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo protected final BuildConfiguration configuration; private final CppConfiguration cppConfiguration; private FeatureConfiguration featureConfiguration; + private CcToolchainFeatures.Variables buildVariables = + new CcToolchainFeatures.Variables.Builder().build(); // Morally equivalent with {@link Context}, except these are mutable. // Keep these in sync with {@link Context}. @@ -641,6 +652,14 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo this.useTestOnlyFlags = linkContext.useTestOnlyFlags; } + /** + * Returns the action name for purposes of querying the crosstool. + */ + // TODO(b/28791924): Expand action types to values in Link.LinkTargetType. + private String getActionName() { + return ACTION_NAME; + } + public CppLinkAction.Builder setLinkArtifactFactory(LinkArtifactFactory linkArtifactFactory) { this.linkArtifactFactory = linkArtifactFactory; return this; @@ -787,6 +806,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo LinkCommandLine.Builder linkCommandLineBuilder = new LinkCommandLine.Builder(configuration, getOwner(), ruleContext) + .setActionName(getActionName()) .setLinkerInputs(linkerInputs) .setRuntimeInputs( ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs))) @@ -893,17 +913,26 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo analysisEnvironment.registerAction(parameterFileWriteAction); } + + // For backwards compatibility, and for tests, + // we permit the link action to be instantiated without a feature configuration. + // In this case, an empty feature configuration is used. + if (featureConfiguration == null) { + this.featureConfiguration = new FeatureConfiguration(); + } + + Map toolchainEnv = + featureConfiguration.getEnvironmentVariables(getActionName(), buildVariables); + // If the crosstool uses action_configs to configure cc compilation, collect execution info // from there, otherwise, use no execution info. // TODO(b/27903698): Assert that the crosstool has an action_config for this action. - ImmutableSet executionRequirements = ImmutableSet.of(); - if (featureConfiguration != null) { - if (featureConfiguration.actionIsConfigured(ACTION_NAME)) { - executionRequirements = - featureConfiguration.getToolForAction(ACTION_NAME).getExecutionRequirements(); - } + ImmutableSet.Builder executionRequirements = ImmutableSet.builder(); + if (featureConfiguration.actionIsConfigured(getActionName())) { + executionRequirements.addAll( + featureConfiguration.getToolForAction(getActionName()).getExecutionRequirements()); } - + return new CppLinkAction( getOwner(), inputsBuilder.deduplicate().build(), @@ -915,7 +944,8 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo isLTOIndexing, allLTOArtifacts, linkCommandLine, - executionRequirements); + toolchainEnv, + executionRequirements.build()); } /** @@ -998,6 +1028,14 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo return this; } + /** + * Sets the build variables that will be used to template the crosstool. + */ + public Builder setBuildVariables(CcToolchainFeatures.Variables buildVariables) { + this.buildVariables = buildVariables; + return this; + } + /** * This is the LTO indexing step, rather than the real link. * diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index 6aa888369c..2a6d88d664 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java @@ -57,6 +57,7 @@ import javax.annotation.Nullable; */ @Immutable public final class LinkCommandLine extends CommandLine { + private final String actionName; private final BuildConfiguration configuration; private final CppConfiguration cppConfiguration; private final ActionOwner owner; @@ -85,14 +86,8 @@ public final class LinkCommandLine extends CommandLine { @Nullable private final Artifact paramFile; @Nullable private final Artifact interfaceSoBuilder; - - /** - * A string constant for the c++ link action, used to access the feature - * configuration. - */ - public static final String CPP_LINK = "c++-link"; - private LinkCommandLine( + String actionName, BuildConfiguration configuration, ActionOwner owner, Artifact output, @@ -140,6 +135,7 @@ public final class LinkCommandLine extends CommandLine { "the need whole archive flag must be false for static links"); } + this.actionName = actionName; this.configuration = Preconditions.checkNotNull(configuration); this.cppConfiguration = configuration.getFragment(CppConfiguration.class); this.variables = variables; @@ -691,7 +687,7 @@ public final class LinkCommandLine extends CommandLine { argv.addAll(cppConfiguration.getLinkOptions()); // The feature config can be null for tests. if (featureConfiguration != null) { - argv.addAll(featureConfiguration.getCommandLine(CPP_LINK, variables)); + argv.addAll(featureConfiguration.getCommandLine(actionName, variables)); } } @@ -1001,6 +997,7 @@ public final class LinkCommandLine extends CommandLine { private final ActionOwner owner; @Nullable private final RuleContext ruleContext; + private String actionName; @Nullable private Artifact output; @Nullable private Artifact interfaceOutput; @Nullable private Artifact symbolCountsOutput; @@ -1063,6 +1060,7 @@ public final class LinkCommandLine extends CommandLine { variables = buildVariables.build(); } return new LinkCommandLine( + actionName, configuration, owner, output, @@ -1089,6 +1087,14 @@ public final class LinkCommandLine extends CommandLine { featureConfiguration); } + /** + * Sets the name of the action for the purposes of querying the crosstool. + */ + public Builder setActionName(String actionName) { + this.actionName = actionName; + return this; + } + /** * Sets the toolchain to use for link flags. If this is not called, the toolchain * is retrieved from the rule. diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java index 8707f46565..4d64a293cb 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java @@ -77,7 +77,10 @@ public class CcToolchainFeaturesTest { return variables.build(); } - private CcToolchainFeatures buildFeatures(String... toolchain) throws Exception { + /** + * Creates a CcToolchainFeatures from features described in the given toolchain fragment. + */ + public static CcToolchainFeatures buildFeatures(String... toolchain) throws Exception { CToolchain.Builder toolchainBuilder = CToolchain.newBuilder(); TextFormat.merge(Joiner.on("").join(toolchain), toolchainBuilder); return new CcToolchainFeatures(toolchainBuilder.buildPartial()); 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 54bc4919d6..74ba8c565a 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 @@ -14,6 +14,7 @@ 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; @@ -31,6 +32,7 @@ import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinatio 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.CppLinkAction.Builder; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; @@ -65,6 +67,54 @@ public class CppLinkActionTest extends BuildViewTestCase { }, masterConfig); } + @Test + public void testToolchainFeatureFlags() throws Exception { + FeatureConfiguration featureConfiguration = + CcToolchainFeaturesTest.buildFeatures( + "feature {", + " name: 'a'", + " flag_set {", + " action: 'c++-link'", + " flag_group { flag: 'some_flag' }", + " }", + "}") + .getFeatureConfiguration("a"); + + CppLinkAction linkAction = + createLinkBuilder( + Link.LinkTargetType.EXECUTABLE, + "out", + ImmutableList.of(), + ImmutableList.of(), + featureConfiguration) + .build(); + assertThat(linkAction.getArgv()).contains("some_flag"); + } + + @Test + public void testToolchainFeatureEnv() throws Exception { + FeatureConfiguration featureConfiguration = + CcToolchainFeaturesTest.buildFeatures( + "feature {", + " name: 'a'", + " env_set {", + " action: 'c++-link'", + " env_entry { key: 'foo', value: 'bar' }", + " }", + "}") + .getFeatureConfiguration("a"); + + CppLinkAction linkAction = + createLinkBuilder( + Link.LinkTargetType.EXECUTABLE, + "out", + ImmutableList.of(), + ImmutableList.of(), + featureConfiguration) + .build(); + assertThat(linkAction.getEnvironment()).containsEntry("foo", "bar"); + } + /** * This mainly checks that non-static links don't have identical keys. Many options are only * allowed on non-static links, and we test several of them here. @@ -100,6 +150,8 @@ 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()); + return builder.build(); } }); @@ -134,6 +186,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(new FeatureConfiguration()); return builder.build(); } }); @@ -188,11 +241,15 @@ public class CppLinkActionTest extends BuildViewTestCase { objects.add(getOutputArtifact("object" + i + ".o")); } - CppLinkAction linkAction = createLinkBuilder( - Link.LinkTargetType.EXECUTABLE, "binary2", objects.build(), - ImmutableList.of()) - .setFake(true) - .build(); + CppLinkAction linkAction = + createLinkBuilder( + Link.LinkTargetType.EXECUTABLE, + "binary2", + objects.build(), + ImmutableList.of(), + new FeatureConfiguration()) + .setFake(true) + .build(); // Ensure that minima are enforced. ResourceSet resources = linkAction.estimateResourceConsumptionLocal(); @@ -215,22 +272,31 @@ public class CppLinkActionTest extends BuildViewTestCase { assertTrue(resources.getIoUsage() == CppLinkAction.MIN_STATIC_LINK_RESOURCES.getIoUsage() || resources.getIoUsage() == scaledSet.getIoUsage()); } - private Builder createLinkBuilder(Link.LinkTargetType type, String outputPath, - Iterable nonLibraryInputs, ImmutableList libraryInputs) + + private Builder createLinkBuilder( + Link.LinkTargetType type, + String outputPath, + Iterable nonLibraryInputs, + ImmutableList libraryInputs, + FeatureConfiguration featureConfiguration) throws Exception { RuleContext ruleContext = createDummyRuleContext(); - Builder builder = new CppLinkAction.Builder( - ruleContext, - new Artifact(new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()), - ruleContext.getConfiguration(), - null) - .addNonLibraryInputs(nonLibraryInputs) - .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs)) - .setLinkType(type) - .setCrosstoolInputs(NestedSetBuilder.emptySet(Order.STABLE_ORDER)) - .setLinkStaticness(type.isStaticLibraryLink() - ? LinkStaticness.FULLY_STATIC - : LinkStaticness.MOSTLY_STATIC); + Builder builder = + new CppLinkAction.Builder( + ruleContext, + new Artifact( + new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()), + ruleContext.getConfiguration(), + null) + .addNonLibraryInputs(nonLibraryInputs) + .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs)) + .setLinkType(type) + .setCrosstoolInputs(NestedSetBuilder.emptySet(Order.STABLE_ORDER)) + .setLinkStaticness( + type.isStaticLibraryLink() + ? LinkStaticness.FULLY_STATIC + : LinkStaticness.MOSTLY_STATIC) + .setFeatureConfiguration(featureConfiguration); return builder; } -- cgit v1.2.3