Do not force users to specify '.so' or '.dll' manually for linkshared=1.
This is legacy behaviour back when it was hard to do. These days, the toolchain can figure out an appropriate name for binaries and shared libraries, but only the binary code path was updated.
TESTED=Added one tiny test to make sure it is no longer an error to create such a cc_binary. I don't know if it's safe to assert on exact name, seeing as the tests would then be platform specific.
RELNOTES: linkshared=1 in cc_binary no longer requires '.so' or '.dll' in the target name
PiperOrigin-RevId: 313398118
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 601270e..1e81a9d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -332,24 +332,23 @@
// if cc_binary includes "linkshared=1", then gcc will be invoked with
// linkopt "-shared", which causes the result of linking to be a shared
- // library. In this case, the name of the executable target should end
- // in ".so" or "dylib" or ".dll".
- Artifact binary;
- PathFragment binaryPath = PathFragment.create(ruleContext.getTarget().getName());
- if (!isLinkShared(ruleContext)) {
+ // library.
+ final Artifact binary;
+
+ // For linkshared=1 we used to force users to specify the file extension manually, as part of
+ // the target name.
+ // This is no longer necessary, the toolchain can figure out the correct file extension.
+ String targetName = ruleContext.getTarget().getName();
+ boolean hasLegacyLinkSharedName =
+ isLinkShared(ruleContext)
+ && (CppFileTypes.SHARED_LIBRARY.matches(targetName)
+ || CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(targetName));
+ if (hasLegacyLinkSharedName) {
+ binary = ruleContext.getBinArtifact(PathFragment.create(targetName));
+ } else {
binary =
CppHelper.getLinkedArtifact(
- ruleContext, ccToolchain, ruleContext.getConfiguration(), LinkTargetType.EXECUTABLE);
- } else {
- binary = ruleContext.getBinArtifact(binaryPath);
- }
-
- if (isLinkShared(ruleContext)
- && !CppFileTypes.SHARED_LIBRARY.matches(binary.getFilename())
- && !CppFileTypes.VERSIONED_SHARED_LIBRARY.matches(binary.getFilename())) {
- ruleContext.attributeError("linkshared", "'linkshared' used in non-shared library");
- fillInRequiredProviders(ruleBuilder, ruleContext);
- return;
+ ruleContext, ccToolchain, ruleContext.getConfiguration(), linkType);
}
LinkingMode linkingMode = getLinkStaticness(ruleContext, cppConfiguration);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index cc0f020..0e29f01 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -742,8 +742,12 @@
@Test
public void testAnalysisEntryHasActionsEvenWithError() throws Exception {
- scratch.file("foo/BUILD",
- "cc_binary(name = 'foo', linkshared = 1, srcs = ['foo.cc'])");
+ scratch.file(
+ "foo/BUILD",
+ "genquery(name = 'foo',",
+ " expression = 'deps(//foo:nosuchtarget)',",
+ " scope = ['//foo:a'])",
+ "sh_library(name = 'a')");
reporter.removeHandler(failFastHandler);
assertThrows(ViewCreationFailedException.class, () -> update("//foo:foo"));
}
@@ -918,22 +922,40 @@
*/
@Test
public void testActionsNotRegisteredInLegacyWhenError() throws Exception {
+
// First find the artifact we want to make sure is not generated by an action with an error.
// Then update the BUILD file and re-analyze.
- scratch.file("actions_not_registered/BUILD",
- "cc_binary(name = 'foo', srcs = ['foo.cc'])");
- ConfiguredTarget foo =
- Iterables.getOnlyElement(update("//actions_not_registered:foo").getTargetsToBuild());
+ scratch.file(
+ "foo/failer.bzl",
+ "def _impl(ctx):",
+ " if ctx.attr.fail:",
+ " fail('failing')",
+ " ctx.actions.run_shell(outputs=[ctx.outputs.out], command='null')",
+ "failer = rule(",
+ " _impl,",
+ " attrs = {",
+ " 'fail': attr.bool(),",
+ " 'out': attr.output(),",
+ " },",
+ ")");
+ scratch.overwriteFile(
+ "foo/BUILD",
+ "load(':failer.bzl', 'failer')",
+ "failer(name = 'foo', fail = False, out = 'foo.txt')");
+ ConfiguredTarget foo = Iterables.getOnlyElement(update("//foo:foo").getTargetsToBuild());
Artifact fooOut = foo.getProvider(FileProvider.class).getFilesToBuild().getSingleton();
assertThat(getActionGraph().getGeneratingAction(fooOut)).isNotNull();
clearAnalysisResult();
- scratch.overwriteFile("actions_not_registered/BUILD",
- "cc_binary(name = 'foo', linkshared = 1, srcs = ['foo.cc'])");
+ // Overwrite with an analysis-time error.
+ scratch.overwriteFile(
+ "foo/BUILD",
+ "load(':failer.bzl', 'failer')",
+ "failer(name = 'foo', fail = True, out = 'foo.txt')");
reporter.removeHandler(failFastHandler);
- assertThrows(ViewCreationFailedException.class, () -> update("//actions_not_registered:foo"));
+ assertThrows(ViewCreationFailedException.class, () -> update("//foo:foo"));
assertThat(getActionGraph().getGeneratingAction(fooOut)).isNull();
}
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
index 41d6f0b..dd9911c 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
@@ -377,7 +377,7 @@
],
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
- "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception",
+ "//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/test/java/com/google/devtools/build/lib/buildtool/util",
"//third_party:junit4",
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ModifyBuildFileTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ModifyBuildFileTest.java
index 00e3205..d97d31b 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/ModifyBuildFileTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/ModifyBuildFileTest.java
@@ -17,8 +17,8 @@
import static org.junit.Assert.assertThrows;
import com.google.devtools.build.lib.actions.BuildFailedException;
-import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
import com.google.devtools.build.lib.buildtool.util.GoogleBuildIntegrationTestCase;
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.vfs.Path;
import java.io.IOException;
import org.junit.Test;
@@ -39,9 +39,11 @@
private void updateBuildFileAndSetMtime(long mtime)
throws IOException {
- // put an error in the BUILD file (linkshared = 1 requires name to end with ".so").
- Path buildFile = write("modify_build_file_test/BUILD",
- "cc_binary(name = 'foo', linkshared = 1, srcs = ['foo.cc'])");
+ // put an error in the BUILD file.
+ Path buildFile =
+ write(
+ "modify_build_file_test/BUILD",
+ "cc_binary(name = 'foo', doesnotexist = 1, srcs = ['foo.cc'])");
buildFile.setLastModifiedTime(mtime);
// other files remain unchanged
}
@@ -74,8 +76,7 @@
// this is supposed to fail.
//
updateBuildFileAndSetMtime(2000);
- assertThrows(
- ViewCreationFailedException.class, () -> buildTarget("//modify_build_file_test:foo"));
+ assertThrows(TargetParsingException.class, () -> buildTarget("//modify_build_file_test:foo"));
//
// Restore the original contents BUILD file and rebuild;
