Skip to content

Commit

Permalink
Move --linkopt flags into user_link_flags
Browse files Browse the repository at this point in the history 
    Previous behavior was to put flags coming from Bazel option --linkopt into
    legacy_link_flags. They should be in user_link_flags instead (together with
    flags coming from linkopts rule attribute). This cl introduces
    --experimental_linkopts_in_user_link_flags option that flips the behavior.

    There is another incompatible change. Previously cc_common.create_link_variables() included flags from --linkopt, with the flag flipped it doesn't anymore. I believe
    --linkopt flags shouldn't be there by default because:
    * We don't tie the API with the specifics of C++ rules/options, enabling theoretical use with other languages (objc)
    * Users are free to use ctx.fragments.cpp to access C++ options and add them explicitly (bazelbuild/bazel#5602)
    * New behavior maintains the symmetry with --copt and user_compile_flags

    RELNOTES: None.
    PiperOrigin-RevId: 205274272
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent bd3c42a commit d2f9754
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 69 deletions.
4 changes: 4 additions & 0 deletions ...elbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,10 @@ public boolean getUseInterfaceSharedObjects() {
return cppOptions.useInterfaceSharedObjects;
}

public boolean getExpandLinkoptsLabels() {
return cppOptions.expandLinkoptsLabels;
}

public boolean getEnableCcSkylarkApi() {
return cppOptions.enableCcSkylarkApi;
}
Expand Down
2 changes: 1 addition & 1 deletion ...ild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1080,7 +1080,7 @@ public CppLinkAction build() throws InterruptedException {
ImmutableSet.Builder<String> executionRequirements = ImmutableSet.builder();
if (featureConfiguration.actionIsConfigured(getActionName())) {
executionRequirements.addAll(
featureConfiguration.getToolRequirementsForAction(getActionName()));
featureConfiguration.getToolForAction(getActionName()).getExecutionRequirements());
}

if (!isLtoIndexing) {
Expand Down
9 changes: 9 additions & 0 deletions ...va/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,15 @@ public Label getFdoPrefetchHintsLabel() {
)
public boolean useLLVMCoverageMapFormat;

@Option(
name = "experimental_expand_linkopts_labels",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
help = "If true, entries in linkopts that are not preceded by - or $ will be expanded.")
public boolean expandLinkoptsLabels;

@Option(
name = "incompatible_enable_legacy_cpp_toolchain_skylark_api",
defaultValue = "true",
Expand Down
6 changes: 6 additions & 0 deletions ...build.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ public enum LinkBuildVariables {
FORCE_PIC("force_pic"),
/** Presence of this variable indicates that the debug symbols should be stripped. */
STRIP_DEBUG_SYMBOLS("strip_debug_symbols"),
@Deprecated
IS_CC_TEST_LINK_ACTION("is_cc_test_link_action"),
@Deprecated
IS_NOT_CC_TEST_LINK_ACTION("is_not_cc_test_link_action"),
/** Truthy when current action is a cc_test linking action, falsey otherwise. */
IS_CC_TEST("is_cc_test"),
/**
Expand Down Expand Up @@ -145,8 +149,10 @@ public static CcToolchainVariables setupVariables(

if (useTestOnlyFlags) {
buildVariables.addIntegerVariable(IS_CC_TEST.getVariableName(), 1);
buildVariables.addStringVariable(IS_CC_TEST_LINK_ACTION.getVariableName(), "");
} else {
buildVariables.addIntegerVariable(IS_CC_TEST.getVariableName(), 0);
buildVariables.addStringVariable(IS_NOT_CC_TEST_LINK_ACTION.getVariableName(), "");
}

if (runtimeLibrarySearchDirectories != null) {
Expand Down
84 changes: 16 additions & 68 deletions ...d.bazel/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,12 @@
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.packages.util.MockCcSupport;
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.LibraryToLinkValue;
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.VariableValue;
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
import java.io.IOException;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -38,15 +35,6 @@
@RunWith(JUnit4.class)
public class LinkBuildVariablesTest extends LinkBuildVariablesTestCase {

@Before
public void createFooFooCcLibraryForRuleContext() throws IOException {
scratch.file("foo/BUILD", "cc_library(name = 'foo')");
}

private RuleContext getRuleContext() throws Exception {
return getRuleContext(getConfiguredTarget("//foo:foo"));
}

@Test
public void testForcePicBuildVariable() throws Exception {
useConfiguration("--force_pic");
Expand All @@ -56,8 +44,7 @@ public void testForcePicBuildVariable() throws Exception {
ConfiguredTarget target = getConfiguredTarget("//x:bin");
CcToolchainVariables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
String variableValue =
getVariableValue(
getRuleContext(), variables, LinkBuildVariables.FORCE_PIC.getVariableName());
getVariableValue(variables, LinkBuildVariables.FORCE_PIC.getVariableName());
assertThat(variableValue).contains("");
}

Expand Down Expand Up @@ -103,9 +90,7 @@ public void testLibrarySearchDirectoriesAreExported() throws Exception {
CcToolchainVariables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
List<String> variableValue =
getSequenceVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.LIBRARY_SEARCH_DIRECTORIES.getVariableName());
variables, LinkBuildVariables.LIBRARY_SEARCH_DIRECTORIES.getVariableName());
assertThat(Iterables.getOnlyElement(variableValue)).contains("some-dir");
}

Expand All @@ -120,8 +105,7 @@ public void testLinkerParamFileIsExported() throws Exception {
ConfiguredTarget target = getConfiguredTarget("//x:bin");
CcToolchainVariables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
String variableValue =
getVariableValue(
getRuleContext(), variables, LinkBuildVariables.LINKER_PARAM_FILE.getVariableName());
getVariableValue(variables, LinkBuildVariables.LINKER_PARAM_FILE.getVariableName());
assertThat(variableValue).matches(".*bin/x/bin" + "-2.params$");
}

Expand All @@ -142,25 +126,14 @@ public void testInterfaceLibraryBuildingVariablesWhenGenerationPossible() throws
getLinkBuildVariables(target, LinkTargetType.NODEPS_DYNAMIC_LIBRARY);

String interfaceLibraryBuilder =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_BUILDER.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_BUILDER.getVariableName());
String interfaceLibraryInput =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_INPUT.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_INPUT.getVariableName());
String interfaceLibraryOutput =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_OUTPUT.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_OUTPUT.getVariableName());
String generateInterfaceLibrary =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.GENERATE_INTERFACE_LIBRARY.getVariableName());
variables, LinkBuildVariables.GENERATE_INTERFACE_LIBRARY.getVariableName());

assertThat(generateInterfaceLibrary).isEqualTo("yes");
assertThat(interfaceLibraryInput).endsWith("libfoo.so");
Expand All @@ -172,7 +145,6 @@ public void testInterfaceLibraryBuildingVariablesWhenGenerationPossible() throws
public void testNoIfsoBuildingWhenWhenThinLtoIndexing() throws Exception {
// Make sure the interface shared object generation is enabled in the configuration
// (which it is not by default for some windows toolchains)
invalidatePackages(true);
AnalysisMock.get()
.ccSupport()
.setupCrosstool(
Expand Down Expand Up @@ -202,25 +174,14 @@ public void testNoIfsoBuildingWhenWhenThinLtoIndexing() throws Exception {
CcToolchainVariables variables = indexAction.getLinkCommandLine().getBuildVariables();

String interfaceLibraryBuilder =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_BUILDER.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_BUILDER.getVariableName());
String interfaceLibraryInput =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_INPUT.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_INPUT.getVariableName());
String interfaceLibraryOutput =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_OUTPUT.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_OUTPUT.getVariableName());
String generateInterfaceLibrary =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.GENERATE_INTERFACE_LIBRARY.getVariableName());
variables, LinkBuildVariables.GENERATE_INTERFACE_LIBRARY.getVariableName());

assertThat(generateInterfaceLibrary).isEqualTo("no");
assertThat(interfaceLibraryInput).endsWith("ignored");
Expand All @@ -244,25 +205,14 @@ public void testInterfaceLibraryBuildingVariablesWhenGenerationNotAllowed() thro
CcToolchainVariables variables = getLinkBuildVariables(target, LinkTargetType.STATIC_LIBRARY);

String interfaceLibraryBuilder =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_BUILDER.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_BUILDER.getVariableName());
String interfaceLibraryInput =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_INPUT.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_INPUT.getVariableName());
String interfaceLibraryOutput =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.INTERFACE_LIBRARY_OUTPUT.getVariableName());
getVariableValue(variables, LinkBuildVariables.INTERFACE_LIBRARY_OUTPUT.getVariableName());
String generateInterfaceLibrary =
getVariableValue(
getRuleContext(),
variables,
LinkBuildVariables.GENERATE_INTERFACE_LIBRARY.getVariableName());
variables, LinkBuildVariables.GENERATE_INTERFACE_LIBRARY.getVariableName());

assertThat(generateInterfaceLibrary).isEqualTo("no");
assertThat(interfaceLibraryInput).endsWith("ignored");
Expand All @@ -281,9 +231,7 @@ public void testOutputExecpath() throws Exception {
CcToolchainVariables variables =
getLinkBuildVariables(target, LinkTargetType.NODEPS_DYNAMIC_LIBRARY);

assertThat(
getVariableValue(
getRuleContext(), variables, LinkBuildVariables.OUTPUT_EXECPATH.getVariableName()))
assertThat(getVariableValue(variables, LinkBuildVariables.OUTPUT_EXECPATH.getVariableName()))
.endsWith("x/libfoo.so");
}

Expand Down

0 comments on commit d2f9754

Please sign in to comment.