Store content digests in repository marker files. https://www.bazel.build/designs/2016/10/18/repository-invalidation.html

Change-Id: I6cb01397a35cd32169a0e415f8d7f944e7d840df
PiperOrigin-RevId: 166200841

5 files changed, 81 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
index 72a873c0cf..de78271c3a 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
@@ -714,13 +714,17 @@ public class SkylarkRepositoryContext {
     if (fileValue == null) {
       throw RepositoryFunction.restart();
     }
-    if (!fileValue.isFile()) {
-      throw new EvalException(Location.BUILTIN,
-          "Not a file: " + rootedPath.asPath().getPathString());
+    if (!fileValue.isFile() || fileValue.isSpecialFile()) {
+      throw new EvalException(
+          Location.BUILTIN, "Not a regular file: " + rootedPath.asPath().getPathString());
     }
 
-    // A label do not contains space so it safe to use as a key.
-    markerData.put("FILE:" + label, Integer.toString(fileValue.realFileStateValue().hashCode()));
+    // A label does not contains space so it safe to use as a key.
+    try {
+      markerData.put("FILE:" + label, RepositoryFunction.fileValueToMarkerValue(fileValue));
+    } catch (IOException e) {
+      throw new EvalException(Location.BUILTIN, e);
+    }
     return new SkylarkPath(rootedPath.asPath());
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java
index 094f5e5ec7..6317a65dca 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewRepositoryFileHandler.java
@@ -157,8 +157,11 @@ public class NewRepositoryFileHandler {
           // TODO(pcloudy): Don't add absolute path into markerData once it's not supported
           fileKey = fileValue.realRootedPath().asPath().getPathString();
         }
-        markerData.put(
-            "FILE:" + fileKey, Integer.toString(fileValue.realFileStateValue().hashCode()));
+        try {
+          markerData.put("FILE:" + fileKey, RepositoryFunction.fileValueToMarkerValue(fileValue));
+        } catch (IOException e) {
+          throw new RepositoryFunctionException(e, Transience.TRANSIENT);
+        }
       } else if (fileContent != null) {
         RepositoryFunction.writeFile(outputDirectory, filename, fileContent);
       } else {
@@ -265,6 +268,13 @@ public class NewRepositoryFileHandler {
             Transience.TRANSIENT);
       }
 
+      if (!fileValue.isFile() || fileValue.isSpecialFile()) {
+        throw new RepositoryFunctionException(
+            new EvalException(
+                rule.getLocation(), String.format("%s is not a regular file", rootedFile.asPath())),
+            Transience.PERSISTENT);
+      }
+
       return fileValue;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index 469eb61222..e82ae7bd60 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.rules.repository;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -30,6 +31,7 @@ import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.rules.ExternalPackageUtil;
 import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
+import com.google.devtools.build.lib.skyframe.FileStateValue.RegularFileStateValue;
 import com.google.devtools.build.lib.skyframe.FileSymlinkException;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
@@ -222,11 +224,11 @@ public abstract class RepositoryFunction {
                   FileSymlinkException.class,
                   InconsistentFilesystemException.class);
 
-      if (fileValue == null || !fileValue.isFile()) {
+      if (fileValue == null || !fileValue.isFile() || fileValue.isSpecialFile()) {
         return false;
       }
 
-      return Objects.equals(value, Integer.toString(fileValue.realFileStateValue().hashCode()));
+      return Objects.equals(value, fileValueToMarkerValue(fileValue));
     } catch (LabelSyntaxException e) {
       throw new IllegalStateException(
           "Key " + key + " is not a correct file key (should be in form FILE:label)", e);
@@ -239,6 +241,22 @@ public abstract class RepositoryFunction {
     }
   }
 
+  /**
+   * Convert to a @{link com.google.devtools.build.lib.skyframe.FileValue} to a String appropriate
+   * for placing in a repository marker file.
+   *
+   * @param fileValue The value to convert. It must correspond to a regular file.
+   */
+  public static String fileValueToMarkerValue(FileValue fileValue) throws IOException {
+    Preconditions.checkArgument(fileValue.isFile() && !fileValue.isSpecialFile());
+    // Return the file content digest in hex. fileValue may or may not have the digest available.
+    byte[] digest = ((RegularFileStateValue) fileValue.realFileStateValue()).getDigest();
+    if (digest == null) {
+      digest = fileValue.realRootedPath().asPath().getDigest();
+    }
+    return BaseEncoding.base16().lowerCase().encode(digest);
+  }
+
   static boolean verifyMarkerDataForFiles(
       Rule rule, Map<String, String> markerData, Environment env) throws InterruptedException {
     for (Map.Entry<String, String> entry : markerData.entrySet()) {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java
index d550924212..722067011e 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryFunctionTest.java
@@ -17,13 +17,20 @@ package com.google.devtools.build.lib.rules.repository;
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.io.BaseEncoding;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.skyframe.FileContentsProxy;
+import com.google.devtools.build.lib.skyframe.FileStateValue;
+import com.google.devtools.build.lib.skyframe.FileStateValue.RegularFileStateValue;
+import com.google.devtools.build.lib.skyframe.FileValue;
+import com.google.devtools.build.lib.skyframe.FileValue.RegularFileValue;
 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;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import java.util.Map;
@@ -112,4 +119,20 @@ public class RepositoryFunctionTest extends BuildViewTestCase {
     assertMarkerFileEscaping("a \\\nb");
     assertMarkerFileEscaping("a \nb");
   }
+
+  @Test
+  public void testFileValueToMarkerValue() throws Exception {
+    RootedPath path = RootedPath.toRootedPath(rootDirectory, scratch.file("foo", "bar"));
+
+    // Digest should be returned if the FileStateValue has it.
+    FileStateValue fsv = new RegularFileStateValue(3, 100, new byte[] {1, 2, 3, 4}, null);
+    FileValue fv = new RegularFileValue(path, fsv);
+    assertThat(RepositoryFunction.fileValueToMarkerValue(fv)).isEqualTo("01020304");
+
+    // Digest should also be returned if the FileStateValue doesn't have it.
+    fsv = new RegularFileStateValue(3, 100, null, new FileContentsProxy(100, 200));
+    fv = new RegularFileValue(path, fsv);
+    String expectedDigest = BaseEncoding.base16().lowerCase().encode(path.asPath().getDigest());
+    assertThat(RepositoryFunction.fileValueToMarkerValue(fv)).isEqualTo(expectedDigest);
+  }
 }
diff --git a/src/test/py/bazel/bazel_external_repository_test.py b/src/test/py/bazel/bazel_external_repository_test.py
index 098aac7fb0..50dcb57351 100644
--- a/src/test/py/bazel/bazel_external_repository_test.py
+++ b/src/test/py/bazel/bazel_external_repository_test.py
@@ -58,7 +58,23 @@ class BazelExternalRepositoryTest(test_base.TestBase):
     exit_code, _, stderr = self.RunBazel(['build', '@six_archive//...'])
     self.assertEqual(exit_code, 0, os.linesep.join(stderr))
 
-    # Test Repository reloading after build file changes
+    fetching_disabled_msg = 'fetching is disabled'
+
+    # Changing the mtime of the BUILD file shouldn't invalidate it.
+    os.utime(self.Path('third_party/six.BUILD'), (100, 200))
+    exit_code, _, stderr = self.RunBazel(
+        ['build', '--nofetch', '@six_archive//...'])
+    self.assertEqual(exit_code, 0, os.linesep.join(stderr))
+    self.assertNotIn(fetching_disabled_msg, os.linesep.join(stderr))
+
+    # Check that --nofetch prints a warning if the BUILD file is changed.
+    self.ScratchFile('third_party/six.BUILD', build_file + ['"a noop string"'])
+    exit_code, _, stderr = self.RunBazel(
+        ['build', '--nofetch', '@six_archive//...'])
+    self.assertEqual(exit_code, 0, os.linesep.join(stderr))
+    self.assertIn(fetching_disabled_msg, os.linesep.join(stderr))
+
+    # Test repository reloading after BUILD file changes.
     self.ScratchFile('third_party/six.BUILD', build_file + ['foobar'])
     exit_code, _, stderr = self.RunBazel(['build', '@six_archive//...'])
     self.assertEqual(exit_code, 1, os.linesep.join(stderr))