Skip to content

Commit

Permalink
C++: Do not look at nocopts in usePicForBinaries
Browse files Browse the repository at this point in the history
This required looking at the rule context and we want to stop doing that from behind the C++ Starlark API. This was only used by one target in the depot that is broken anyway. Temporarily in case this breaks anyone in Bazel, rather than using an incompatible flag, we can fix anyone broken by this using a rule feature. If no one complains, this replacement will be removed in a later release.

#4570

RELNOTES:none
PiperOrigin-RevId: 232848408
  • Loading branch information
oquenchil authored and Copybara-Service committed Feb 7, 2019
1 parent cffad55 commit 58f4db9
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,7 @@ private static boolean usePic(
if (isLinkShared(ruleContext)) {
return ccToolchainProvider.usePicForDynamicLibraries(featureConfiguration);
} else {
return CppHelper.usePicForBinaries(ruleContext, ccToolchainProvider, featureConfiguration);
return CppHelper.usePicForBinaries(ccToolchainProvider, featureConfiguration);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,10 @@ public CcCompilationHelper(
Preconditions.checkNotNull(ruleContext.getFragment(CppConfiguration.class));
setGenerateNoPicAction(
!ccToolchain.usePicForDynamicLibraries(featureConfiguration)
|| !CppHelper.usePicForBinaries(ruleContext, ccToolchain, featureConfiguration));
|| !CppHelper.usePicForBinaries(ccToolchain, featureConfiguration));
setGeneratePicAction(
ccToolchain.usePicForDynamicLibraries(featureConfiguration)
|| CppHelper.usePicForBinaries(ruleContext, ccToolchain, featureConfiguration));
|| CppHelper.usePicForBinaries(ccToolchain, featureConfiguration));
ruleErrorConsumer = ruleContext;
actionRegistry = ruleContext;
actionConstructionContext = ruleContext;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -431,8 +431,7 @@ private CcLinkingOutputs createCcLinkActions(CcCompilationOutputs ccOutputs)
"can only handle static links");

LibraryToLinkWrapper.Builder libraryToLinkBuilder = LibraryToLinkWrapper.builder();
boolean usePicForBinaries =
CppHelper.usePicForBinaries(ruleContext, ccToolchain, featureConfiguration);
boolean usePicForBinaries = CppHelper.usePicForBinaries(ccToolchain, featureConfiguration);
boolean usePicForDynamicLibs = ccToolchain.usePicForDynamicLibraries(featureConfiguration);

PathFragment labelName = PathFragment.create(label.getName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,16 +505,17 @@ public static void checkLinkstampsUnique(

/** Returns whether binaries must be compiled with position independent code. */
public static boolean usePicForBinaries(
RuleContext ruleContext,
CcToolchainProvider toolchain,
FeatureConfiguration featureConfiguration) {
CppConfiguration config = ruleContext.getFragment(CppConfiguration.class);
if (CcCommon.noCoptsMatches("-fPIC", ruleContext)) {
// TODO(b/124030770): Please do not use this feature without contacting the C++ rules team at
// [email protected]. The feature will be removed in a later Bazel release and it might
// break you. Contact us so we can find alternatives for your build.
if (featureConfiguration.getRequestedFeatures().contains("coptnopic")) {
return false;
}
return config.forcePic()
return toolchain.getCppConfiguration().forcePic()
|| (toolchain.usePicForDynamicLibraries(featureConfiguration)
&& config.getCompilationMode() != CompilationMode.OPT);
&& toolchain.getCppConfiguration().getCompilationMode() != CompilationMode.OPT);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,20 +395,27 @@ public void testNoCoptfPicOverride() throws Exception {
" srcs = [ 'library.cc' ])",
"cc_binary(name = 'nopic',",
" srcs = [ 'binary.cc' ],",
" features = ['coptnopic'],",
" nocopts = '-fPIC')",
"cc_binary(name = 'libnopic.so',",
" srcs = [ 'binary.cc' ],",
" features = ['coptnopic'],",
" nocopts = '-fPIC')",
"cc_library(name = 'nopiclib',",
" srcs = [ 'library.cc' ],",
" features = ['coptnopic'],",
" nocopts = '-fPIC')");

assertThat(getCppCompileAction("//a:pic").getArguments()).contains("-fPIC");
assertThat(getCppCompileAction("//a:libpic.so").getArguments()).contains("-fPIC");
assertThat(getCppCompileAction("//a:piclib").getArguments()).contains("-fPIC");
assertThat(getCppCompileAction("//a:piclib").getOutputFile().getFilename())
.contains("library.pic.o");
assertThat(getCppCompileAction("//a:nopic").getArguments()).doesNotContain("-fPIC");
assertThat(getCppCompileAction("//a:libnopic.so").getArguments()).doesNotContain("-fPIC");
assertThat(getCppCompileAction("//a:nopiclib").getArguments()).doesNotContain("-fPIC");
assertThat(getCppCompileAction("//a:nopiclib").getOutputFile().getFilename())
.contains("library.o");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
import com.google.devtools.build.lib.analysis.util.AnalysisMock;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
Expand Down Expand Up @@ -217,8 +216,7 @@ private boolean usePicForBinariesWithConfiguration(String... configuration) thro
/* requestedFeatures= */ ImmutableSet.of(),
/* unsupportedFeatures= */ ImmutableSet.of(),
toolchainProvider);
RuleContext ruleContext = getRuleContext(target);
return CppHelper.usePicForBinaries(ruleContext, toolchainProvider, featureConfiguration);
return CppHelper.usePicForBinaries(toolchainProvider, featureConfiguration);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,7 @@ public void testLinkstampCompileDependsOnAllCcBinaryLinkingInputs() throws Excep
/* requestedFeatures= */ ImmutableSet.of(),
/* unsupportedFeatures= */ ImmutableSet.of(),
toolchain);
boolean usePic =
CppHelper.usePicForBinaries(getRuleContext(target), toolchain, featureConfiguration);
boolean usePic = CppHelper.usePicForBinaries(toolchain, featureConfiguration);

CppLinkAction generatingAction = (CppLinkAction) getGeneratingAction(executable);

Expand Down

0 comments on commit 58f4db9

Please sign in to comment.