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

5 files changed, 50 insertions, 18 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index c7f11d8f29..35d5a99504 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -396,9 +396,13 @@ public final class CcCommon {
         continue;
       }
       PathFragment includesPath = packageFragment.getRelative(includesAttr).normalize();
-      if (!includesPath.isNormalized()) {
+      // It's okay for the includes path to start with ../workspace-name for external repos.
+      if ((packageIdentifier.getRepository().isMain() && !includesPath.isNormalized())
+          || (!packageIdentifier.getRepository().isMain()
+              && !includesPath.startsWith(
+                  packageIdentifier.getRepository().getPathUnderExecRoot()))) {
         ruleContext.attributeError("includes",
-            "Path references a path above the execution root.");
+            includesAttr + " references a path above the execution root (" + includesPath + ").");
       }
       if (includesPath.segmentCount() == 0) {
         ruleContext.attributeError(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index de7373a8ba..2e64a02e03 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -1106,7 +1106,7 @@ public final class CcLibraryHelper {
         ruleContext.getLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot();
     contextBuilder.addQuoteIncludeDir(repositoryPath);
     contextBuilder.addQuoteIncludeDir(
-        ruleContext.getConfiguration().getGenfilesFragment().getRelative(repositoryPath));
+        repositoryPath.getRelative(ruleContext.getConfiguration().getGenfilesFragment()));
 
     for (PathFragment systemIncludeDir : systemIncludeDirs) {
       contextBuilder.addSystemIncludeDir(systemIncludeDir);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 21d0d307c5..d878e90f63 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.lib.analysis.actions.ExecutionInfoSpecifier;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.CollectionUtils;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -328,10 +329,15 @@ public class CppCompileAction extends AbstractAction
         continue;
       }
 
-      if (include.isAbsolute()
-          || !PathFragment.EMPTY_FRAGMENT.getRelative(include).normalize().isNormalized()) {
-        ruleContext.ruleError(
-            "The include path '" + include + "' references a path outside of the execution root.");
+      // One starting ../ is okay for getting to a sibling repository.
+      PathFragment originalInclude = include;
+      if (include.startsWith(new PathFragment(Label.EXTERNAL_PATH_PREFIX))) {
+        include = include.relativeTo(Label.EXTERNAL_PATH_PREFIX);
+      }
+
+      if (include.isAbsolute() || !include.normalize().isNormalized()) {
+        ruleContext.ruleError("The include path '" + originalInclude
+            + "' references a path outside of the execution root.");
       }
     }
   }
@@ -971,6 +977,7 @@ public class CppCompileAction extends AbstractAction
         if (execPath.getBaseName().endsWith(".pcm")) {
           continue;
         }
+        RepositoryName repositoryName = RepositoryName.MAIN;
         PathFragment execPathFragment = execPath.asFragment();
         if (execPathFragment.isAbsolute()) {
           // Absolute includes from system paths are ignored.
@@ -983,14 +990,25 @@ public class CppCompileAction extends AbstractAction
           // the build with an error.
           if (execPath.startsWith(execRoot)) {
             execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path
+          } else if (execPath.startsWith(execRoot.getParentDirectory())) {
+            // External repository.
+            execPathFragment = execPath.relativeTo(execRoot.getParentDirectory());
+            String workspace = execPathFragment.getSegment(0);
+            execPathFragment = execPathFragment.relativeTo(workspace);
+            try {
+              repositoryName = RepositoryName.create("@" + workspace);
+            } catch (LabelSyntaxException e) {
+              throw new IllegalStateException(workspace + " is not a valid repository name");
+            }
           } else {
             problems.add(execPathFragment.getPathString());
             continue;
           }
         }
-        Artifact artifact = allowedDerivedInputsMap.get(execPathFragment);
+        Artifact artifact = allowedDerivedInputsMap.get(
+            repositoryName.getPathUnderExecRoot().getRelative(execPathFragment));
         if (artifact == null) {
-          artifact = artifactResolver.resolveSourceArtifact(execPathFragment, RepositoryName.MAIN);
+          artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repositoryName);
         }
         if (artifact != null) {
           inputs.add(artifact);
@@ -1003,7 +1021,7 @@ public class CppCompileAction extends AbstractAction
           problems.add(execPathFragment.getPathString());
         }
       }
-      //TODO(b/22551695): Remove in favor of seperate implementations.
+      //TODO(b/22551695): Remove in favor of separate implementations.
       if (semantics == null || semantics.needsIncludeValidation()) {
         problems.assertProblemFree(this, getSourceFile());
       }
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 f5e0ab5f39..7e7e83ae21 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
@@ -485,7 +485,7 @@ public class CppHelper {
         Preconditions.checkState(Link.SHARED_LIBRARY_FILETYPES.matches(artifact.getFilename()));
         symlinkedArtifacts.add(isCppRuntime
             ? SolibSymlinkAction.getCppRuntimeSymlink(
-                ruleContext, artifact, solibDirOverride, configuration)
+            ruleContext, artifact, solibDirOverride, configuration)
             : SolibSymlinkAction.getDynamicLibrarySymlink(
                 ruleContext, artifact, false, true, configuration));
       }
@@ -494,7 +494,8 @@ public class CppHelper {
     }
     return ImmutableList.of(
         factory.createMiddlemanAllowMultiple(ruleContext.getAnalysisEnvironment(), actionOwner,
-            ruleContext.getPackageDirectory(), purpose, artifacts,
+            ruleContext.getLabel().getPackageIdentifier().getPathUnderExecRoot(), purpose,
+            artifacts,
             configuration.getMiddlemanDirectory(ruleContext.getRule().getRepository())));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
index 512281ad93..e77d02a71d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
@@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactResolver;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -93,7 +94,7 @@ public class HeaderDiscovery {
       return inputs.build();
     }
     List<Path> systemIncludePrefixes = permittedSystemIncludePrefixes;
-
+    RepositoryName repositoryName = RepositoryName.MAIN;
     // Check inclusions.
     IncludeProblems problems = new IncludeProblems();
     for (Path execPath : depSet.getDependencies()) {
@@ -111,16 +112,24 @@ public class HeaderDiscovery {
         // non-system include paths here should never be absolute. If they
         // are, it's probably due to a non-hermetic #include, & we should stop
         // the build with an error.
+        // funky but tolerable path
         if (execPath.startsWith(execRoot)) {
-          execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path
-        } else {
-          problems.add(execPathFragment.getPathString());
-          continue;
+          execPathFragment = execPath.relativeTo(execRoot);
+        } else if (execPath.startsWith(execRoot.getParentDirectory())) {
+          // External repository.
+          execPathFragment = execPath.relativeTo(execRoot.getParentDirectory());
+          String workspace = execPathFragment.getSegment(0);
+          execPathFragment = execPathFragment.relativeTo(workspace);
+          try {
+            repositoryName = RepositoryName.create("@" + workspace);
+          } catch (LabelSyntaxException e) {
+            throw new IllegalStateException(workspace + " is not a valid repository name");
+          }
         }
       }
       Artifact artifact = allowedDerivedInputsMap.get(execPathFragment);
       if (artifact == null) {
-        artifact = artifactResolver.resolveSourceArtifact(execPathFragment, RepositoryName.MAIN);
+        artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repositoryName);
       }
       if (artifact != null) {
         inputs.add(artifact);