Instead of filtering for dynamic libraries when creating solib symlinks, assert that solib symlinks always point to dynamic libraries.

Note that this makes it possible to crash Bazel with a malicious CROSSTOOL file, but since we are migrating to cc_toolchain_suite which raises a user-friendly error earlier, this is okay.

--
MOS_MIGRATED_REVID=130915262

3 files changed, 53 insertions, 55 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
index b09ff13cc4..ee2b08d68a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -20,7 +20,6 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MiddlemanFactory;
-import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
@@ -460,73 +459,47 @@ public class CppHelper {
   static List<Artifact> getAggregatingMiddlemanForCppRuntimes(RuleContext ruleContext,
       String purpose, TransitiveInfoCollection dep, String solibDirOverride,
       BuildConfiguration configuration) {
-    return getMiddlemanInternal(
-        ruleContext.getAnalysisEnvironment(), ruleContext, ruleContext.getActionOwner(), purpose,
+    return getMiddlemanInternal(ruleContext, ruleContext.getActionOwner(), purpose,
         dep, true, true, solibDirOverride, configuration);
   }
 
   @VisibleForTesting
-  public static List<Artifact> getAggregatingMiddlemanForTesting(AnalysisEnvironment env,
+  public static List<Artifact> getAggregatingMiddlemanForTesting(
       RuleContext ruleContext, ActionOwner owner, String purpose, TransitiveInfoCollection dep,
       boolean useSolibSymlinks, BuildConfiguration configuration) {
     return getMiddlemanInternal(
-        env, ruleContext, owner, purpose, dep, useSolibSymlinks, false, null, configuration);
+        ruleContext, owner, purpose, dep, useSolibSymlinks, false, null, configuration);
   }
 
   /**
    * Internal implementation for getAggregatingMiddlemanForCppRuntimes.
    */
-  private static List<Artifact> getMiddlemanInternal(AnalysisEnvironment env,
+  private static List<Artifact> getMiddlemanInternal(
       RuleContext ruleContext, ActionOwner actionOwner, String purpose,
       TransitiveInfoCollection dep, boolean useSolibSymlinks, boolean isCppRuntime,
       String solibDirOverride, BuildConfiguration configuration) {
     if (dep == null) {
       return ImmutableList.of();
     }
-    MiddlemanFactory factory = env.getMiddlemanFactory();
+    MiddlemanFactory factory = ruleContext.getAnalysisEnvironment().getMiddlemanFactory();
     Iterable<Artifact> artifacts = dep.getProvider(FileProvider.class).getFilesToBuild();
     if (useSolibSymlinks) {
       List<Artifact> symlinkedArtifacts = new ArrayList<>();
       for (Artifact artifact : artifacts) {
-        symlinkedArtifacts.add(solibArtifactMaybe(
-            ruleContext, artifact, isCppRuntime, solibDirOverride, configuration));
+        Preconditions.checkState(Link.SHARED_LIBRARY_FILETYPES.matches(artifact.getFilename()));
+        symlinkedArtifacts.add(isCppRuntime
+            ? SolibSymlinkAction.getCppRuntimeSymlink(
+                ruleContext, artifact, solibDirOverride, configuration)
+            : SolibSymlinkAction.getDynamicLibrarySymlink(
+                ruleContext, artifact, false, true, configuration));
       }
       artifacts = symlinkedArtifacts;
       purpose += "_with_solib";
     }
     return ImmutableList.of(
-        factory.createMiddlemanAllowMultiple(env, actionOwner, ruleContext.getPackageDirectory(),
-            purpose, artifacts, configuration.getMiddlemanDirectory(
-                ruleContext.getRule().getRepository())));
-  }
-
-  /**
-   * If the artifact is a shared library, returns the solib symlink artifact associated with it.
-   *
-   * @param ruleContext the context of the rule that creates the symlink
-   * @param artifact the library the solib symlink should point to
-   * @param isCppRuntime whether the library is a C++ runtime
-   * @param solibDirOverride if not null, forces the solib symlink to be in this directory
-   */
-  private static Artifact solibArtifactMaybe(RuleContext ruleContext, Artifact artifact,
-      boolean isCppRuntime, String solibDirOverride, BuildConfiguration configuration) {
-    if (SHARED_LIBRARY_FILETYPES.matches(artifact.getFilename())) {
-      return isCppRuntime
-        ? SolibSymlinkAction.getCppRuntimeSymlink(
-            ruleContext, artifact, solibDirOverride, configuration)
-        : SolibSymlinkAction.getDynamicLibrarySymlink(
-            ruleContext, artifact, false, true, configuration);
-    } else {
-      return artifact;
-    }
-  }
-
-  /**
-   * Returns the type of archives being used.
-   */
-  public static Link.ArchiveType archiveType(BuildConfiguration config) {
-    CppConfiguration cppConfig = config.getFragment(CppConfiguration.class);
-    return cppConfig.archiveType();
+        factory.createMiddlemanAllowMultiple(ruleContext.getAnalysisEnvironment(), actionOwner,
+            ruleContext.getPackageDirectory(), purpose, artifacts,
+            configuration.getMiddlemanDirectory(ruleContext.getRule().getRepository())));
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/CompilationHelperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/CompilationHelperTest.java
index c0d612f836..3a452bef07 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/CompilationHelperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/CompilationHelperTest.java
@@ -53,8 +53,7 @@ public class CompilationHelperTest extends BuildViewTestCase {
   private List<Artifact> getAggregatingMiddleman(
       ConfiguredTarget rule, BuildConfiguration configuration, boolean withSolib) throws Exception {
     return CppHelper.getAggregatingMiddlemanForTesting(
-        analysisEnvironment,
-        getRuleContext(rule),
+        getRuleContext(rule, analysisEnvironment),
         ActionsTestUtil.NULL_ACTION_OWNER,
         "middleman",
         rule,
@@ -91,8 +90,8 @@ public class CompilationHelperTest extends BuildViewTestCase {
    */
   @Test
   public void testMiddlemanAndSolibMiddlemanAreDistinct() throws Exception {
-    ConfiguredTarget rule =
-        scratchConfiguredTarget("package", "a", "cc_binary(name = 'a'," + "    srcs = ['a.cc'])");
+    ConfiguredTarget rule = scratchConfiguredTarget("package", "liba.so",
+        "cc_binary(name = 'liba.so', srcs = ['a.cc'], linkshared = 1)");
 
     List<Artifact> middleman = getAggregatingMiddleman(rule, false);
     assertThat(middleman).hasSize(1);
@@ -112,10 +111,10 @@ public class CompilationHelperTest extends BuildViewTestCase {
 
     // Equivalent cc / Python configurations:
 
-    ConfiguredTarget ccRuleA = getConfiguredTarget("//foo:a");
+    ConfiguredTarget ccRuleA = getConfiguredTarget("//foo:liba.so");
     List<Artifact> middleman1 = getAggregatingMiddleman(ccRuleA, true);
     try {
-      ConfiguredTarget ccRuleB = getConfiguredTarget("//foo:b");
+      ConfiguredTarget ccRuleB = getConfiguredTarget("//foo:libb.so");
       getAggregatingMiddleman(ccRuleB, true);
       analysisEnvironment.registerWith(getMutableActionGraph());
       fail("Expected ActionConflictException due to same middleman artifact with different files");
@@ -127,8 +126,11 @@ public class CompilationHelperTest extends BuildViewTestCase {
     // This should succeed because the py_binary's middleman is under the Python configuration's
     // internal directory, while the cc_binary's middleman is under the cc config's directory,
     // and both configurations are the same.
-    ConfiguredTarget pyRuleC = getConfiguredTarget("//foo:c");
-    List<Artifact> middleman2 = getAggregatingMiddleman(pyRuleC, true);
+    ConfiguredTarget pyRuleB = getDirectPrerequisite(
+        getConfiguredTarget("//foo:c"), "//foo:libb.so");
+
+
+    List<Artifact> middleman2 = getAggregatingMiddleman(pyRuleB, true);
     assertEquals(
         Iterables.getOnlyElement(middleman1).getExecPathString(),
         Iterables.getOnlyElement(middleman2).getExecPathString());
@@ -145,10 +147,10 @@ public class CompilationHelperTest extends BuildViewTestCase {
 
     // Equivalent cc / Java configurations:
 
-    ConfiguredTarget ccRuleA = getConfiguredTarget("//foo:a");
+    ConfiguredTarget ccRuleA = getConfiguredTarget("//foo:liba.so");
     List<Artifact> middleman1 = getAggregatingMiddleman(ccRuleA, true);
     try {
-      ConfiguredTarget ccRuleB = getConfiguredTarget("//foo:b");
+      ConfiguredTarget ccRuleB = getConfiguredTarget("//foo:libb.so");
       getAggregatingMiddleman(ccRuleB, true);
       analysisEnvironment.registerWith(getMutableActionGraph());
       fail("Expected ActionConflictException due to same middleman artifact with different files");
@@ -159,8 +161,9 @@ public class CompilationHelperTest extends BuildViewTestCase {
 
     // This should succeed because the java_binary's middleman is under the Java configuration's
     // internal directory, while the cc_binary's middleman is under the cc config's directory.
-    ConfiguredTarget javaRuleD = getConfiguredTarget("//foo:d");
-    List<Artifact> middleman2 = getAggregatingMiddleman(javaRuleD, false);
+    ConfiguredTarget javaRuleB = getDirectPrerequisite(
+        getConfiguredTarget("//foo:d"), "//foo:libb.so");
+    List<Artifact> middleman2 = getAggregatingMiddleman(javaRuleB, false);
     assertFalse(
         Iterables.getOnlyElement(middleman1)
             .getExecPathString()
@@ -170,14 +173,18 @@ public class CompilationHelperTest extends BuildViewTestCase {
   private void setupJavaPythonCcConfigurationFiles() throws IOException {
     scratch.file(
         "foo/BUILD",
-        "cc_binary(name = 'a',",
+        "cc_binary(name = 'liba.so',",
+        "    linkshared = 1,",
         "    srcs = ['a.cc'])",
-        "cc_binary(name = 'b',",
+        "cc_binary(name = 'libb.so',",
+        "    linkshared = 1,",
         "    srcs = ['b.cc'])",
         "py_binary(name = 'c',",
+        "    data = [':libb.so'],",
         "    srcs = ['c.py'])",
         "java_binary(name = 'd',",
         "    srcs = ['d.java'],",
+        "    data = [':libb.so'],",
         "    main_class = 'd')");
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index ad82d32dfe..f5fda5c2f1 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -445,6 +445,18 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     return view.getDirectPrerequisitesForTesting(reporter, target, masterConfig);
   }
 
+  protected ConfiguredTarget getDirectPrerequisite(ConfiguredTarget target, String label)
+      throws Exception {
+    Label candidateLabel = Label.parseAbsolute(label);
+    for (ConfiguredTarget candidate : getDirectPrerequisites(target)) {
+      if (candidate.getLabel().equals(candidateLabel)) {
+        return candidate;
+      }
+    }
+
+    return null;
+  }
+
   /**
    * Asserts that a target's prerequisites contain the given dependency.
    */
@@ -496,6 +508,12 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
         reporter, target, new StubAnalysisEnvironment(), masterConfig);
   }
 
+  protected RuleContext getRuleContext(ConfiguredTarget target,
+      AnalysisEnvironment analysisEnvironment) throws Exception {
+    return view.getRuleContextForTesting(
+        reporter, target, analysisEnvironment, masterConfig);
+  }
+
   /**
    * Creates and returns a rule context to use for Skylark tests that is equivalent to the one
    * that was used to create the given configured target.