Simplify the contract of RepositoryFunction to "I am given a Rule and a directory that I should populate". The directory itself is not created because local_repository actually puts a symlink in its place.

As a side effect, make HTTP downloading, git cloning and archive decompressing not be SkyFunctions. This is necessary because it needs to be the RepositoryFunction and not a 
dependent SkyFunction that populates the output directory, because it that's the case what happens is:

1. RepositoryDelegatorFunction cleans up the directory and prepares it for RepositoryFunction
2. RepositoryFunction calls env.getValue(<function that populates the directory>)
3. That value hasn't been computed yet, thus RepositoryDelegatorFunction returns early
4. The function that populates the directory is called
5. RepositoryDelegatorFunction is restarted 
6. RepositoryDelegatorFunction cleans up the directory
7. RepositoryFunction calls env.getValue(), and nothing is done because the value has already been computed
8. RepositoryDelegatorFunction proudly returns, even though the directory is actually empty

Another way to solve this problem would be to make RepositoryFunction not clean the directory up on Skyframe restarts, but that means that we'd need to keep state somewhere, which doesn't strike me as a particularly great idea because let's keep state outside of Skyframe only when absolutely necessary (e.g. the marker files for cross-server instance persistence of downloaded repositories). That "somewhere" could either be a member variable of RepositoryDelegatorFunction or the file system.

Note that this change causes external communication to be re-done in a few more cases than before (see that changes to the test cases), but I'd rather we be correct and simple than fast. We can optimize things later if needed and there is enough complexity going around, thank you very much.

--
MOS_MIGRATED_REVID=110134397

1 files changed, 21 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
index 4200d92f4c..a0d318db22 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.rules.repository.RepositoryFunction.Reposit
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.RepositoryValue;
 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.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -58,6 +59,15 @@ public class RepositoryDelegatorFunction implements SkyFunction {
     this.isFetch = isFetch;
   }
 
+  private void setupRepositoryRoot(Path repoRoot) throws RepositoryFunctionException {
+    try {
+      FileSystemUtils.deleteTree(repoRoot);
+      FileSystemUtils.createDirectoryAndParents(repoRoot.getParentDirectory());
+    } catch (IOException e) {
+      throw new RepositoryFunctionException(e, Transience.TRANSIENT);
+    }
+  }
+
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env)
       throws SkyFunctionException, InterruptedException {
@@ -75,18 +85,20 @@ public class RepositoryDelegatorFunction implements SkyFunction {
           "Could not find handler for " + rule), Transience.PERSISTENT);
     }
 
+    Path repoRoot =
+        RepositoryFunction.getExternalRepositoryDirectory(directories).getRelative(rule.getName());
+
     if (handler.isLocal()) {
       // Local repositories are always fetched because the operation is generally fast and they do
       // not depend on non-local data, so it does not make much sense to try to catch from across
       // server instances.
-      return handler.fetch(rule, env);
+      setupRepositoryRoot(repoRoot);
+      return handler.fetch(rule, repoRoot, env);
     }
 
     // We check the repository root for existence here, but we can't depend on the FileValue,
     // because it's possible that we eventually create that directory in which case the FileValue
     // and the state of the file system would be inconsistent.
-    Path repoRoot =
-        RepositoryFunction.getExternalRepositoryDirectory(directories).getRelative(rule.getName());
 
     byte[] ruleSpecificData = handler.getRuleSpecificMarkerData(rule, env);
     boolean markerUpToDate = handler.isFilesystemUpToDate(rule, ruleSpecificData);
@@ -106,11 +118,16 @@ public class RepositoryDelegatorFunction implements SkyFunction {
 
     if (isFetch.get()) {
       // Fetching enabled, go ahead.
-      SkyValue result = handler.fetch(rule, env);
+      setupRepositoryRoot(repoRoot);
+      SkyValue result = handler.fetch(rule, repoRoot, env);
       if (env.valuesMissing()) {
         return null;
       }
 
+      // No new Skyframe dependencies must be added between calling the repository implementation
+      // and writing the marker file because if they aren't computed, it would cause a Skyframe
+      // restart thus calling the possibly very slow (networking, decompression...) fetch()
+      // operation again. So we write the marker file here immediately.
       handler.writeMarkerFile(rule, ruleSpecificData);
       return result;
     }