Create runfiles at both .runfiles/ws/external/repo and .runfiles/repo

The major piece of #848.

RELNOTES[INC]: All repositories are now directly under the x.runfiles directory in the runfiles tree (previously, external repositories were at x.runfiles/main-repo/external/other-repo. This simplifies handling remote repository runfiles considerably, but will break existing references to external repository runfiles.

--
MOS_MIGRATED_REVID=120722312

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

@@ -20,6 +20,7 @@
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -458,23 +459,99 @@ public Map<PathFragment, Artifact> getRunfilesInputs(EventHandler eventHandler,
     // Copy manifest map to another manifest map, prepending the workspace name to every path.
     // E.g. for workspace "myworkspace", the runfile entry "mylib.so"->"/path/to/mylib.so" becomes
     // "myworkspace/mylib.so"->"/path/to/mylib.so".
-    Map<PathFragment, Artifact> rootManifest = new HashMap<>();
-    for (Map.Entry<PathFragment, Artifact> entry : manifest.entrySet()) {
-      checker.put(rootManifest, suffix.getRelative(entry.getKey()), entry.getValue());
-    }
+    ManifestBuilder builder = new ManifestBuilder(suffix, legacyExternalRunfiles);
+    builder.addUnderWorkspace(manifest, checker);
 
     // Finally add symlinks relative to the root of the runfiles tree, on top of everything else.
     // This operation is always checked for conflicts, to match historical behavior.
     if (conflictPolicy == ConflictPolicy.IGNORE) {
       checker = new ConflictChecker(ConflictPolicy.WARN, eventHandler, location);
     }
-    for (Map.Entry<PathFragment, Artifact> entry : getRootSymlinksAsMap(checker).entrySet()) {
-      PathFragment mappedPath = entry.getKey();
-      Artifact mappedArtifact = entry.getValue();
-      checker.put(rootManifest, mappedPath, mappedArtifact);
+    builder.add(getRootSymlinksAsMap(checker), checker);
+    return builder.build();
+  }
+
+  /**
+   * Helper class to handle munging the paths of external artifacts.
+   */
+  @VisibleForTesting
+  static final class ManifestBuilder {
+    // Manifest of paths to artifacts. Path fragments are relative to the .runfiles directory.
+    private final Map<PathFragment, Artifact> manifest;
+    private final PathFragment workspaceName;
+    private final boolean legacyExternalRunfiles;
+    // Whether we saw the local workspace name in the runfiles. If legacyExternalRunfiles is true,
+    // then this is true, as anything under external/ will also have a runfile under the local
+    // workspace.
+    private boolean sawWorkspaceName;
+
+    public ManifestBuilder(
+        PathFragment workspaceName, boolean legacyExternalRunfiles) {
+      this.manifest = new HashMap<>();
+      this.workspaceName = workspaceName;
+      this.legacyExternalRunfiles = legacyExternalRunfiles;
+      this.sawWorkspaceName = legacyExternalRunfiles;
     }
 
-    return rootManifest;
+    /**
+     * Adds a map under the workspaceName.
+     */
+    public void addUnderWorkspace(
+        Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) {
+      for (Map.Entry<PathFragment, Artifact> entry : inputManifest.entrySet()) {
+        PathFragment path = entry.getKey();
+        if (isUnderWorkspace(path)) {
+          sawWorkspaceName = true;
+          checker.put(manifest, workspaceName.getRelative(path), entry.getValue());
+        } else {
+          if (legacyExternalRunfiles) {
+            checker.put(manifest, workspaceName.getRelative(path), entry.getValue());
+          }
+          // Always add the non-legacy .runfiles/repo/whatever path.
+          checker.put(manifest, getExternalPath(path), entry.getValue());
+        }
+      }
+    }
+
+    /**
+     * Adds a map to the root directory.
+     */
+    public void add(Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) {
+      for (Map.Entry<PathFragment, Artifact> entry : inputManifest.entrySet()) {
+        checker.put(manifest, checkForWorkspace(entry.getKey()), entry.getValue());
+      }
+    }
+
+    /**
+     * Returns the manifest, adding the workspaceName directory if it is not already present.
+     */
+    public Map<PathFragment, Artifact> build() {
+      if (!sawWorkspaceName) {
+        // If we haven't seen it and we have seen other files, add the workspace name directory.
+        // It might not be there if all of the runfiles are from other repos (and then running from
+        // x.runfiles/ws will fail, because ws won't exist). We can't tell Runfiles to create a
+        // directory, so instead this creates a hidden file inside the desired directory.
+        manifest.put(workspaceName.getRelative(".runfile"), null);
+      }
+      return manifest;
+    }
+
+    private PathFragment getExternalPath(PathFragment path) {
+      return checkForWorkspace(path.relativeTo(Label.EXTERNAL_PACKAGE_NAME));
+    }
+
+    private PathFragment checkForWorkspace(PathFragment path) {
+      sawWorkspaceName = sawWorkspaceName || path.getSegment(0).equals(workspaceName);
+      return path;
+    }
+
+    private static boolean isUnderWorkspace(PathFragment path) {
+      return !path.startsWith(Label.EXTERNAL_PACKAGE_NAME);
+    }
+  }
+
+  boolean getLegacyExternalRunfiles() {
+    return legacyExternalRunfiles;
   }
 
   /**

src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java

@@ -192,6 +192,7 @@ protected String getRawProgressMessage() {
   protected String computeKey() {
     Fingerprint f = new Fingerprint();
     f.addString(GUID);
+    f.addBoolean(runfiles.getLegacyExternalRunfiles());
     Map<PathFragment, Artifact> symlinks = runfiles.getSymlinksAsMap(null);
     f.addInt(symlinks.size());
     for (Map.Entry<PathFragment, Artifact> symlink : symlinks.entrySet()) {

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java

@@ -710,7 +710,7 @@ public String getCpu() {
 
     @Option(name = "legacy_external_runfiles",
         defaultValue = "true",
-        category = "undocumented",
+        category = "strategy",
         help = "If true, build runfiles symlink forests for external repositories under "
             + ".runfiles/wsname/external/repo (in addition to .runfiles/repo).")
     public boolean legacyExternalRunfiles;

src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java

@@ -16,10 +16,12 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -41,7 +43,7 @@ public class RunfilesTest extends FoundationTestCase {
   private void checkWarning() {
     assertContainsEvent("obscured by a -> /workspace/a");
     assertEquals("Runfiles.filterListForObscuringSymlinks should have warned once",
-                 1, eventCollector.count());
+        1, eventCollector.count());
     assertEquals(EventKind.WARNING, Iterables.getOnlyElement(eventCollector).getKind());
   }
 
@@ -83,7 +85,7 @@ public void testFilterListForObscuringSymlinksCatchesBadObscurerNoListener() thr
                                           root);
     obscuringMap.put(pathA, artifactA);
     obscuringMap.put(new PathFragment("a/b"), new Artifact(new PathFragment("c/b"),
-                                                         root));
+        root));
     assertThat(Runfiles.filterListForObscuringSymlinks(null, null, obscuringMap).entrySet())
         .containsExactly(Maps.immutableEntry(pathA, artifactA)).inOrder();
   }
@@ -117,7 +119,7 @@ public void testFilterListForObscuringSymlinksNoObscurers() throws Exception {
                                        root);
     obscuringMap.put(pathBC, artifactBC);
     assertThat(Runfiles.filterListForObscuringSymlinks(reporter, null, obscuringMap)
-                          .entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactA),
+        .entrySet()).containsExactly(Maps.immutableEntry(pathA, artifactA),
         Maps.immutableEntry(pathBC, artifactBC));
     assertNoEvents();
   }
@@ -316,4 +318,72 @@ public void testBuilderMergeConflictPolicyInheritStrictest() {
     Runfiles r4 = new Runfiles.Builder("TESTING").merge(r2).merge(r1).build();
     assertEquals(Runfiles.ConflictPolicy.ERROR, r4.getConflictPolicy());
   }
+
+  @Test
+  public void testLegacyRunfilesStructure() {
+    Root root = Root.asSourceRoot(scratch.resolve("/workspace"));
+    PathFragment workspaceName = new PathFragment("wsname");
+    PathFragment pathB = new PathFragment("external/repo/b");
+    Artifact artifactB = new Artifact(pathB, root);
+
+    Runfiles.ManifestBuilder builder = new Runfiles.ManifestBuilder(workspaceName, true);
+
+    Map<PathFragment, Artifact> inputManifest = Maps.newHashMap();
+    inputManifest.put(pathB, artifactB);
+    Runfiles.ConflictChecker checker = new Runfiles.ConflictChecker(
+        Runfiles.ConflictPolicy.WARN, reporter, null);
+    builder.addUnderWorkspace(inputManifest, checker);
+
+    assertThat(builder.build().entrySet()).containsExactly(
+        Maps.immutableEntry(workspaceName.getRelative(pathB), artifactB),
+        Maps.immutableEntry(new PathFragment("repo/b"), artifactB));
+    assertNoEvents();
+  }
+
+  @Test
+  public void testRunfileAdded() {
+    Root root = Root.asSourceRoot(scratch.resolve("/workspace"));
+    PathFragment workspaceName = new PathFragment("wsname");
+    PathFragment pathB = new PathFragment("external/repo/b");
+    Artifact artifactB = new Artifact(pathB, root);
+
+    Runfiles.ManifestBuilder builder = new Runfiles.ManifestBuilder(workspaceName, false);
+
+    Map<PathFragment, Artifact> inputManifest = ImmutableMap.<PathFragment, Artifact>builder()
+        .put(pathB, artifactB)
+        .build();
+    Runfiles.ConflictChecker checker = new Runfiles.ConflictChecker(
+        Runfiles.ConflictPolicy.WARN, reporter, null);
+    builder.addUnderWorkspace(inputManifest, checker);
+
+    assertThat(builder.build().entrySet()).containsExactly(
+        Maps.immutableEntry(workspaceName.getRelative(".runfile"), null),
+        Maps.immutableEntry(new PathFragment("repo/b"), artifactB));
+    assertNoEvents();
+  }
+
+  // TODO(kchodorow): remove this once the default workspace name is always set.
+  @Test
+  public void testConflictWithExternal() {
+    Root root = Root.asSourceRoot(scratch.resolve("/workspace"));
+    PathFragment pathB = new PathFragment("repo/b");
+    PathFragment externalPathB = Label.EXTERNAL_PACKAGE_NAME.getRelative(pathB);
+    Artifact artifactB = new Artifact(pathB, root);
+    Artifact artifactExternalB = new Artifact(externalPathB, root);
+
+    Runfiles.ManifestBuilder builder = new Runfiles.ManifestBuilder(
+        PathFragment.EMPTY_FRAGMENT, false);
+
+    Map<PathFragment, Artifact> inputManifest = ImmutableMap.<PathFragment, Artifact>builder()
+        .put(pathB, artifactB)
+        .put(externalPathB, artifactExternalB)
+        .build();
+    Runfiles.ConflictChecker checker = new Runfiles.ConflictChecker(
+        Runfiles.ConflictPolicy.WARN, reporter, null);
+    builder.addUnderWorkspace(inputManifest, checker);
+
+    assertThat(builder.build().entrySet()).containsExactly(
+        Maps.immutableEntry(new PathFragment("repo/b"), artifactExternalB));
+    checkConflictWarning();
+  }
 }