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/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;
+ }
+ }
}
