diff --git a/src/python/pants/engine/internals/native.py b/src/python/pants/engine/internals/native.py index aaaef2090e4..0a51708d31d 100644 --- a/src/python/pants/engine/internals/native.py +++ b/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) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 877b40e923a..546e8ac64be 100644 --- a/src/python/pants/option/global_options.py +++ b/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." ) diff --git a/src/rust/engine/engine_cffi/src/lib.rs b/src/rust/engine/engine_cffi/src/lib.rs index cd3911c1aea..54c69cfb9cd 100644 --- a/src/rust/engine/engine_cffi/src/lib.rs +++ b/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 { 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, ) } diff --git a/src/rust/engine/src/context.rs b/src/rust/engine/src/context.rs index c922553b1a8..335f5bf1d02 100644 --- a/src/rust/engine/src/context.rs +++ b/src/rust/engine/src/context.rs @@ -91,7 +91,6 @@ impl Core { process_execution_use_local_cache: bool, remote_execution_headers: BTreeMap, process_execution_local_enable_nailgun: bool, - experimental_fs_watcher: bool, ) -> Result { // 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 { diff --git a/src/rust/engine/watch/src/lib.rs b/src/rust/engine/watch/src/lib.rs index e2e35aa4277..1ac53d59b01 100644 --- a/src/rust/engine/watch/src/lib.rs +++ b/src/rust/engine/watch/src/lib.rs @@ -53,7 +53,6 @@ struct Inner { watcher: RecommendedWatcher, executor: Executor, liveness: Receiver, - 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, - enabled: bool, ) -> Result, 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, 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() }; diff --git a/src/rust/engine/watch/src/tests.rs b/src/rust/engine/watch/src/tests.rs index 03d9cdf456f..946da191e8e 100644 --- a/src/rust/engine/watch/src/tests.rs +++ b/src/rust/engine/watch/src/tests.rs @@ -33,7 +33,7 @@ async fn setup_watch( file_path: PathBuf, ) -> Arc { 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 diff --git a/tests/python/pants_test/init/test_options_initializer.py b/tests/python/pants_test/init/test_options_initializer.py index 5dc4e249b06..dac07160673 100644 --- a/tests/python/pants_test/init/test_options_initializer.py +++ b/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)) diff --git a/tests/python/pants_test/pantsd/test_watchman_launcher.py b/tests/python/pants_test/pantsd/test_watchman_launcher.py index 5541730782c..9894409de4d 100644 --- a/tests/python/pants_test/pantsd/test_watchman_launcher.py +++ b/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):