Remove circular symlinks to external dependencies

Fixes #87.

--
MOS_MIGRATED_REVID=91784426

src/main/java/com/google/devtools/build/lib/bazel/repository/RepositoryFunction.java

@@ -27,10 +27,8 @@
 import com.google.devtools.build.lib.skyframe.FileSymlinkCycleException;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
-import com.google.devtools.build.lib.skyframe.PackageFunction;
 import com.google.devtools.build.lib.skyframe.PackageValue;
 import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.RootedPath;
@@ -48,7 +46,6 @@
  * Parent class for repository-related Skyframe functions.
  */
 public abstract class RepositoryFunction implements SkyFunction {
-  private static final String EXTERNAL_REPOSITORY_DIRECTORY = ".external-repository";
   private BlazeDirectories directories;
 
   @Override
@@ -73,7 +70,7 @@ protected Path getExternalRepositoryDirectory() {
   }
 
   public static Path getExternalRepositoryDirectory(BlazeDirectories directories) {
-    return directories.getOutputBase().getRelative(EXTERNAL_REPOSITORY_DIRECTORY);
+    return directories.getOutputBase().getRelative(ExternalPackage.NAME);
   }
 
   /**
@@ -107,15 +104,15 @@ public static Rule getRule(
       RepositoryName repositoryName, @Nullable String ruleClassName, Environment env)
       throws RepositoryFunctionException {
     SkyKey packageKey = PackageValue.key(
-        PackageIdentifier.createInDefaultRepo(PackageFunction.EXTERNAL_PACKAGE_NAME));
+        PackageIdentifier.createInDefaultRepo(ExternalPackage.NAME));
     PackageValue packageValue;
     try {
       packageValue = (PackageValue) env.getValueOrThrow(packageKey,
           NoSuchPackageException.class);
     } catch (NoSuchPackageException e) {
       throw new RepositoryFunctionException(
           new BuildFileNotFoundException(
-              PackageFunction.EXTERNAL_PACKAGE_NAME, "Could not load //external package"),
+              ExternalPackage.NAME, "Could not load //external package"),
           Transience.PERSISTENT);
     }
     if (packageValue == null) {
@@ -126,7 +123,7 @@ public static Rule getRule(
     if (rule == null) {
       throw new RepositoryFunctionException(
           new BuildFileContainsErrorsException(
-              PackageFunction.EXTERNAL_PACKAGE_NAME,
+              ExternalPackage.NAME,
               "The repository named '" + repositoryName + "' could not be resolved"),
           Transience.PERSISTENT);
     }
@@ -138,9 +135,6 @@ public static Rule getRule(
   /**
    * Adds the repository's directory to the graph and, if it's a symlink, resolves it to an
    * actual directory.
-   *
-   * <p>Also creates a symlink from x/external/x to x, where x is the directory containing a
-   * WORKSPACE file. This is used in the execution root.</p>
    */
   @Nullable
   protected static FileValue getRepositoryDirectory(Path repositoryDirectory, Environment env)
@@ -156,20 +150,6 @@ protected static FileValue getRepositoryDirectory(Path repositoryDirectory, Envi
           new IOException("Could not access " + repositoryDirectory + ": " + e.getMessage()),
           Transience.PERSISTENT);
     }
