Disable watchman by default. (#9714)

### Problem

Using the notify crate for file invalidation is now feature complete, and so we are using two file watching implementations simultaneously under `pantsd`.

### Solution

Disable watchman by default, and deprecate it. Additionally, remove the implementation of the `experimental` flag from notify. Fixes #8710, fixes #8198, fixes #7375, fixes #5487, fixes #4999, fixes #3479.

src/python/pants/engine/internals/native.py

@@ -1003,7 +1003,6 @@ def ti(type_obj):
             execution_options.process_execution_use_local_cache,
             self.context.utf8_dict(execution_options.remote_execution_headers),
             execution_options.process_execution_local_enable_nailgun,
-            execution_options.experimental_fs_watcher,
         )
         if scheduler_result.is_throw:
             value = self.context.from_value(scheduler_result.throw_handle)

src/python/pants/option/global_options.py

@@ -102,7 +102,6 @@ class ExecutionOptions:
     remote_execution_extra_platform_properties: Any
     remote_execution_headers: Any
     process_execution_local_enable_nailgun: bool
-    experimental_fs_watcher: bool
 
     @classmethod
     def from_bootstrap_options(cls, bootstrap_options):
@@ -128,7 +127,6 @@ def from_bootstrap_options(cls, bootstrap_options):
             remote_execution_extra_platform_properties=bootstrap_options.remote_execution_extra_platform_properties,
             remote_execution_headers=bootstrap_options.remote_execution_headers,
             process_execution_local_enable_nailgun=bootstrap_options.process_execution_local_enable_nailgun,
-            experimental_fs_watcher=bootstrap_options.experimental_fs_watcher,
         )
 
 
@@ -154,7 +152,6 @@ def from_bootstrap_options(cls, bootstrap_options):
     remote_execution_extra_platform_properties=[],
     remote_execution_headers={},
     process_execution_local_enable_nailgun=False,
-    experimental_fs_watcher=True,
 )
 
 
@@ -665,9 +662,11 @@ def register_bootstrap_options(cls, register):
             "--watchman-enable",
             type=bool,
             advanced=True,
-            default=True,
+            default=False,
+            removal_version="1.30.0.dev0",
+            removal_hint="The native watcher is now sufficient to monitor for filesystem changes.",
             help="Use the watchman daemon filesystem event watcher to watch for changes "
-            "in the buildroot. Disable this to rely solely on the experimental pants engine filesystem watcher.",
+            "in the buildroot in addition to the built in watcher.",
         )
         register(
             "--watchman-version", advanced=True, default="4.9.0-pants1", help="Watchman version."
@@ -903,6 +902,8 @@ def register_bootstrap_options(cls, register):
             type=bool,
             default=True,
             advanced=True,
+            removal_version="1.30.0.dev0",
+            removal_hint="Enabled by default: flag is disabled.",
             help="Whether to use the engine filesystem watcher which registers the workspace"
             " for kernel file change events",
         )
