Make --watchfs a common command option.

Adding an options parameter to DiffAwareness#getCurrentView seems like the
simplest way to achieve that.

Alternatives considered:
1. Making the diff awareness modules stateful. However, I did not want to do so
as I've also been working on improving the module API to reduce state, or at
least to have a proper lifecycle management for any necessary state.

2. Making the watchFs flag a constructor parameter. However, that would also
invalidate any implementations that don't use the flag (of which we have
several).

3. Only passing in a single boolean flag instead of an options class provider;
however, this is a more principled, futureproof API, which allows other
modules / awareness implementations to use their own options.

RELNOTES: --watchfs is now a command option; the startup option of the same
    name is deprecated. I.e., use bazel build --watchfs, not blaze --watchfs
    build.

--
MOS_MIGRATED_REVID=136154395

Author: ulfjack

src/main/java/com/google/devtools/build/lib/bazel/BazelDiffAwarenessModule.java

@@ -19,15 +19,20 @@
 import com.google.devtools.build.lib.runtime.WorkspaceBuilder;
 import com.google.devtools.build.lib.skyframe.DiffAwareness;
 import com.google.devtools.build.lib.skyframe.LocalDiffAwareness;
+import com.google.devtools.common.options.OptionsBase;
 
 /**
  * Provides the {@link DiffAwareness} implementation that uses the Java watch service.
  */
 public class BazelDiffAwarenessModule extends BlazeModule {
   @Override
   public void workspaceInit(BlazeDirectories directories, WorkspaceBuilder builder) {
-    if (builder.enableWatchFs()) {
-      builder.addDiffAwarenessFactory(new LocalDiffAwareness.Factory(ImmutableList.<String>of()));
-    }
+    // Order here is important - LocalDiffAwareness creation always succeeds, so it must be last.
+    builder.addDiffAwarenessFactory(new LocalDiffAwareness.Factory(ImmutableList.<String>of()));
+  }
+
+  @Override
+  public Iterable<Class<? extends OptionsBase>> getCommonCommandOptions() {
+    return ImmutableList.<Class<? extends OptionsBase>>of(LocalDiffAwareness.Options.class);
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/commands/FetchCommand.java

@@ -71,9 +71,7 @@ public ExitCode exec(CommandEnvironment env, OptionsProvider options) {
     }
 
     try {
-      env.setupPackageCache(
-          options.getOptions(PackageCacheOptions.class),
-          runtime.getDefaultsPackageContent());
+      env.setupPackageCache(options, runtime.getDefaultsPackageContent());
     } catch (InterruptedException e) {
       env.getReporter().handle(Event.error("fetch interrupted"));
       return ExitCode.INTERRUPTED;

src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java

@@ -134,8 +134,7 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat
     validateOptions(request);
     BuildOptions buildOptions = runtime.createBuildOptions(request);
     // Sync the package manager before sending the BuildStartingEvent in runLoadingPhase()
-    env.setupPackageCache(request.getPackageCacheOptions(),
-        DefaultsPackage.getDefaultsPackageContent(buildOptions));
+    env.setupPackageCache(request, DefaultsPackage.getDefaultsPackageContent(buildOptions));
 
     ExecutionTool executionTool = null;
     LoadingResult loadingResult = null;

src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java

@@ -189,9 +189,7 @@ private BlazeRuntime(
   public void initWorkspace(BlazeDirectories directories, BinTools binTools)
       throws AbruptExitException {
     Preconditions.checkState(this.workspace == null);
-    boolean watchFS = startupOptionsProvider != null
-        && startupOptionsProvider.getOptions(BlazeServerStartupOptions.class).watchFS;
-    WorkspaceBuilder builder = new WorkspaceBuilder(directories, binTools, watchFS);
+    WorkspaceBuilder builder = new WorkspaceBuilder(directories, binTools);
     for (BlazeModule module : blazeModules) {
       module.workspaceInit(directories, builder);
     }

src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java

@@ -51,6 +51,7 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.OptionPriority;
+import com.google.devtools.common.options.OptionsClassProvider;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsProvider;
@@ -470,23 +471,24 @@ public void throwPendingException() throws AbruptExitException {
    *
    * @see DefaultsPackage
    */
-  public void setupPackageCache(PackageCacheOptions packageCacheOptions,
+  public void setupPackageCache(OptionsClassProvider options,
       String defaultsPackageContents) throws InterruptedException, AbruptExitException {
     SkyframeExecutor skyframeExecutor = getSkyframeExecutor();
     if (!skyframeExecutor.hasIncrementalState()) {
       skyframeExecutor.resetEvaluator();
     }
     skyframeExecutor.sync(
         reporter,
-        packageCacheOptions,
+        options.getOptions(PackageCacheOptions.class),
         getOutputBase(),
         getWorkingDirectory(),
         defaultsPackageContents,
         getCommandId(),
         // TODO(bazel-team): this optimization disallows rule-specified additional dependencies
         // on the client environment!
         getWhitelistedClientEnv(),
-        timestampGranularityMonitor);
+        timestampGranularityMonitor,
+        options);
   }
 
   public void recordLastExecutionTime() {
@@ -516,6 +518,15 @@ void beforeCommand(Command command, OptionsParser optionsParser,
       CommonCommandOptions options, long execStartTimeNanos, long waitTimeInMs)
       throws AbruptExitException {
     commandStartTime -= options.startupTime;
+    if (runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).watchFS) {
+      try {
+        // TODO(ulfjack): Get rid of the startup option and drop this code.
+        optionsParser.parse("--watchfs");
+      } catch (OptionsParsingException e) {
+        // This should never happen.
+        throw new IllegalStateException(e);
+      }
+    }
 
     eventBus.post(new GotOptionsEvent(runtime.getStartupOptionsProvider(), optionsParser));
     throwPendingException();

src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java

@@ -36,7 +36,6 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
-
 import java.util.Map;
 
 /**
@@ -46,7 +45,6 @@
 public final class WorkspaceBuilder {
   private final BlazeDirectories directories;
   private final BinTools binTools;
-  private final boolean watchFs;
 
   private SkyframeExecutorFactory skyframeExecutorFactory;
   private WorkspaceStatusAction.Factory workspaceStatusActionFactory;
@@ -63,10 +61,9 @@ public final class WorkspaceBuilder {
   private final ImmutableList.Builder<SkyValueDirtinessChecker> customDirtinessCheckers =
       ImmutableList.builder();
 
-  WorkspaceBuilder(BlazeDirectories directories, BinTools binTools, boolean watchFs) {
+  WorkspaceBuilder(BlazeDirectories directories, BinTools binTools) {
     this.directories = directories;
     this.binTools = binTools;
-    this.watchFs = watchFs;
   }
 
   BlazeWorkspace build(
@@ -104,10 +101,6 @@ BlazeWorkspace build(
         workspaceStatusActionFactory, binTools);
   }
 
-  public boolean enableWatchFs() {
-    return watchFs;
-  }
-
   /**
    * Sets a factory for creating {@link SkyframeExecutor} objects. Note that only one factory per
    * workspace is allowed.
@@ -136,7 +129,9 @@ public WorkspaceBuilder setWorkspaceStatusActionFactory(
 
   /**
    * Add a {@link DiffAwareness} factory. These will be used to determine which files, if any,
-   * changed between Blaze commands.
+   * changed between Blaze commands. Note that these factories are attempted in the order in which
+   * they are added to this class, so order matters - in order to guarantee a specific order, only
+   * a single module should add such factories.
    */
   public WorkspaceBuilder addDiffAwarenessFactory(DiffAwareness.Factory factory) {
     this.diffAwarenessFactories.add(Preconditions.checkNotNull(factory));

src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java

@@ -20,7 +20,6 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.runtime.BlazeCommand;
 import com.google.devtools.build.lib.runtime.BlazeRuntime;
 import com.google.devtools.build.lib.runtime.Command;
@@ -114,8 +113,7 @@ public BuildConfiguration get() {
           // package path. Since info inherits all the build options, all the necessary information
           // is available here.
           env.setupPackageCache(
-              optionsProvider.getOptions(PackageCacheOptions.class),
-              runtime.getDefaultsPackageContent(optionsProvider));
+              optionsProvider, runtime.getDefaultsPackageContent(optionsProvider));
           // TODO(bazel-team): What if there are multiple configurations? [multi-config]
           configuration = env
               .getConfigurations(optionsProvider)

src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java

@@ -83,9 +83,7 @@ public ExitCode exec(CommandEnvironment env, OptionsProvider options) {
     QueryOptions queryOptions = options.getOptions(QueryOptions.class);
 
     try {
-      env.setupPackageCache(
-          options.getOptions(PackageCacheOptions.class),
-          runtime.getDefaultsPackageContent());
+      env.setupPackageCache(options, runtime.getDefaultsPackageContent());
     } catch (InterruptedException e) {
       env.getReporter().handle(Event.error("query interrupted"));
       return ExitCode.INTERRUPTED;

src/main/java/com/google/devtools/build/lib/skyframe/DiffAwareness.java

@@ -15,9 +15,8 @@
 
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Path;
-
+import com.google.devtools.common.options.OptionsClassProvider;
 import java.io.Closeable;
-
 import javax.annotation.Nullable;
 
 /**
@@ -55,7 +54,7 @@ interface View {
    *     {@link DiffAwareness} instance. The {@link DiffAwareness} is expected to close itself in
    *     this case.
    */
-  View getCurrentView() throws BrokenDiffAwarenessException;
+  View getCurrentView(OptionsClassProvider options) throws BrokenDiffAwarenessException;
 
   /**
    * Returns the set of files of interest that have been modified between the given two views.

src/main/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManager.java

@@ -13,17 +13,16 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.skyframe.DiffAwareness.View;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Path;
-
+import com.google.devtools.common.options.OptionsClassProvider;
 import java.util.Map;
 import java.util.logging.Logger;
-
 import javax.annotation.Nullable;
 
 /**
@@ -34,11 +33,13 @@ public final class DiffAwarenessManager {
 
   private static final Logger LOG = Logger.getLogger(DiffAwarenessManager.class.getName());
 
-  private final ImmutableSet<? extends DiffAwareness.Factory> diffAwarenessFactories;
+  // The manager attempts to instantiate these in the order in which they are passed to the
+  // constructor; this is critical in the case where a factory always succeeds.
+  private final ImmutableList<? extends DiffAwareness.Factory> diffAwarenessFactories;
   private Map<Path, DiffAwarenessState> currentDiffAwarenessStates = Maps.newHashMap();
 
   public DiffAwarenessManager(Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories) {
-    this.diffAwarenessFactories = ImmutableSet.copyOf(diffAwarenessFactories);
+    this.diffAwarenessFactories = ImmutableList.copyOf(diffAwarenessFactories);
   }
 
   private static class DiffAwarenessState {
@@ -79,15 +80,16 @@ public interface ProcessableModifiedFileSet {
    * Gets the set of changed files since the last call with this path entry, or
    * {@code ModifiedFileSet.EVERYTHING_MODIFIED} if this is the first such call.
    */
-  public ProcessableModifiedFileSet getDiff(EventHandler eventHandler, Path pathEntry) {
+  public ProcessableModifiedFileSet getDiff(
+      EventHandler eventHandler, Path pathEntry, OptionsClassProvider options) {
     DiffAwarenessState diffAwarenessState = maybeGetDiffAwarenessState(pathEntry);
     if (diffAwarenessState == null) {
       return BrokenProcessableModifiedFileSet.INSTANCE;
     }
     DiffAwareness diffAwareness = diffAwarenessState.diffAwareness;
     View newView;
     try {
-      newView = diffAwareness.getCurrentView();
+      newView = diffAwareness.getCurrentView(options);
     } catch (BrokenDiffAwarenessException e) {
       handleBrokenDiffAwareness(eventHandler, pathEntry, e);
       return BrokenProcessableModifiedFileSet.INSTANCE;

src/main/java/com/google/devtools/build/lib/skyframe/LocalDiffAwareness.java

@@ -14,18 +14,20 @@
 
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
+import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionsBase;
 import java.io.IOException;
 import java.nio.file.FileSystems;
 import java.nio.file.Path;
-import java.nio.file.WatchService;
 import java.util.Set;
 
 /**
@@ -38,6 +40,19 @@
  * {@link WatchServiceDiffAwareness}.
  */
 public abstract class LocalDiffAwareness implements DiffAwareness {
+  /**
+   * Option to enable / disable local diff awareness.
+   */
+  public static final class Options extends OptionsBase {
+    @Option(
+        name = "watchfs",
+        defaultValue = "false",
+        category = "server startup",
+        help = "If true, %{product} tries to use the operating system's file watch service for "
+            + "local changes instead of scanning every file for a change."
+    )
+    public boolean watchFS;
+  }
 
   /** Factory for creating {@link LocalDiffAwareness} instances. */
   public static class Factory implements DiffAwareness.Factory {
@@ -73,16 +88,26 @@ public DiffAwareness maybeCreate(com.google.devtools.build.lib.vfs.Path pathEntr
         return new MacOSXFsEventsDiffAwareness(resolvedPathEntryFragment.toString());
       }
 
-      WatchService watchService;
-      try {
-        watchService = FileSystems.getDefault().newWatchService();
-      } catch (IOException e) {
-        return null;
-      }
-      return new WatchServiceDiffAwareness(resolvedPathEntryFragment.toString(), watchService);
+      return new WatchServiceDiffAwareness(resolvedPathEntryFragment.toString());
     }
   }
 
+  /**
+   * A view that results in any subsequent getDiff calls returning
+   * {@link ModifiedFileSet#EVERYTHING_MODIFIED}. Use this if --watchFs is disabled.
+   *
+   * <p>The position is set to -2 in order for {@link #areInSequence} below to always return false
+   * if this view is passed to it. Any negative number would work; we don't use -1 as the other
+   * view may have a position of 0.
+   */
+  protected static final View EVERYTHING_MODIFIED =
+      new SequentialView(/*owner=*/null, /*position=*/-2, ImmutableSet.<Path>of());
+
+  public static boolean areInSequence(SequentialView oldView, SequentialView newView) {
+    // Keep this in sync with the EVERYTHING_MODIFIED View above.
+    return oldView.owner == newView.owner && (oldView.position + 1) == newView.position;
+  }
+
   private int numGetCurrentViewCalls = 0;
 
   /** Root directory to watch. This is an absolute path. */
@@ -96,7 +121,8 @@ protected LocalDiffAwareness(String watchRoot) {
    * The WatchService is inherently sequential and side-effectful, so we enforce this by only
    * supporting {@link #getDiff} calls that happen to be sequential.
    */
-  private static class SequentialView implements DiffAwareness.View {
+  @VisibleForTesting
+  static class SequentialView implements DiffAwareness.View {
     private final LocalDiffAwareness owner;
     private final int position;
     private final Set<Path> modifiedAbsolutePaths;
@@ -107,10 +133,6 @@ public SequentialView(LocalDiffAwareness owner, int position, Set<Path> modified
       this.modifiedAbsolutePaths = modifiedAbsolutePaths;
     }
 
-    public static boolean areInSequence(SequentialView oldView, SequentialView newView) {
-      return oldView.owner == newView.owner && (oldView.position + 1) == newView.position;
-    }
-
     @Override
     public String toString() {
       return String.format("SequentialView[owner=%s, position=%d, modifiedAbsolutePaths=%s]", owner,
@@ -119,7 +141,7 @@ public String toString() {
   }
 
   /**
-   * Returns true on any call before first call to {@link #newView(Set<Path>)}.
+   * Returns true on any call before first call to {@link #newView}.
    */
   protected boolean isFirstCall() {
     return numGetCurrentViewCalls == 0;
@@ -129,8 +151,7 @@ protected boolean isFirstCall() {
    * Create a new views using a list of modified absolute paths. This will increase the view
    * counter.
    */
-  protected SequentialView newView(Set<Path> modifiedAbsolutePaths)
-      throws BrokenDiffAwarenessException {
+  protected SequentialView newView(Set<Path> modifiedAbsolutePaths) {
     numGetCurrentViewCalls++;
     return new SequentialView(this, numGetCurrentViewCalls, modifiedAbsolutePaths);
   }
@@ -146,7 +167,7 @@ public ModifiedFileSet getDiff(View oldView, View newView)
     } catch (ClassCastException e) {
       throw new IncompatibleViewException("Given views are not from LocalDiffAwareness");
     }
-    if (!SequentialView.areInSequence(oldSequentialView, newSequentialView)) {
+    if (!areInSequence(oldSequentialView, newSequentialView)) {
       return ModifiedFileSet.EVERYTHING_MODIFIED;
     }
     return ModifiedFileSet.builder()