-
-    String targetName = repositoryDirectory.getBaseName();
-    try {
-      Path backlink = repositoryDirectory.getRelative("external").getRelative(targetName);
-      FileSystemUtils.createDirectoryAndParents(backlink.getParentDirectory());
-      if (backlink.exists()) {
-        backlink.delete();
-      }
-      backlink.createSymbolicLink(repositoryDirectory);
-    } catch (IOException e) {
-      throw new RepositoryFunctionException(new IOException(
-          "Error creating execution root symlink for " + targetName + ": " + e.getMessage()),
-          Transience.TRANSIENT);
-    }
     return value;
   }
 

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -110,7 +110,7 @@ protected NameConflictException(String message) {
 
   /**
    * The root of the source tree in which this package was found. It is an invariant that
-   * {@code sourceRoot.getRelative(name).equals(packageDirectory)}.
+   * {@code sourceRoot.getRelative(packageId.getPathFragment()).equals(packageDirectory)}.
    */
   private Path sourceRoot;
 
@@ -312,9 +312,9 @@ protected void setDefaultRestrictedTo(Set<Label> environments) {
     defaultRestrictedTo = environments;
   }
 
-  public static Path getSourceRoot(Path buildFile, PathFragment nameFragment) {
+  private static Path getSourceRoot(Path buildFile, PathFragment packageFragment) {
     Path current = buildFile.getParentDirectory();
-    for (int i = 0, len = nameFragment.segmentCount(); i < len && current != null; i++) {
+    for (int i = 0, len = packageFragment.segmentCount(); i < len && current != null; i++) {
       current = current.getParentDirectory();
     }
     return current;
@@ -341,12 +341,12 @@ protected void finishInit(Builder builder) {
     this.filename = builder.filename;
     this.packageDirectory = filename.getParentDirectory();
 
-    this.sourceRoot = getSourceRoot(filename, nameFragment);
+    this.sourceRoot = getSourceRoot(filename, packageIdentifier.getPathFragment());
     if ((sourceRoot == null
-        || !sourceRoot.getRelative(nameFragment).equals(packageDirectory))
+        || !sourceRoot.getRelative(packageIdentifier.getPathFragment()).equals(packageDirectory))
         && !filename.getBaseName().equals("WORKSPACE")) {
       throw new IllegalArgumentException(
-          "Invalid BUILD file name for package '" + name + "': " + filename);
+          "Invalid BUILD file name for package '" + packageIdentifier + "': " + filename);
     }
 
     this.makeEnv = builder.makeEnv.build();
@@ -395,7 +395,7 @@ public Path getFilename() {
    * Returns the source root (a directory) beneath which this package's BUILD file was found.
    *
    * Assumes invariant:
-   * {@code getSourceRoot().getRelative(getName()).equals(getPackageDirectory())}
+   * {@code getSourceRoot().getRelative(packageId.getPathFragment()).equals(getPackageDirectory())}
    */
   public Path getSourceRoot() {
     return sourceRoot;

src/test/shell/bazel/external_integration_test.sh

@@ -406,6 +406,7 @@ cat external/endangered/fox/male
 EOF
   chmod +x zoo/female.sh
 
+  bazel clean --expunge
   bazel run //zoo:breeding-program >& $TEST_log \
     || echo "Expected build/run to succeed"
   kill_nc

src/test/shell/bazel/local_repository_test.sh

@@ -366,4 +366,59 @@ EOF
   expect_log "@x-repo//x  to //a:a"
 }
 
+function test_external_includes() {
+  clib=$TEST_TMPDIR/clib
+  mkdir -p $clib/include
+  cat > $clib/include/clib.h <<EOF
+int x();
+EOF
+  cat > $clib/clib.cc <<EOF
+#include "clib.h"
+int x() {
+  return 3;
+}
+EOF
+  cat > $clib/BUILD <<EOF
+cc_library(
+    name = "clib",
+    srcs = ["clib.cc"],
+    hdrs = glob(["**/*.h"]),
+    includes = ["include"],
+    visibility = ["//visibility:public"],
+)
+EOF
+
+  cat > WORKSPACE <<EOF
+local_repository(
+    name = "clib-repo",
+    path = "$clib",
+)
+
+bind(
+    name = "clib",
+    actual = "@clib-repo//:clib"
+)
+EOF
+  cat > BUILD <<EOF
+cc_binary(
+    name = "printer",
+    srcs = ["printer.cc"],
+    deps = ["//external:clib"],
+)
+EOF
+  cat > printer.cc <<EOF
+#include <stdio.h>
+
+#include "clib.h"
+
+int main() {
+  printf("My number is %d\n", x());
+  return 0;
+}
+EOF
+
+  bazel run //:printer >& $TEST_log || fail "Running //:printer failed"
+  expect_log "My number is 3"
+}
+
 run_suite "local repository tests"