diff --git a/src/python/pants/engine/internals/native.py b/src/python/pants/engine/internals/native.py index aaaef2090e49..0a51708d31d0 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 877b40e923ab..e35cf6fa56ac 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, ) @@ -903,6 +900,8 @@ def register_bootstrap_options(cls, register): type=bool, default=True, advanced=True, + removal_version="1.29.0.dev2", + 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 +1062,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 1517a5503442..b37b0899e6fc 100644 --- a/src/rust/engine/engine_cffi/src/lib.rs +++ b/src/rust/engine/engine_cffi/src/lib.rs @@ -239,7 +239,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, @@ -270,7 +269,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, @@ -314,7 +312,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 @@ -426,7 +423,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 450433210a84..208354911b90 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())?; executor.block_on({ let watcher = watcher.clone(); let graph = graph.clone(); @@ -384,6 +378,7 @@ impl NodeContext for Context { entry_id: Some(entry_id), core: self.core.clone(), session: self.session.clone(), + run_id: self.run_id, } } diff --git a/src/rust/engine/watch/src/lib.rs b/src/rust/engine/watch/src/lib.rs index 10c4bb10a948..8f62a5a6a36a 100644 --- a/src/rust/engine/watch/src/lib.rs +++ b/src/rust/engine/watch/src/lib.rs @@ -54,7 +54,6 @@ struct Inner { invalidatables: Vec>, 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. @@ -75,7 +74,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 @@ -88,28 +86,27 @@ 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, invalidatables: Vec::new(), executor, liveness: liveness_receiver, - enabled, background_task_inputs: Some(( ignorer, canonical_build_root, @@ -293,13 +290,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 b18b060a1827..674634a0509b 100644 --- a/src/rust/engine/watch/src/tests.rs +++ b/src/rust/engine/watch/src/tests.rs @@ -34,7 +34,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.add_invalidatable(invalidatable).await; watcher.watch(file_path).await.unwrap(); diff --git a/tests/python/pants_test/init/test_options_initializer.py b/tests/python/pants_test/init/test_options_initializer.py index 5dc4e249b06d..dac071606734 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))