Change execution root for external repositories to be ../repo

Some of the important aspect of this change:

* Remote repos in the execution root are under output_base/execroot/repo_name, so the prefix is ../repo_name (to escape the local workspace name).
* Package roots for external repos were previously "output_base/", they are now output_base/external/repo_name (which means source artifacts always have a relative path from their repository).
* Outputs are under bazel-bin/external/repo_name/ (or similarly under genfiles). Note that this is a bit of a change from how this was implemented in the previous cl.

Fixes #1262.

RELNOTES[INC]: Previously, an external repository would be symlinked into the
execution root at execroot/local_repo/external/remote_repo. This changes it to
be at execroot/remote_repo. This may break genrules/Skylark actions that
hardcode execution root paths. If this causes breakages for you, ensure that
genrules are using $(location :target) to access files and Skylark rules are
using http://bazel.io/docs/skylark/lib/File.html's path, dirname, etc.
functions.

Roll forward of bdfd58a.

--
MOS_MIGRATED_REVID=133606309

9 files changed, 83 insertions, 74 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
index 1f83f28005..734f7d4f09 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
@@ -21,18 +21,15 @@ 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;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Test for {@link Runfiles}.
@@ -321,37 +318,36 @@ public class RunfilesTest extends FoundationTestCase {
 
   @Test
   public void testLegacyRunfilesStructure() {
-    Root root = Root.asSourceRoot(scratch.resolve("/workspace"));
+    Root root = Root.asSourceRoot(scratch.resolve("/workspace/external/repo"));
     PathFragment workspaceName = new PathFragment("wsname");
-    PathFragment pathB = new PathFragment("external/repo/b");
+    PathFragment pathB = new PathFragment("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);
+    inputManifest.put(new PathFragment("../repo").getRelative(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(workspaceName.getRelative("external/repo/b"), artifactB),
         Maps.immutableEntry(new PathFragment("repo/b"), artifactB));
     assertNoEvents();
   }
 
   @Test
   public void testRunfileAdded() {
-    Root root = Root.asSourceRoot(scratch.resolve("/workspace"));
+    Root root = Root.asSourceRoot(scratch.resolve("/workspace/external/repo"));
     PathFragment workspaceName = new PathFragment("wsname");
-    PathFragment pathB = new PathFragment("external/repo/b");
+    PathFragment pathB = new PathFragment("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();
+    Map<PathFragment, Artifact> inputManifest = ImmutableMap.of(
+        new PathFragment("../repo").getRelative(pathB), artifactB);
     Runfiles.ConflictChecker checker = new Runfiles.ConflictChecker(
         Runfiles.ConflictPolicy.WARN, reporter, null);
     builder.addUnderWorkspace(inputManifest, checker);
@@ -361,29 +357,4 @@ public class RunfilesTest extends FoundationTestCase {
         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();
-  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 2ea8887392..f9daedb462 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -38,6 +38,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigurationFactory;
 import com.google.devtools.build.lib.buildtool.BuildRequest.BuildRequestOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
 import com.google.devtools.build.lib.packages.PackageFactory;
@@ -189,6 +190,8 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
     loadingPhaseRunner = skyframeExecutor.getLoadingPhaseRunner(
         pkgFactory.getRuleClassNames(), defaultFlags().contains(Flag.SKYFRAME_LOADING_PHASE));
     buildView = new BuildView(directories, ruleClassProvider, skyframeExecutor, null);
+    buildView.setArtifactRoots(
+        ImmutableMap.of(PackageIdentifier.createInMainRepo(""), rootDirectory));
     useConfiguration();
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/SymlinkForestTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/SymlinkForestTest.java
index d86d2be261..5e0cda6d61 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/SymlinkForestTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/SymlinkForestTest.java
@@ -27,7 +27,6 @@ import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.testutil.ManualClock;
-import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -125,7 +124,12 @@ public class SymlinkForestTest {
   @Test
   public void testDeleteTreesBelowNotPrefixed() throws IOException {
     createTestDirectoryTree();
-    SymlinkForest.deleteTreesBelowNotPrefixed(topDir, new String[]{"file-"});
+    SymlinkForest forest = SymlinkForest.builder()
+        .setWorkspace(topDir)
+        .setProductName("mock-name")
+        .setPrefixes(new String[]{"file-"})
+        .build();
+    forest.deleteTreesBelowNotPrefixed();
     assertTrue(file1.exists());
     assertTrue(file2.exists());
     assertFalse(aDir.exists());
@@ -187,7 +191,13 @@ public class SymlinkForestTest {
 
     Path linkRoot = fileSystem.getPath("/linkRoot");
     createDirectoryAndParents(linkRoot);
-    new SymlinkForest(packageRootMap, linkRoot, TestConstants.PRODUCT_NAME, "wsname")
+    SymlinkForest.builder()
+        .setLegacyExternalRunfiles(false)
+        .setPackageRoots(packageRootMap)
+        .setWorkspace(linkRoot)
+        .setProductName("mock-name")
+        .setWorkspaceName("wsname")
+        .build()
         .plantSymlinkForest();
 
     assertLinksTo(linkRoot, rootA, "pkgA");
@@ -215,7 +225,13 @@ public class SymlinkForestTest {
             .put(createPkg(rootX, rootY, "foo"), rootX)
             .build();
 
-    new SymlinkForest(packageRootMap, linkRoot, TestConstants.PRODUCT_NAME, "wsname")
+    SymlinkForest.builder()
+        .setLegacyExternalRunfiles(false)
+        .setPackageRoots(packageRootMap)
+        .setWorkspace(linkRoot)
+        .setProductName("mock-name")
+        .setWorkspaceName("wsname")
+        .build()
         .plantSymlinkForest();
     assertLinksTo(linkRoot, rootX, "file");
   }
@@ -223,24 +239,32 @@ public class SymlinkForestTest {
   @Test
   public void testRemotePackage() throws Exception {
     Path outputBase = fileSystem.getPath("/ob");
-    Path rootY = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX).getRelative("y");
-    Path rootZ = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX).getRelative("z");
-    Path rootW = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX).getRelative("w");
+    Path rootY = outputBase.getRelative(Label.EXTERNAL_PACKAGE_NAME).getRelative("y");
+    Path rootZ = outputBase.getRelative(Label.EXTERNAL_PACKAGE_NAME).getRelative("z");
+    Path rootW = outputBase.getRelative(Label.EXTERNAL_PACKAGE_NAME).getRelative("w");
     createDirectoryAndParents(rootY);
+    createDirectoryAndParents(rootZ);
+    createDirectoryAndParents(rootW);
     FileSystemUtils.createEmptyFile(rootY.getRelative("file"));
 
     ImmutableMap<PackageIdentifier, Path> packageRootMap =
         ImmutableMap.<PackageIdentifier, Path>builder()
             // Remote repo without top-level package.
-            .put(createPkg(outputBase, "y", "w"), outputBase)
+            .put(createPkg(outputBase, "y", "w"), rootY)
             // Remote repo with and without top-level package.
-            .put(createPkg(outputBase, "z", ""), outputBase)
-            .put(createPkg(outputBase, "z", "a/b/c"), outputBase)
+            .put(createPkg(outputBase, "z", ""), rootZ)
+            .put(createPkg(outputBase, "z", "a/b/c"), rootZ)
             // Only top-level pkg.
-            .put(createPkg(outputBase, "w", ""), outputBase)
+            .put(createPkg(outputBase, "w", ""), rootW)
             .build();
 
-    new SymlinkForest(packageRootMap, linkRoot, TestConstants.PRODUCT_NAME, "wsname")
+    SymlinkForest.builder()
+        .setLegacyExternalRunfiles(false)
+        .setPackageRoots(packageRootMap)
+        .setWorkspace(linkRoot)
+        .setProductName("mock-name")
+        .setWorkspaceName("wsname")
+        .build()
         .plantSymlinkForest();
     assertFalse(linkRoot.getRelative(Label.EXTERNAL_PATH_PREFIX + "/y/file").exists());
     assertLinksTo(
@@ -263,9 +287,15 @@ public class SymlinkForestTest {
             .put(Label.EXTERNAL_PACKAGE_IDENTIFIER, root)
             .build();
 
-    new SymlinkForest(packageRootMap, linkRoot, TestConstants.PRODUCT_NAME, "wsname")
+    SymlinkForest.builder()
+        .setLegacyExternalRunfiles(false)
+        .setPackageRoots(packageRootMap)
+        .setWorkspace(linkRoot)
+        .setProductName("mock-name")
+        .setWorkspaceName("wsname")
+        .build()
         .plantSymlinkForest();
-    assertThat(linkRoot.getRelative(Label.EXTERNAL_PATH_PREFIX).exists()).isFalse();
+    assertThat(linkRoot.getRelative(Label.EXTERNAL_PACKAGE_NAME).exists()).isFalse();
   }
 
   @Test
@@ -277,7 +307,13 @@ public class SymlinkForestTest {
             .put(createPkg(root, "y", "w"), root)
             .build();
 
-    new SymlinkForest(packageRootMap, linkRoot, TestConstants.PRODUCT_NAME, "wsname")
+    SymlinkForest.builder()
+        .setLegacyExternalRunfiles(true)
+        .setPackageRoots(packageRootMap)
+        .setWorkspace(linkRoot)
+        .setProductName("mock-name")
+        .setWorkspaceName("wsname")
+        .build()
         .plantSymlinkForest();
     assertThat(linkRoot.getRelative("../wsname").exists()).isTrue();
   }
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
index 3f5a0e0820..42642b8851 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
@@ -19,15 +19,13 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertSame;
 
 import com.google.devtools.build.lib.vfs.PathFragment;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Unit tests for {@link PackageIdentifier}.
@@ -39,8 +37,7 @@ public class PackageIdentifierTest {
     PackageIdentifier fooA = PackageIdentifier.parse("@foo//a");
     assertThat(fooA.getRepository().strippedName()).isEqualTo("foo");
     assertThat(fooA.getPackageFragment().getPathString()).isEqualTo("a");
-    assertThat(fooA.getRepository().getSourceRoot()).isEqualTo(
-        new PathFragment("external/foo"));
+    assertThat(fooA.getRepository().getSourceRoot()).isEqualTo(new PathFragment("external/foo"));
 
     PackageIdentifier absoluteA = PackageIdentifier.parse("//a");
     assertThat(absoluteA.getRepository().strippedName()).isEqualTo("");
@@ -101,10 +98,12 @@ public class PackageIdentifierTest {
   }
 
   @Test
-  public void testRunfilesDir() throws Exception {
-    assertThat(PackageIdentifier.create("@foo", new PathFragment("bar/baz")).getRunfilesPath())
+  public void testPathUnderExecRoot() throws Exception {
+    assertThat(
+        PackageIdentifier.create("@foo", new PathFragment("bar/baz")).getPathUnderExecRoot())
         .isEqualTo(new PathFragment("../foo/bar/baz"));
-    assertThat(PackageIdentifier.create("@", new PathFragment("bar/baz")).getRunfilesPath())
+    assertThat(
+        PackageIdentifier.create("@", new PathFragment("bar/baz")).getPathUnderExecRoot())
         .isEqualTo(new PathFragment("bar/baz"));
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java
index 9a39bbfda1..24a19172c7 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java
@@ -58,12 +58,12 @@ public class RepositoryNameTest {
   }
 
   @Test
-  public void testRunfilesDir() throws Exception {
-    assertThat(RepositoryName.create("@foo").getRunfilesPath())
+  public void testPathUnderExecRoot() throws Exception {
+    assertThat(RepositoryName.create("@foo").getPathUnderExecRoot())
         .isEqualTo(new PathFragment("../foo"));
-    assertThat(RepositoryName.create("@").getRunfilesPath())
+    assertThat(RepositoryName.create("@").getPathUnderExecRoot())
         .isEqualTo(PathFragment.EMPTY_FRAGMENT);
-    assertThat(RepositoryName.create("").getRunfilesPath())
+    assertThat(RepositoryName.create("").getPathUnderExecRoot())
         .isEqualTo(PathFragment.EMPTY_FRAGMENT);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
index 1fab14fa9b..5b3d20ca95 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
@@ -696,7 +696,7 @@ public class CcCommonTest extends BuildViewTestCase {
     checkError(
         "test",
         "bad_relative_include",
-        "Path references a path above the execution root.",
+        "../.. references a path above the execution root (..).",
         "cc_library(name='bad_relative_include', srcs=[], includes=['../..'])");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 2099df68a1..e476138dbe 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -1121,8 +1121,8 @@ public class CcLibraryConfiguredTargetTest extends BuildViewTestCase {
   @Test
   public void testSystemIncludePathsOutsideExecutionRoot() throws Exception {
     checkError("root", "a",
-        "The include path '../system' references a path outside of the execution root.",
-        "cc_library(name='a', srcs=['a.cc'], copts=['-isystem../system'])");
+        "The include path '../../system' references a path outside of the execution root.",
+        "cc_library(name='a', srcs=['a.cc'], copts=['-isystem../../system'])");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 00b27d247e..af79883d2a 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -340,7 +340,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("@r//:cclib");
     assertContainsEvent(
-        "/external/r/BUILD:2:10: Label '@r//:sub/my_sub_lib.h' crosses boundary of "
+        "/r/BUILD:2:10: Label '@r//:sub/my_sub_lib.h' crosses boundary of "
             + "subpackage '@r//sub' (perhaps you meant to put the colon here: "
             + "'@r//sub:my_sub_lib.h'?)");
   }
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java b/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java
index 8db1105771..ba50706f2e 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java
@@ -65,7 +65,7 @@ public class TestConstants {
         "com.google.devtools.build.lib.bazel.rules.BazelRulesModule";
   public static final ImmutableList<String> IGNORED_MESSAGE_PREFIXES = ImmutableList.<String>of();
 
-  public static final String GCC_INCLUDE_PATH = "external/bazel_tools/tools/cpp/gcc3";
+  public static final String GCC_INCLUDE_PATH = "../bazel_tools/tools/cpp/gcc3";
 
   public static final String TOOLS_REPOSITORY = "@bazel_tools";