Add a normal startup-option for setting the digest function.

We continue to support the jvm property -Dbazel.DigestFunction, for backwards compatibility, but this will go away. The startup-option is marked experimental for now as we iron out issues. (note: leaving this out of release notes until the experimental tag is removed)

As part of this refactor, the default constructor calls for FileSystem and derived classes will now use this default. This should remove the need for constructors that accept custom hash functions.

RELNOTES: None.
PiperOrigin-RevId: 207035217

9 files changed, 169 insertions, 28 deletions

diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index 12c7cdd1a2..9e7d979855 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -536,6 +536,11 @@ static vector<string> GetArgumentArray(
   } else {
     result.push_back("--noexpand_configs_in_place");
   }
+  if (!globals->options->digest_function.empty()) {
+    // Only include this if a value is requested - we rely on the empty case
+    // being "null" to set the programmatic default in the server.
+    result.push_back("--digest_function=" + globals->options->digest_function);
+  }
   if (globals->options->oom_more_eagerly) {
     result.push_back("--experimental_oom_more_eagerly");
   } else {
diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc
index b2db261526..a2d26b74d3 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -92,6 +92,7 @@ StartupOptions::StartupOptions(const string &product_name,
       java_logging_formatter(
           "com.google.devtools.build.lib.util.SingleLineFormatter"),
       expand_configs_in_place(true),
+      digest_function(),
       original_startup_options_(std::vector<RcStartupFlag>()) {
   bool testing = !blaze::GetEnv("TEST_TMPDIR").empty();
   if (testing) {
@@ -137,6 +138,7 @@ StartupOptions::StartupOptions(const string &product_name,
   RegisterNullaryStartupFlag("write_command_log");
   RegisterUnaryStartupFlag("command_port");
   RegisterUnaryStartupFlag("connect_timeout_secs");
+  RegisterUnaryStartupFlag("digest_function");
   RegisterUnaryStartupFlag("experimental_oom_more_eagerly_threshold");
   // TODO(b/5568649): remove this deprecated alias for server_javabase
   RegisterUnaryStartupFlag("host_javabase");
@@ -343,6 +345,10 @@ blaze_exit_code::ExitCode StartupOptions::ProcessArg(
       return blaze_exit_code::BAD_ARGV;
     }
     option_sources["connect_timeout_secs"] = rcfile;
+  } else if ((value = GetUnaryOption(arg, next_arg, "--digest_function")) !=
+             NULL) {
+    digest_function = value;
+    option_sources["digest_function"] = rcfile;
   } else if ((value = GetUnaryOption(arg, next_arg, "--command_port")) !=
              NULL) {
     if (!blaze_util::safe_strto32(value, &command_port) ||
diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h
index af9f276e24..cca38369c3 100644
--- a/src/main/cpp/startup_options.h
+++ b/src/main/cpp/startup_options.h
@@ -298,6 +298,9 @@ class StartupOptions {
 
   bool expand_configs_in_place;
 
+  // The hash function to use when computing file digests.
+  std::string digest_function;
+
   // The startup options as received from the user and rc files, tagged with
   // their origin. This is populated by ProcessArgs.
   std::vector<RcStartupFlag> original_startup_options_;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BazelFileSystemModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BazelFileSystemModule.java
index a734424a03..9e57bf84e9 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BazelFileSystemModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BazelFileSystemModule.java
@@ -13,11 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.runtime;
 
+import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.unix.UnixFileSystem;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.DigestHashFunction.DefaultAlreadySetException;
 import com.google.devtools.build.lib.vfs.DigestHashFunction.DigestFunctionConverter;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.JavaIoFileSystem;
@@ -34,27 +36,36 @@ import com.google.devtools.common.options.OptionsProvider;
 public class BazelFileSystemModule extends BlazeModule {
 
   @Override
-  public FileSystem getFileSystem(OptionsProvider startupOptions) throws AbruptExitException {
-    final DigestHashFunction hashFunction;
-    String value = null;
+  public void globalInit(OptionsProvider startupOptionsProvider) throws AbruptExitException {
+    BlazeServerStartupOptions startupOptions =
+        Preconditions.checkNotNull(
+            startupOptionsProvider.getOptions(BlazeServerStartupOptions.class));
+    DigestHashFunction commandLineHashFunction = startupOptions.digestHashFunction;
     try {
-      value = System.getProperty("bazel.DigestFunction", "SHA256");
-      hashFunction = new DigestFunctionConverter().convert(value);
-    } catch (OptionsParsingException e) {
-      throw new AbruptExitException(
-          "The specified hash function '" + value + "' is not supported.",
-          ExitCode.COMMAND_LINE_ERROR,
-          e);
+      if (commandLineHashFunction != null) {
+        DigestHashFunction.setDefault(commandLineHashFunction);
+      } else {
+        String value = System.getProperty("bazel.DigestFunction", "SHA256");
+        DigestHashFunction jvmPropertyHash;
+        try {
+          jvmPropertyHash = new DigestFunctionConverter().convert(value);
+        } catch (OptionsParsingException e) {
+          throw new AbruptExitException(ExitCode.COMMAND_LINE_ERROR, e);
+        }
+        DigestHashFunction.setDefault(jvmPropertyHash);
+      }
+    } catch (DefaultAlreadySetException e) {
+      throw new AbruptExitException(ExitCode.BLAZE_INTERNAL_ERROR, e);
     }
+  }
+
+  @Override
+  public FileSystem getFileSystem(OptionsProvider startupOptions) {
     if ("0".equals(System.getProperty("io.bazel.EnableJni"))) {
       // Ignore UnixFileSystem, to be used for bootstrapping.
-      return OS.getCurrent() == OS.WINDOWS
-          ? new WindowsFileSystem(hashFunction)
-          : new JavaIoFileSystem(hashFunction);
+      return OS.getCurrent() == OS.WINDOWS ? new WindowsFileSystem() : new JavaIoFileSystem();
     }
     // The JNI-based UnixFileSystem is faster, but on Windows it is not available.
-    return OS.getCurrent() == OS.WINDOWS
-        ? new WindowsFileSystem(hashFunction)
-        : new UnixFileSystem(hashFunction);
+    return OS.getCurrent() == OS.WINDOWS ? new WindowsFileSystem() : new UnixFileSystem();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
index 8fdd14082d..6d434f8d56 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.runtime;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.util.OptionsUtils;
+import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.Option;
@@ -395,14 +396,28 @@ public class BlazeServerStartupOptions extends OptionsBase {
   public boolean clientDebug;
 
   @Option(
-    name = "connect_timeout_secs",
-    defaultValue = "30", // NOTE: only for documentation, value is set and used by the client.
-    documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
-    effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
-    help = "The amount of time the client waits for each attempt to connect to the server"
-  )
+      name = "connect_timeout_secs",
+      defaultValue = "30", // NOTE: only for documentation, value is set and used by the client.
+      documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
+      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+      help = "The amount of time the client waits for each attempt to connect to the server")
   public int connectTimeoutSecs;
 
+  // TODO(b/109764197): Add OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS & remove the
+  // experimental tag once this has been tested and is ready for use.
+  @Option(
+      name = "digest_function",
+      defaultValue = "null",
+      converter = DigestHashFunction.DigestFunctionConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {
+        OptionEffectTag.LOSES_INCREMENTAL_STATE,
+        OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION
+      },
+      metadataTags = OptionMetadataTag.EXPERIMENTAL,
+      help = "The hash function to use when computing file digests.")
+  public DigestHashFunction digestHashFunction;
+
   @Deprecated
   @Option(
       name = "expand_configs_in_place",
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/DigestHashFunction.java b/src/main/java/com/google/devtools/build/lib/vfs/DigestHashFunction.java
index be18f0ac92..12aee0757b 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/DigestHashFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/DigestHashFunction.java
@@ -26,13 +26,15 @@ import java.util.Map.Entry;
 /** Type of hash function to use for digesting files. */
 // The underlying HashFunctions are immutable and thread safe.
 public class DigestHashFunction {
+  // This map must be declared first to make sure that calls to register() have it ready.
   private static final HashMap<String, DigestHashFunction> hashFunctionRegistry = new HashMap<>();
 
-  public static final DigestHashFunction MD5 = DigestHashFunction.register(Hashing.md5(), "MD5");
-  public static final DigestHashFunction SHA1 =
-      DigestHashFunction.register(Hashing.sha1(), "SHA-1", "SHA1");
-  public static final DigestHashFunction SHA256 =
-      DigestHashFunction.register(Hashing.sha256(), "SHA-256", "SHA256");
+  public static final DigestHashFunction MD5 = register(Hashing.md5(), "MD5");
+  public static final DigestHashFunction SHA1 = register(Hashing.sha1(), "SHA-1", "SHA1");
+  public static final DigestHashFunction SHA256 = register(Hashing.sha256(), "SHA-256", "SHA256");
+
+  private static DigestHashFunction defaultHash;
+  private static boolean defaultHasBeenSet = false;
 
   private final HashFunction hash;
   private final String name;
@@ -81,6 +83,55 @@ public class DigestHashFunction {
     return hashFunction;
   }
 
+  /**
+   * Returns the default DigestHashFunction for this instance of Bazel.
+   *
+   * <p>Note: This is a synchronized function, to make sure it does not occur concurrently with
+   * {@link #setDefault(DigestHashFunction)}. Once this value is set, it's a constant, so to prevent
+   * blocking calls, users should cache this value if needed.
+   *
+   * @throws DefaultNotSetException if the default has not yet been set by a previous call to {@link
+   *     #setDefault}.
+   */
+  public static synchronized DigestHashFunction getDefault() throws DefaultNotSetException {
+    if (!defaultHasBeenSet) {
+      throw new DefaultNotSetException("DigestHashFunction default has not been set");
+    }
+    return defaultHash;
+  }
+
+  /** Indicates that the default has not been initialized. */
+  public static final class DefaultNotSetException extends Exception {
+    DefaultNotSetException(String message) {
+      super(message);
+    }
+  }
+
+  /**
+   * Sets the default DigestHashFunction for this instance of Bazel - can only be set once to
+   * prevent incongruities.
+   *
+   * @throws DefaultAlreadySetException if it was already set.
+   */
+  public static synchronized void setDefault(DigestHashFunction hash)
+      throws DefaultAlreadySetException {
+    if (defaultHasBeenSet) {
+      throw new DefaultAlreadySetException(
+          String.format(
+              "setDefault(%s) failed. The default has already been set to %s, you cannot reset it.",
+              hash.name, defaultHash.name));
+    }
+    defaultHash = hash;
+    defaultHasBeenSet = true;
+  }
+
+  /** Failure to set the default if the default already being set. */
+  public static final class DefaultAlreadySetException extends Exception {
+    DefaultAlreadySetException(String message) {
+      super(message);
+    }
+  }
+
   /** Converts a string to its registered {@link DigestHashFunction}. */
   public static class DigestFunctionConverter implements Converter<DigestHashFunction> {
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
index 8d10e5800b..1c3a163f62 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java
@@ -21,6 +21,7 @@ import com.google.common.collect.Lists;
 import com.google.common.io.ByteSource;
 import com.google.common.io.CharStreams;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.vfs.DigestHashFunction.DefaultNotSetException;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
@@ -39,7 +40,18 @@ public abstract class FileSystem {
   private final DigestHashFunction digestFunction;
 
   public FileSystem() {
-    this(DigestHashFunction.MD5);
+    DigestHashFunction defaultHash;
+    try {
+      defaultHash = DigestHashFunction.getDefault();
+    } catch (DefaultNotSetException e) {
+      // For now, be tolerant for cases where the default has not been set, and fallback to MD5, the
+      // old default.
+      // TODO(b/109764197): Remove this, third_party uses of this library should set their own
+      // default, and tests should either set their own default or be able to be run with multiple
+      // digest functions.
+      defaultHash = DigestHashFunction.MD5;
+    }
+    digestFunction = defaultHash;
   }
 
   public FileSystem(DigestHashFunction digestFunction) {
diff --git a/src/test/cpp/bazel_startup_options_test.cc b/src/test/cpp/bazel_startup_options_test.cc
index ff37ed4781..4f0e460693 100644
--- a/src/test/cpp/bazel_startup_options_test.cc
+++ b/src/test/cpp/bazel_startup_options_test.cc
@@ -93,6 +93,7 @@ TEST_F(BazelStartupOptionsTest, ValidStartupFlags) {
   ExpectIsUnaryOption(options, "bazelrc");
   ExpectIsUnaryOption(options, "command_port");
   ExpectIsUnaryOption(options, "connect_timeout_secs");
+  ExpectIsUnaryOption(options, "digest_function");
   ExpectIsUnaryOption(options, "experimental_oom_more_eagerly_threshold");
   ExpectIsUnaryOption(options, "host_javabase");
   ExpectIsUnaryOption(options, "server_javabase");
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/DigestHashFunctionTest.java b/src/test/java/com/google/devtools/build/lib/vfs/DigestHashFunctionTest.java
index e39b3c0a54..1e4044bd31 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/DigestHashFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/DigestHashFunctionTest.java
@@ -14,9 +14,12 @@
 package com.google.devtools.build.lib.vfs;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.hash.Hashing;
 import com.google.devtools.build.lib.vfs.DigestHashFunction.DigestFunctionConverter;
+import java.lang.reflect.Field;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -29,6 +32,20 @@ import org.junit.runners.JUnit4;
 public class DigestHashFunctionTest {
   private final DigestFunctionConverter converter = new DigestFunctionConverter();
 
+  @Before
+  public void resetStaticDefault() throws IllegalAccessException, NoSuchFieldException {
+    // The default is effectively a Singleton, and it does not allow itself to be set multiple
+    // times. In order to test this reasonably, though, we reset the sentinel boolean to false and
+    // the value to null, which are the values before setDefault is called.
+    Field defaultHasBeenSet = DigestHashFunction.class.getDeclaredField("defaultHasBeenSet");
+    defaultHasBeenSet.setAccessible(true);
+    defaultHasBeenSet.set(null, false);
+
+    Field defaultValue = DigestHashFunction.class.getDeclaredField("defaultHash");
+    defaultValue.setAccessible(true);
+    defaultValue.set(null, null);
+  }
+
   @Test
   public void convertReturnsTheSameValueAsTheConstant() throws Exception {
     assertThat(converter.convert("sha-256")).isSameAs(DigestHashFunction.SHA256);
@@ -64,4 +81,24 @@ public class DigestHashFunctionTest {
     assertThat(converter.convert("goodFastHash64"))
         .isSameAs(converter.convert("GOOD-fast-HASH-64"));
   }
+
+  @Test
+  public void unsetDefaultThrows() {
+    assertThrows(
+        DigestHashFunction.DefaultNotSetException.class, () -> DigestHashFunction.getDefault());
+  }
+
+  @Test
+  public void setDefaultDoesNotThrow() throws Exception {
+    DigestHashFunction.setDefault(DigestHashFunction.SHA1);
+    DigestHashFunction.getDefault();
+  }
+
+  @Test
+  public void cannotSetDefaultMultipleTimes() throws Exception {
+    DigestHashFunction.setDefault(DigestHashFunction.MD5);
+    assertThrows(
+        DigestHashFunction.DefaultAlreadySetException.class,
+        () -> DigestHashFunction.setDefault(DigestHashFunction.SHA1));
+  }
 }