@@ -1063,6 +1064,8 @@ def validate_instance(cls, opts):
         Raises pants.option.errors.OptionsError on validation failure.
         """
         if opts.get("loop") and not opts.enable_pantsd:
+            # TODO: This remains the case today because there are two spots that
+            # call `run_goal_rules`: fixing in a followup.
             raise OptionsError(
                 "The `--loop` option requires `--enable-pantsd`, in order to watch files."
             )

src/rust/engine/engine_cffi/src/lib.rs

@@ -240,7 +240,6 @@ pub extern "C" fn scheduler_create(
   process_execution_use_local_cache: bool,
   remote_execution_headers_buf: BufferBuffer,
   process_execution_local_enable_nailgun: bool,
-  experimental_fs_watcher: bool,
 ) -> RawResult {
   match make_core(
     tasks_ptr,
@@ -271,7 +270,6 @@ pub extern "C" fn scheduler_create(
     process_execution_use_local_cache,
     remote_execution_headers_buf,
     process_execution_local_enable_nailgun,
-    experimental_fs_watcher,
   ) {
     Ok(core) => RawResult {
       is_throw: false,
@@ -315,7 +313,6 @@ fn make_core(
   process_execution_use_local_cache: bool,
   remote_execution_headers_buf: BufferBuffer,
   process_execution_local_enable_nailgun: bool,
-  experimental_fs_watcher: bool,
 ) -> Result<Core, String> {
   let root_type_ids = root_type_ids.to_vec();
   let ignore_patterns = ignore_patterns_buf
@@ -427,7 +424,6 @@ fn make_core(
     process_execution_use_local_cache,
     remote_execution_headers,
     process_execution_local_enable_nailgun,
-    experimental_fs_watcher,
   )
 }
 

src/rust/engine/src/context.rs

@@ -91,7 +91,6 @@ impl Core {
     process_execution_use_local_cache: bool,
     remote_execution_headers: BTreeMap<String, String>,
     process_execution_local_enable_nailgun: bool,
-    experimental_fs_watcher: bool,
   ) -> Result<Core, String> {
     // Randomize CAS address order to avoid thundering herds from common config.
     let mut remote_store_servers = remote_store_servers;
@@ -266,12 +265,7 @@ impl Core {
       GitignoreStyleExcludes::create_with_gitignore_file(ignore_patterns, gitignore_file)
         .map_err(|e| format!("Could not parse build ignore patterns: {:?}", e))?;
 
-    let watcher = InvalidationWatcher::new(
-      executor.clone(),
-      build_root.clone(),
-      ignorer.clone(),
-      experimental_fs_watcher,
-    )?;
+    let watcher = InvalidationWatcher::new(executor.clone(), build_root.clone(), ignorer.clone())?;
     watcher.start(&graph);
 
     Ok(Core {

src/rust/engine/watch/src/lib.rs

@@ -53,7 +53,6 @@ struct Inner {
   watcher: RecommendedWatcher,
   executor: Executor,
   liveness: Receiver<String>,
-  enabled: bool,
   // Until the background task has started, contains the relevant inputs to launch it via
   // start_background_thread. The decoupling of creating the `InvalidationWatcher` and starting it
   // is to allow for testing of the background thread.
@@ -74,7 +73,6 @@ impl InvalidationWatcher {
     executor: Executor,
     build_root: PathBuf,
     ignorer: Arc<GitignoreStyleExcludes>,
-    enabled: bool,
   ) -> Result<Arc<InvalidationWatcher>, String> {
     // Inotify events contain canonical paths to the files being watched.
     // If the build_root contains a symlink the paths returned in notify events
@@ -87,28 +85,26 @@ impl InvalidationWatcher {
       .map_err(|e| format!("Failed to begin watching the filesystem: {}", e))?;
 
     let (liveness_sender, liveness_receiver) = crossbeam_channel::unbounded();
-    if enabled {
-      // On darwin the notify API is much more efficient if you watch the build root
-      // recursively, so we set up that watch here and then return early when watch() is
-      // called by nodes that are running. On Linux the notify crate handles adding paths to watch
-      // much more efficiently so we do that instead on Linux.
-      if cfg!(target_os = "macos") {
-        watcher
-          .watch(canonical_build_root.clone(), RecursiveMode::Recursive)
-          .map_err(|e| {
-            format!(
-              "Failed to begin recursively watching files in the build root: {}",
-              e
-            )
-          })?
-      }
+
+    // On darwin the notify API is much more efficient if you watch the build root
+    // recursively, so we set up that watch here and then return early when watch() is
+    // called by nodes that are running. On Linux the notify crate handles adding paths to watch
+    // much more efficiently so we do that instead on Linux.
+    if cfg!(target_os = "macos") {
+      watcher
+        .watch(canonical_build_root.clone(), RecursiveMode::Recursive)
+        .map_err(|e| {
+          format!(
+            "Failed to begin recursively watching files in the build root: {}",
+            e
+          )
+        })?
     }
 
     Ok(Arc::new(InvalidationWatcher(Mutex::new(Inner {
       watcher,
       executor,
       liveness: liveness_receiver,
-      enabled,
       background_task_inputs: Some((
         ignorer,
         canonical_build_root,
@@ -254,13 +250,14 @@ impl InvalidationWatcher {
   /// Add a path to the set of paths being watched by this invalidation watcher, non-recursively.
   ///
   pub async fn watch(self: &Arc<Self>, path: PathBuf) -> Result<(), notify::Error> {
+    if cfg!(target_os = "macos") {
+      // Short circuit here if we are on a Darwin platform because we should be watching
+      // the entire build root recursively already.
+      return Ok(());
+    }
+
     let executor = {
       let inner = self.0.lock();
-      if cfg!(target_os = "macos") || !inner.enabled {
-        // Short circuit here if we are on a Darwin platform because we should be watching
-        // the entire build root recursively already, or if we are not enabled.
-        return Ok(());
-      }
       inner.executor.clone()
     };
 

src/rust/engine/watch/src/tests.rs

@@ -33,7 +33,7 @@ async fn setup_watch(
   file_path: PathBuf,
 ) -> Arc<InvalidationWatcher> {
   let executor = Executor::new(Handle::current());
-  let watcher = InvalidationWatcher::new(executor, build_root, ignorer, /*enabled*/ true)
+  let watcher = InvalidationWatcher::new(executor, build_root, ignorer)
     .expect("Couldn't create InvalidationWatcher");
   watcher.watch(file_path).await.unwrap();
   watcher

tests/python/pants_test/init/test_options_initializer.py

@@ -22,16 +22,9 @@ def test_invalid_version(self):
     def test_global_options_validation(self):
         # Specify an invalid combination of options.
         ob = OptionsBootstrapper.create(
-            args=[
-                "--backend-packages=[]",
-                "--backend-packages2=[]",
-                "--v2",
-                "--no-v1",
-                "--loop",
-                "--no-enable-pantsd",
-            ]
+            args=["--backend-packages=[]", "--backend-packages2=[]", "--remote-execution",]
         )
         build_config = BuildConfigInitializer.get(ob)
         with self.assertRaises(OptionsError) as exc:
             OptionsInitializer.create(ob, build_config)
-        self.assertIn("The `--loop` option requires `--enable-pantsd`", str(exc.exception))
+        self.assertIn("The `--remote-execution` option requires", str(exc.exception))

tests/python/pants_test/pantsd/test_watchman_launcher.py

@@ -10,7 +10,8 @@
 
 class TestWatchmanLauncher(TestBase):
     def watchman_launcher(self, cli_options=()):
-        bootstrap_options = self.get_bootstrap_options(cli_options)
+        options = ("--watchman-enable", *cli_options)
+        bootstrap_options = self.get_bootstrap_options(options)
         return WatchmanLauncher.create(bootstrap_options)
 
     def create_mock_watchman(self, is_alive):