ArtifactFactory.findSourceRoot now handles the case where an execPath refers to a remote repository.

--
MOS_MIGRATED_REVID=106051348

4 files changed, 86 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
index c4fae04531..67b210e8ca 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
@@ -21,7 +21,9 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -318,20 +320,38 @@ public class ArtifactFactory implements ArtifactResolver, ArtifactSerializer, Ar
     return createArtifactIfNotValid(findSourceRoot(execPath, baseExecPath, baseRoot), execPath);
   }
 
+  /**
+   * Probe the known packages to find the longest package prefix up until the base, or until the
+   * root directory if our execPath doesn't start with baseExecPath due to uplevel references.
+   */
   @Nullable
   private Root findSourceRoot(
       PathFragment execPath, @Nullable PathFragment baseExecPath, @Nullable Root baseRoot) {
-    // Probe the known packages to find the longest package prefix up until the base, or until the
-    // root directory if our execPath doesn't start with baseExecPath due to uplevel references.
-    PathFragment dir;
-    for (dir = execPath.getParentDirectory();
-        dir != null && !dir.equals(baseExecPath);
-        dir = dir.getParentDirectory()) {
-      Root sourceRoot = packageRoots.get(PackageIdentifier.createInDefaultRepo(dir));
+    PathFragment dir = execPath.getParentDirectory();
+    if (dir == null) {
+      return null;
+    }
+
+    // This obviously only works when a repository name does not contain a slash. However, this is
+    // fine, because LocalRepositoryFunction checks that the name doesn't contain one.
+    RepositoryName repoName = PackageIdentifier.DEFAULT_REPOSITORY_NAME;
+    if (dir.segmentCount() >= 2 && dir.getSegment(0).equals("external")) {
+      try {
+        repoName = RepositoryName.create("@" + dir.getSegment(1));
+      } catch (LabelSyntaxException e) {
+        return null;
+      }
+      dir = dir.subFragment(2, dir.segmentCount());
+    }
+
+    while (dir != null && !dir.equals(baseExecPath)) {
+      Root sourceRoot = packageRoots.get(PackageIdentifier.create(repoName, dir));
       if (sourceRoot != null) {
         return sourceRoot;
       }
+      dir = dir.getParentDirectory();
     }
+
     return dir != null && dir.equals(baseExecPath) ? baseRoot : null;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalRepositoryFunction.java
index fe62852c34..c5b6e3cae4 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalRepositoryFunction.java
@@ -48,6 +48,13 @@ public class LocalRepositoryFunction extends RepositoryFunction {
       return null;
     }
 
+    if (rule.getName().contains("/")) {
+      throw new RepositoryFunctionException(
+          new EvalException(
+              rule.getLocation(), "In " + rule + " the 'name' attribute must not contain slashes"),
+          Transience.PERSISTENT);
+    }
+
     AggregatingAttributeMapper mapper = AggregatingAttributeMapper.of(rule);
     String path = mapper.get("path", Type.STRING);
     PathFragment pathFragment = new PathFragment(path);
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
index 21a19ed290..aaf5fca146 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactFactoryTest.java
@@ -53,6 +53,7 @@ public class ArtifactFactoryTest {
   private Path execRoot;
   private Root clientRoot;
   private Root clientRoRoot;
+  private Root alienRoot;
   private Root outRoot;
 
   private PathFragment fooPath;
@@ -63,6 +64,10 @@ public class ArtifactFactoryTest {
   private PackageIdentifier barPackage;
   private PathFragment barRelative;
 
+  private PathFragment alienPath;
+  private PackageIdentifier alienPackage;
+  private PathFragment alienRelative;
+
   private ArtifactFactory artifactFactory;
 
   @Before
@@ -70,6 +75,7 @@ public class ArtifactFactoryTest {
     execRoot = scratch.dir("/output/workspace");
     clientRoot = Root.asSourceRoot(scratch.dir("/client/workspace"));
     clientRoRoot = Root.asSourceRoot(scratch.dir("/client/RO/workspace"));
+    alienRoot = Root.asSourceRoot(scratch.dir("/client/workspace"));
     outRoot = Root.asDerivedRoot(execRoot, execRoot.getRelative("out-root/x/bin"));
 
     fooPath = new PathFragment("foo");
@@ -80,6 +86,10 @@ public class ArtifactFactoryTest {
     barPackage = PackageIdentifier.createInDefaultRepo(barPath);
     barRelative = barPath.getRelative("barsource.txt");
 
+    alienPath = new PathFragment("external/alien");
+    alienPackage = PackageIdentifier.create("@alien", alienPath);
+    alienRelative = alienPath.getRelative("alien.txt");
+
     artifactFactory = new ArtifactFactory(execRoot);
     setupRoots();
   }
@@ -88,6 +98,7 @@ public class ArtifactFactoryTest {
     Map<PackageIdentifier, Root> packageRootMap = new HashMap<>();
     packageRootMap.put(fooPackage, clientRoot);
     packageRootMap.put(barPackage, clientRoRoot);
+    packageRootMap.put(alienPackage, alienRoot);
     artifactFactory.setPackageRoots(packageRootMap);
     artifactFactory.setDerivedArtifactRoots(ImmutableList.of(outRoot));
   }
@@ -114,6 +125,13 @@ public class ArtifactFactoryTest {
   }
 
   @Test
+  public void testResolveArtifact_inExternalRepo() throws Exception {
+    assertSame(
+        artifactFactory.getSourceArtifact(alienRelative, alienRoot),
+        artifactFactory.resolveSourceArtifact(alienRelative));
+  }
+
+  @Test
   public void testResolveArtifact_noDerived_derivedRoot() throws Exception {
     assertNull(artifactFactory.resolveSourceArtifact(
             outRoot.getPath().getRelative(fooRelative).relativeTo(execRoot)));
diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh
index afd8e30594..71d0c82db7 100755
--- a/src/test/shell/bazel/local_repository_test.sh
+++ b/src/test/shell/bazel/local_repository_test.sh
@@ -171,7 +171,7 @@ EOF
 }
 
 function test_non_existent_external_ref() {
-  mkdir zoo
+  mkdir -p zoo
   touch zoo/BallPit.java
   cat > zoo/BUILD <<EOF
 java_binary(
@@ -448,6 +448,8 @@ int main() {
 EOF
 
   bazel fetch //:printer || fail "Fetch failed"
+  bazel build @clib-repo//:clib >& $TEST_log \
+    || fail "Building @clib-repo//:clib failed"
   bazel run //:printer >& $TEST_log || fail "Running //:printer failed"
   expect_log "My number is 3"
 }
@@ -616,6 +618,9 @@ cc_binary(
     srcs = ["bin.cc"],
 )
 EOF
+  cat > $r/bin.cc <<EOF
+int main() { return 0; };
+EOF
 
   cat > WORKSPACE <<EOF
 local_repository(
@@ -624,7 +629,7 @@ local_repository(
 )
 EOF
 
-  bazel build --nobuild @r//:bin || fail "build failed"
+  bazel build @r//:bin || fail "build failed"
 }
 
 function test_output_file_in_local_repository() {
@@ -892,4 +897,31 @@ EOF
   expect_log "@r a"
 }
 
+function test_slash_in_repo_name() {
+  local r=$TEST_TMPDIR/r
+  rm -fr $r
+  mkdir -p $r/a
+
+  touch $r/a/WORKSPACE
+  cat > $r/a/BUILD <<EOF
+cc_binary(
+    name = "bin",
+    srcs = ["bin.cc"],
+)
+EOF
+  cat > $r/a/bin.cc <<EOF
+int main() { return 0; };
+EOF
+
+  cat > WORKSPACE <<EOF
+local_repository(
+    name = "r/a",
+    path = "$r/a",
+)
+EOF
+
+  bazel build @r/a//:bin &> $TEST_log && fail "expected build failure, but succeeded"
+  expect_log "the 'name' attribute must not contain slashes"
+}
+
 run_suite "local repository tests"