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, 1 insertions, 0 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index debcc37f87..d7789b7cc1 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -982,6 +982,7 @@ java_test(
         ":testutil",
         "//src/main/java/com/google/devtools/build/lib:bazel-main",
         "//src/main/java/com/google/devtools/build/lib:bazel-repository",
+        "//src/main/java/com/google/devtools/build/lib:build-base",
         "//src/main/java/com/google/devtools/build/lib:collect",
         "//src/main/java/com/google/devtools/build/lib:inmemoryfs",
         "//src/main/java/com/google/devtools/build/lib:vfs",