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
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelDiffAwarenessModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelDiffAwarenessModule.java
index 4140a7d..b5a85d3 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelDiffAwarenessModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelDiffAwarenessModule.java
@@ -19,6 +19,7 @@
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.
@@ -26,8 +27,12 @@
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);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/commands/FetchCommand.java b/src/main/java/com/google/devtools/build/lib/bazel/commands/FetchCommand.java
index 6f6d48a..0d78adc 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/commands/FetchCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/commands/FetchCommand.java
@@ -71,9 +71,7 @@
}
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;
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index e0f9682..9857283 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -134,8 +134,7 @@
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;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index 27e2ecc..dc67a17 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -189,9 +189,7 @@
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);
}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index a68226d..e22b09b 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/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,7 +471,7 @@
*
* @see DefaultsPackage
*/
- public void setupPackageCache(PackageCacheOptions packageCacheOptions,
+ public void setupPackageCache(OptionsClassProvider options,
String defaultsPackageContents) throws InterruptedException, AbruptExitException {
SkyframeExecutor skyframeExecutor = getSkyframeExecutor();
if (!skyframeExecutor.hasIncrementalState()) {
@@ -478,7 +479,7 @@
}
skyframeExecutor.sync(
reporter,
- packageCacheOptions,
+ options.getOptions(PackageCacheOptions.class),
getOutputBase(),
getWorkingDirectory(),
defaultsPackageContents,
@@ -486,7 +487,8 @@
// 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 @@
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();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java b/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java
index d7f0bfd..241d569 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java
+++ b/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 @@
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 @@
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 @@
/**
* 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));
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
index b5e596f..38c7d78 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
+++ b/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 @@
// 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)
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
index 6c689d2..c52a3fe 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
@@ -83,9 +83,7 @@
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;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DiffAwareness.java b/src/main/java/com/google/devtools/build/lib/skyframe/DiffAwareness.java
index f51cdfe..64c9033 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/DiffAwareness.java
+++ b/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 @@
* {@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.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManager.java
index 0287097..76f00a7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManager.java
+++ b/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 @@
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,7 +80,8 @@
* 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;
@@ -87,7 +89,7 @@
DiffAwareness diffAwareness = diffAwarenessState.diffAwareness;
View newView;
try {
- newView = diffAwareness.getCurrentView();
+ newView = diffAwareness.getCurrentView(options);
} catch (BrokenDiffAwarenessException e) {
handleBrokenDiffAwareness(eventHandler, pathEntry, e);
return BrokenProcessableModifiedFileSet.INSTANCE;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LocalDiffAwareness.java b/src/main/java/com/google/devtools/build/lib/skyframe/LocalDiffAwareness.java
index cae5cde..753f920 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/LocalDiffAwareness.java
+++ b/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 @@
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 @@
* 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 @@
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 @@
}
/**
- * 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 @@
* 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 @@
} 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()
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwareness.java b/src/main/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwareness.java
index cf0fc62..17a9413 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwareness.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwareness.java
@@ -17,7 +17,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.UnixJniLoader;
-
+import com.google.devtools.common.options.OptionsClassProvider;
import java.io.File;
import java.nio.file.Path;
@@ -25,11 +25,11 @@
* A {@link DiffAwareness} that use fsevents to watch the filesystem to use in lieu of
* {@link LocalDiffAwareness}.
*
- * <p>
- * On OS X, the local diff awareness cannot work because WatchService is dummy and do polling, which
- * is slow (https://bugs.openjdk.java.net/browse/JDK-7133447).
+ * <p>On OS X, the local diff awareness cannot work because WatchService is dummy and do polling,
+ * which is slow (https://bugs.openjdk.java.net/browse/JDK-7133447).
*/
public final class MacOSXFsEventsDiffAwareness extends LocalDiffAwareness {
+ private final double latency;
private boolean closed;
@@ -37,23 +37,15 @@
// structure).
private long nativePointer;
+ private boolean opened;
+
/**
* Watch changes on the file system under <code>watchRoot</code> with a granularity of
* <code>delay</code> seconds.
*/
MacOSXFsEventsDiffAwareness(String watchRoot, double latency) {
super(watchRoot);
- create(new String[] {watchRootPath.toAbsolutePath().toString()}, latency);
-
- // Start a thread that just contains the OS X run loop.
- new Thread(
- new Runnable() {
- @Override
- public void run() {
- MacOSXFsEventsDiffAwareness.this.run();
- }
- })
- .start();
+ this.latency = latency;
}
/**
@@ -73,19 +65,41 @@
*/
private native void run();
+ private void init() {
+ // The code below is based on the assumption that init() can never fail, which is currently the
+ // case; if you change init(), then you also need to update {@link #getCurrentView}.
+ Preconditions.checkState(!opened);
+ opened = true;
+ create(new String[] {watchRootPath.toAbsolutePath().toString()}, latency);
+ // Start a thread that just contains the OS X run loop.
+ new Thread(
+ new Runnable() {
+ @Override
+ public void run() {
+ MacOSXFsEventsDiffAwareness.this.run();
+ }
+ })
+ .start();
+ }
+
/**
* Close this watch service, this service should not be used any longer after closing.
*/
- public synchronized void close() {
- Preconditions.checkState(!closed);
- closed = true;
- doClose();
+ @Override
+ public void close() {
+ if (opened) {
+ Preconditions.checkState(!closed);
+ closed = true;
+ doClose();
+ }
}
+ private static final boolean JNI_AVAILABLE;
+
/**
* JNI code stopping the main loop and shutting down listening to FSEvents.
*/
- private synchronized native void doClose();
+ private native void doClose();
/**
* JNI code returning the list of absolute path modified since last call.
@@ -93,11 +107,36 @@
private native String[] poll();
static {
- UnixJniLoader.loadJni();
+ boolean loadJniWorked = false;
+ try {
+ UnixJniLoader.loadJni();
+ loadJniWorked = true;
+ } catch (UnsatisfiedLinkError ignored) {
+ // Unfortunately, we compile this class into the Bazel bootstrap binary, which doesn't have
+ // access to the JNI code (to simplify bootstrap). This is the quick and dirty way to
+ // hard-disable --watchfs in the bootstrap binary.
+ }
+ JNI_AVAILABLE = loadJniWorked;
}
@Override
- public synchronized View getCurrentView() throws BrokenDiffAwarenessException {
+ public View getCurrentView(OptionsClassProvider options)
+ throws BrokenDiffAwarenessException {
+ if (!JNI_AVAILABLE) {
+ return EVERYTHING_MODIFIED;
+ }
+ // See WatchServiceDiffAwareness#getCurrentView for an explanation of this logic.
+ boolean watchFs = options.getOptions(Options.class).watchFS;
+ if (watchFs && !opened) {
+ init();
+ } else if (!watchFs && opened) {
+ close();
+ throw new BrokenDiffAwarenessException("Switched off --watchfs again");
+ } else if (!opened) {
+ // The only difference with WatchServiceDiffAwareness#getCurrentView is this if; the init()
+ // call above can never fail, so we don't need to re-check the opened flag after init().
+ return EVERYTHING_MODIFIED;
+ }
Preconditions.checkState(!closed);
ImmutableSet.Builder<Path> paths = ImmutableSet.builder();
for (String path : poll()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 3b0bbb4..7a45eed 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -67,6 +67,7 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.common.options.OptionsClassProvider;
import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Collection;
@@ -266,11 +267,12 @@
String defaultsPackageContents,
UUID commandId,
Map<String, String> clientEnv,
- TimestampGranularityMonitor tsgm)
+ TimestampGranularityMonitor tsgm,
+ OptionsClassProvider options)
throws InterruptedException, AbruptExitException {
super.sync(eventHandler, packageCacheOptions, outputBase, workingDirectory,
- defaultsPackageContents, commandId, clientEnv, tsgm);
- handleDiffs(eventHandler, packageCacheOptions.checkOutputFiles);
+ defaultsPackageContents, commandId, clientEnv, tsgm, options);
+ handleDiffs(eventHandler, packageCacheOptions.checkOutputFiles, options);
}
/**
@@ -325,11 +327,12 @@
*/
@VisibleForTesting
public void handleDiffs(EventHandler eventHandler) throws InterruptedException {
- handleDiffs(eventHandler, /*checkOutputFiles=*/ false);
+ handleDiffs(eventHandler, /*checkOutputFiles=*/false, OptionsClassProvider.EMPTY);
}
- private void handleDiffs(EventHandler eventHandler, boolean checkOutputFiles)
- throws InterruptedException {
+ private void handleDiffs(
+ EventHandler eventHandler, boolean checkOutputFiles, OptionsClassProvider options)
+ throws InterruptedException {
if (lastAnalysisDiscarded) {
// Values were cleared last build, but they couldn't be deleted because they were needed for
// the execution phase. We can delete them now.
@@ -344,7 +347,7 @@
pathEntriesWithoutDiffInformation = Sets.newHashSet();
for (Path pathEntry : pkgLocator.get().getPathEntries()) {
DiffAwarenessManager.ProcessableModifiedFileSet modifiedFileSet =
- diffAwarenessManager.getDiff(eventHandler, pathEntry);
+ diffAwarenessManager.getDiff(eventHandler, pathEntry, options);
if (modifiedFileSet.getModifiedFileSet().treatEverythingAsModified()) {
pathEntriesWithoutDiffInformation.add(Pair.of(pathEntry, modifiedFileSet));
} else {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index f2bdf9c..146c468 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1673,7 +1673,8 @@
String defaultsPackageContents,
UUID commandId,
Map<String, String> clientEnv,
- TimestampGranularityMonitor tsgm)
+ TimestampGranularityMonitor tsgm,
+ OptionsClassProvider options)
throws InterruptedException, AbruptExitException {
preparePackageLoading(
createPackageLocator(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WatchServiceDiffAwareness.java b/src/main/java/com/google/devtools/build/lib/skyframe/WatchServiceDiffAwareness.java
index b6d274d..91ca5b4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WatchServiceDiffAwareness.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WatchServiceDiffAwareness.java
@@ -17,9 +17,10 @@
import com.google.common.collect.HashBiMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.util.Preconditions;
-
+import com.google.devtools.common.options.OptionsClassProvider;
import java.io.IOException;
import java.nio.file.ClosedWatchServiceException;
+import java.nio.file.FileSystems;
import java.nio.file.FileVisitResult;
import java.nio.file.Files;
import java.nio.file.LinkOption;
@@ -39,7 +40,6 @@
* two consecutive calls. Uses the standard Java WatchService, which uses 'inotify' on Linux.
*/
public final class WatchServiceDiffAwareness extends LocalDiffAwareness {
-
/**
* Bijection from WatchKey to the (absolute) Path being watched. WatchKeys don't have this
* functionality built-in so we do it ourselves.
@@ -49,13 +49,66 @@
/** Every directory is registered under this watch service. */
private WatchService watchService;
- WatchServiceDiffAwareness(String watchRoot, WatchService watchService) {
+ WatchServiceDiffAwareness(String watchRoot) {
super(watchRoot);
- this.watchService = watchService;
+ }
+
+ private void init() {
+ Preconditions.checkState(watchService == null);
+ try {
+ watchService = FileSystems.getDefault().newWatchService();
+ } catch (IOException ignored) {
+ // According to the docs, this can never happen with the default file system provider.
+ }
}
@Override
- public View getCurrentView() throws BrokenDiffAwarenessException {
+ public View getCurrentView(OptionsClassProvider options) throws BrokenDiffAwarenessException {
+ // We need to consider 4 cases for watchFs:
+ // previous view current view
+ // disabled disabled -> EVERYTHING_MODIFIED
+ // disabled enabled -> valid View (1)
+ // enabled disabled -> throw BrokenDiffAwarenessException
+ // enabled enabled -> valid View
+ //
+ // (1) When watchFs gets enabled, we need to consider both the delta from the previous view
+ // to the current view (1a), and from the current view to the next view (1b).
+ // (1a) If watchFs was previously disabled, then previous view was either EVERYTHING_MODIFIED,
+ // or we threw a BrokenDiffAwarenessException. The first is safe because comparing it to
+ // any view results in ModifiedFileSet.EVERYTHING_MODIFIED. The second is safe because
+ // the previous diff awareness gets closed and we're now in a new instance; comparisons
+ // between views with different owners always results in
+ // ModifiedFileSet.EVERYTHING_MODIFIED.
+ // (1b) On the next run, we want to see the files that were modified between the current and the
+ // next run. For that, the view we return needs to be valid; however, it's ok for it to
+ // contain files that are modified between init() and poll() below, because those are
+ // already taken into account for the current build, as we ended up with
+ // ModifiedFileSet.EVERYTHING_MODIFIED in the current build.
+ boolean watchFs = options.getOptions(Options.class).watchFS;
+ if (watchFs && watchService == null) {
+ init();
+ } else if (!watchFs && (watchService != null)) {
+ close();
+ // The contract is that throwing BrokenDiffAwarenessException prevents reuse of the same
+ // diff awareness object.
+ // Consider this sequence of builds:
+ // 1. build --watchfs // startup the listener
+ // 2. build --nowatchfs // shutdown the listener
+ // 3. build --watchfs // startup the listener
+ //
+ // In the third build, we have to be careful not to reuse information from the first build,
+ // since we don't know what changed between the second and third builds. One way to ensure
+ // that is to carefully ensure that we increment the iteration numbers on every call;
+ // LocalDiffAwareness will only return a Diff if the Views are in sequential order. The other
+ // is to not reuse the DiffAwareness object, but create a new one; the DiffAwarenessManager
+ // always assumes EVERYTHING_MODIFIED for different objects. That seems safer, so we're using
+ // that here.
+ throw new BrokenDiffAwarenessException("Switched off --watchfs again");
+ }
+ // If init() failed, then this if also applies.
+ if (watchService == null) {
+ return EVERYTHING_MODIFIED;
+ }
Set<Path> modifiedAbsolutePaths;
if (isFirstCall()) {
try {
@@ -86,10 +139,12 @@
@Override
public void close() {
- try {
- watchService.close();
- } catch (IOException ignored) {
- // Nothing we can do here.
+ if (watchService != null) {
+ try {
+ watchService.close();
+ } catch (IOException ignored) {
+ // Nothing we can do here.
+ }
}
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsClassProvider.java b/src/main/java/com/google/devtools/common/options/OptionsClassProvider.java
index 90e7d48..b28c29d 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsClassProvider.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsClassProvider.java
@@ -13,11 +13,20 @@
// limitations under the License.
package com.google.devtools.common.options;
+import javax.annotation.Nullable;
+
/**
* A read-only interface for options parser results, which only allows to query the options of
* a specific class, but not e.g. the residue any other information pertaining to the command line.
*/
public interface OptionsClassProvider {
+ public static final OptionsClassProvider EMPTY = new OptionsClassProvider() {
+ @Override @Nullable
+ public <O extends OptionsBase> O getOptions(Class<O> optionsClass) {
+ return null;
+ }
+ };
+
/**
* Returns the options instance for the given {@code optionsClass}, that is,
* the parsed options, or null if it is not among those available.
@@ -25,5 +34,5 @@
* <p>The returned options should be treated by library code as immutable and
* a provider is permitted to return the same options instance multiple times.
*/
- <O extends OptionsBase> O getOptions(Class<O> optionsClass);
+ @Nullable <O extends OptionsBase> O getOptions(Class<O> optionsClass);
}
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
index c137cd3..32e8b4b 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java
@@ -57,6 +57,7 @@
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.common.options.Options;
+import com.google.devtools.common.options.OptionsClassProvider;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.ArrayList;
@@ -411,7 +412,7 @@
private View currentView;
@Override
- public View getCurrentView() {
+ public View getCurrentView(OptionsClassProvider options) {
lastView = currentView;
currentView = new View() {};
return currentView;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManagerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManagerTest.java
index b58abc1..e2aebba 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManagerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/DiffAwarenessManagerTest.java
@@ -28,6 +28,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.common.options.OptionsClassProvider;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
@@ -42,7 +43,6 @@
*/
@RunWith(JUnit4.class)
public class DiffAwarenessManagerTest {
-
private FileSystem fs;
private Path root;
protected EventCollectionApparatus events;
@@ -67,7 +67,8 @@
assertEquals(
"Expected EVERYTHING_MODIFIED since there are no factories",
ModifiedFileSet.EVERYTHING_MODIFIED,
- manager.getDiff(events.reporter(), pathEntry).getModifiedFileSet());
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY)
+ .getModifiedFileSet());
events.assertNoWarningsOrErrors();
}
@@ -80,12 +81,12 @@
DiffAwarenessFactoryStub factory = new DiffAwarenessFactoryStub();
factory.inject(pathEntry, diffAwareness1);
DiffAwarenessManager manager = new DiffAwarenessManager(ImmutableList.of(factory));
- manager.getDiff(events.reporter(), pathEntry);
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertFalse("diffAwareness1 shouldn't have been closed yet", diffAwareness1.closed());
manager.reset();
assertTrue("diffAwareness1 should have been closed by reset", diffAwareness1.closed());
factory.inject(pathEntry, diffAwareness2);
- manager.getDiff(events.reporter(), pathEntry);
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertFalse("diffAwareness2 shouldn't have been closed yet", diffAwareness2.closed());
events.assertNoWarningsOrErrors();
}
@@ -101,21 +102,26 @@
DiffAwarenessFactoryStub factory = new DiffAwarenessFactoryStub();
factory.inject(pathEntry, diffAwareness);
DiffAwarenessManager manager = new DiffAwarenessManager(ImmutableList.of(factory));
- ProcessableModifiedFileSet firstProcessableDiff = manager.getDiff(events.reporter(), pathEntry);
+ ProcessableModifiedFileSet firstProcessableDiff =
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(
"Expected EVERYTHING_MODIFIED on first call to getDiff",
ModifiedFileSet.EVERYTHING_MODIFIED,
firstProcessableDiff.getModifiedFileSet());
firstProcessableDiff.markProcessed();
- ProcessableModifiedFileSet processableDiff1 = manager.getDiff(events.reporter(), pathEntry);
+ ProcessableModifiedFileSet processableDiff1 =
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(diff1, processableDiff1.getModifiedFileSet());
- ProcessableModifiedFileSet processableDiff2 = manager.getDiff(events.reporter(), pathEntry);
+ ProcessableModifiedFileSet processableDiff2 =
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(ModifiedFileSet.union(diff1, diff2), processableDiff2.getModifiedFileSet());
processableDiff2.markProcessed();
- ProcessableModifiedFileSet processableDiff3 = manager.getDiff(events.reporter(), pathEntry);
+ ProcessableModifiedFileSet processableDiff3 =
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(diff3, processableDiff3.getModifiedFileSet());
events.assertNoWarningsOrErrors();
- ProcessableModifiedFileSet processableDiff4 = manager.getDiff(events.reporter(), pathEntry);
+ ProcessableModifiedFileSet processableDiff4 =
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(ModifiedFileSet.EVERYTHING_MODIFIED, processableDiff4.getModifiedFileSet());
events.assertContainsWarning("error");
}
@@ -139,7 +145,8 @@
DiffAwarenessManager manager =
new DiffAwarenessManager(ImmutableList.of(factory1, factory2, factory3));
- ProcessableModifiedFileSet processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ ProcessableModifiedFileSet processableDiff =
+ manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
events.assertNoWarningsOrErrors();
assertEquals(
"Expected EVERYTHING_MODIFIED on first call to getDiff for diffAwareness1",
@@ -147,7 +154,7 @@
processableDiff.getModifiedFileSet());
processableDiff.markProcessed();
- processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ processableDiff = manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
events.assertContainsEventWithFrequency("error in getCurrentView", 1);
assertEquals(
"Expected EVERYTHING_MODIFIED because of broken getCurrentView",
@@ -156,18 +163,18 @@
processableDiff.markProcessed();
factory1.remove(pathEntry);
- processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ processableDiff = manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(
"Expected EVERYTHING_MODIFIED on first call to getDiff for diffAwareness2",
ModifiedFileSet.EVERYTHING_MODIFIED,
processableDiff.getModifiedFileSet());
processableDiff.markProcessed();
- processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ processableDiff = manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(diff2, processableDiff.getModifiedFileSet());
processableDiff.markProcessed();
- processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ processableDiff = manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
events.assertContainsEventWithFrequency("error in getDiff", 1);
assertEquals(
"Expected EVERYTHING_MODIFIED because of broken getDiff",
@@ -176,14 +183,14 @@
processableDiff.markProcessed();
factory2.remove(pathEntry);
- processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ processableDiff = manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(
"Expected EVERYTHING_MODIFIED on first call to getDiff for diffAwareness3",
ModifiedFileSet.EVERYTHING_MODIFIED,
processableDiff.getModifiedFileSet());
processableDiff.markProcessed();
- processableDiff = manager.getDiff(events.reporter(), pathEntry);
+ processableDiff = manager.getDiff(events.reporter(), pathEntry, OptionsClassProvider.EMPTY);
assertEquals(diff3, processableDiff.getModifiedFileSet());
processableDiff.markProcessed();
}
@@ -235,7 +242,7 @@
}
@Override
- public View getCurrentView() throws BrokenDiffAwarenessException {
+ public View getCurrentView(OptionsClassProvider options) throws BrokenDiffAwarenessException {
if (curSequenceNum == brokenViewNum) {
throw new BrokenDiffAwarenessException("error in getCurrentView");
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwarenessTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwarenessTest.java
index 490c002..a76184c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwarenessTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/MacOSXFsEventsDiffAwarenessTest.java
@@ -18,7 +18,10 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.skyframe.DiffAwareness.View;
+import com.google.devtools.build.lib.skyframe.LocalDiffAwareness.Options;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.OptionsClassProvider;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.FileVisitResult;
@@ -57,11 +60,15 @@
private MacOSXFsEventsDiffAwareness underTest;
private Path watchedPath;
+ private OptionsClassProvider watchFsEnabledProvider;
@Before
public void setUp() throws Exception {
watchedPath = com.google.common.io.Files.createTempDir().getCanonicalFile().toPath();
underTest = new MacOSXFsEventsDiffAwareness(watchedPath.toString());
+ LocalDiffAwareness.Options localDiffOptions = new LocalDiffAwareness.Options();
+ localDiffOptions.watchFS = true;
+ watchFsEnabledProvider = new LocalDiffAwarenessOptionsProvider(localDiffOptions);
}
@After
@@ -100,16 +107,35 @@
@Test
public void testSimple() throws Exception {
- View view1 = underTest.getCurrentView();
+ View view1 = underTest.getCurrentView(watchFsEnabledProvider);
scratchFile("a/b/c");
scratchFile("b/c/d");
Thread.sleep(200); // Wait until the events propagate
- View view2 = underTest.getCurrentView();
+ View view2 = underTest.getCurrentView(watchFsEnabledProvider);
assertDiff(view1, view2, "a", "a/b", "a/b/c", "b", "b/c", "b/c/d");
rmdirs(watchedPath.resolve("a"));
rmdirs(watchedPath.resolve("b"));
Thread.sleep(200); // Wait until the events propagate
- View view3 = underTest.getCurrentView();
+ View view3 = underTest.getCurrentView(watchFsEnabledProvider);
assertDiff(view2, view3, "a", "a/b", "a/b/c", "b", "b/c", "b/c/d");
}
+
+ /**
+ * Only returns a fixed options class for {@link LocalDiffAwareness.Options}.
+ */
+ private static final class LocalDiffAwarenessOptionsProvider implements OptionsClassProvider {
+ private final Options localDiffOptions;
+
+ private LocalDiffAwarenessOptionsProvider(Options localDiffOptions) {
+ this.localDiffOptions = localDiffOptions;
+ }
+
+ @Override
+ public <O extends OptionsBase> O getOptions(Class<O> optionsClass) {
+ if (optionsClass.equals(LocalDiffAwareness.Options.class)) {
+ return optionsClass.cast(localDiffOptions);
+ }
+ return null;
+ }
+ }
}
