Ignore notify events for pants_ignore patterns. (pantsbuild#9406)

* Create a git ignorer on the context object. Adjust all call sites which
create a posix fs to pass in an ignorer.

* Ignore fsevents from files that match pants_ignore patterns.

* Always pass is_dir = false to ignorer to avoid stat-ing every path the
event watch thread sees.

src/rust/engine/fs/fs_util/src/main.rs

@@ -662,7 +662,13 @@ fn expand_files_helper(
 }
 
 fn make_posix_fs<P: AsRef<Path>>(executor: task_executor::Executor, root: P) -> fs::PosixFS {
-  fs::PosixFS::new(&root, &[], executor).unwrap()
+  // Unwrapping the output of creating the git ignorer with no patterns is infallible.
+  fs::PosixFS::new(
+    &root,
+    fs::GitignoreStyleExcludes::create(&[]).unwrap(),
+    executor,
+  )
+  .unwrap()
 }
 
 fn ensure_uploaded_to_remote(

src/rust/engine/fs/src/lib.rs

@@ -131,7 +131,7 @@ pub struct GitignoreStyleExcludes {
 }
 
 impl GitignoreStyleExcludes {
-  fn create(patterns: &[String]) -> Result<Arc<Self>, String> {
+  pub fn create(patterns: &[String]) -> Result<Arc<Self>, String> {
     if patterns.is_empty() {
       return Ok(EMPTY_IGNORE.clone());
     }
@@ -171,6 +171,13 @@ impl GitignoreStyleExcludes {
       ::ignore::Match::Ignore(_) => true,
     }
   }
+
+  pub fn is_ignored_or_child_of_ignored_path(&self, path: &Path, is_dir: bool) -> bool {
+    match self.gitignore.matched_path_or_any_parents(path, is_dir) {
+      ::ignore::Match::None | ::ignore::Match::Whitelist(_) => false,
+      ::ignore::Match::Ignore(_) => true,
+    }
+  }
 }
 
 lazy_static! {
@@ -587,15 +594,15 @@ pub struct PosixFS {
 impl PosixFS {
   pub fn new<P: AsRef<Path>>(
     root: P,
-    ignore_patterns: &[String],
+    ignorer: Arc<GitignoreStyleExcludes>,
     executor: task_executor::Executor,
   ) -> Result<PosixFS, String> {
-    Self::new_with_symlink_behavior(root, ignore_patterns, executor, SymlinkBehavior::Aware)
+    Self::new_with_symlink_behavior(root, ignorer, executor, SymlinkBehavior::Aware)
   }
 
   pub fn new_with_symlink_behavior<P: AsRef<Path>>(
     root: P,
-    ignore_patterns: &[String],
+    ignorer: Arc<GitignoreStyleExcludes>,
     executor: task_executor::Executor,
     symlink_behavior: SymlinkBehavior,
   ) -> Result<PosixFS, String> {
@@ -616,15 +623,9 @@ impl PosixFS {
       })
       .map_err(|e| format!("Could not canonicalize root {:?}: {:?}", root, e))?;
 
-    let ignore = GitignoreStyleExcludes::create(&ignore_patterns).map_err(|e| {
-      format!(
-        "Could not parse build ignore inputs {:?}: {:?}",
-        ignore_patterns, e
-      )
-    })?;
     Ok(PosixFS {
       root: canonical_root,
-      ignore: ignore,
+      ignore: ignorer,
       executor: executor,
       symlink_behavior: symlink_behavior,
     })

src/rust/engine/fs/src/posixfs_tests.rs

@@ -2,8 +2,9 @@ use tempfile;
 use testutil;
 
 use crate::{
-  Dir, DirectoryListing, File, GlobExpansionConjunction, GlobMatching, Link, PathGlobs, PathStat,
-  PathStatGetter, PosixFS, Stat, StrictGlobMatching, SymlinkBehavior, VFS,
+  Dir, DirectoryListing, File, GitignoreStyleExcludes, GlobExpansionConjunction, GlobMatching,
+  Link, PathGlobs, PathStat, PathStatGetter, PosixFS, Stat, StrictGlobMatching, SymlinkBehavior,
+  VFS,
 };
 
 use async_trait::async_trait;
@@ -384,7 +385,7 @@ async fn assert_only_file_is_executable(path: &Path, want_is_executable: bool) {
 fn new_posixfs<P: AsRef<Path>>(dir: P) -> PosixFS {
   PosixFS::new(
     dir.as_ref(),
-    &[],
+    GitignoreStyleExcludes::create(&[]).unwrap(),
     task_executor::Executor::new(Handle::current()),
   )
   .unwrap()
@@ -393,7 +394,7 @@ fn new_posixfs<P: AsRef<Path>>(dir: P) -> PosixFS {
 fn new_posixfs_symlink_oblivious<P: AsRef<Path>>(dir: P) -> PosixFS {
   PosixFS::new_with_symlink_behavior(
     dir.as_ref(),
-    &[],
+    GitignoreStyleExcludes::create(&[]).unwrap(),
     task_executor::Executor::new(Handle::current()),
     SymlinkBehavior::Oblivious,
   )

src/rust/engine/fs/store/src/snapshot.rs

@@ -4,7 +4,9 @@
 use crate::Store;
 use bazel_protos;
 use boxfuture::{try_future, BoxFuture, Boxable};
-use fs::{Dir, File, GlobMatching, PathGlobs, PathStat, PosixFS, SymlinkBehavior};
+use fs::{
+  Dir, File, GitignoreStyleExcludes, GlobMatching, PathGlobs, PathStat, PosixFS, SymlinkBehavior,
+};
 use futures::future::TryFutureExt;
 use futures01::future::{self, join_all, Future};
 use hashing::{Digest, EMPTY_DIGEST};
@@ -540,7 +542,7 @@ impl Snapshot {
       .or_else(|_| {
         let posix_fs = Arc::new(try_future!(PosixFS::new_with_symlink_behavior(
           root_path,
-          &[],
+          try_future!(GitignoreStyleExcludes::create(&[])),
           executor,
           SymlinkBehavior::Oblivious
         )));

src/rust/engine/fs/store/src/snapshot_tests.rs

@@ -8,8 +8,8 @@ use tokio::runtime::Handle;
 
 use crate::{OneOffStoreFileByDigest, Snapshot, Store};
 use fs::{
-  Dir, File, GlobExpansionConjunction, GlobMatching, PathGlobs, PathStat, PosixFS,
-  StrictGlobMatching,
+  Dir, File, GitignoreStyleExcludes, GlobExpansionConjunction, GlobMatching, PathGlobs, PathStat,
+  PosixFS, StrictGlobMatching,
 };
 
 use std;
@@ -36,7 +36,8 @@ fn setup() -> (
   )
   .unwrap();
   let dir = tempfile::Builder::new().prefix("root").tempdir().unwrap();
-  let posix_fs = Arc::new(PosixFS::new(dir.path(), &[], executor).unwrap());
+  let ignorer = GitignoreStyleExcludes::create(&[]).unwrap();
+  let posix_fs = Arc::new(PosixFS::new(dir.path(), ignorer, executor).unwrap());
   let file_saver = OneOffStoreFileByDigest::new(store.clone(), posix_fs.clone());
   (store, dir, posix_fs, file_saver)
 }

src/rust/engine/process_execution/src/local.rs

@@ -382,7 +382,8 @@ pub trait CapturedWorkdir {
           future::ok(store::Snapshot::empty()).to_boxed()
         } else {
           // Use no ignore patterns, because we are looking for explicitly listed paths.
-          future::done(fs::PosixFS::new(workdir_path2, &[], executor))
+          future::done(fs::GitignoreStyleExcludes::create(&[]))
+            .and_then(|ignorer| future::done(fs::PosixFS::new(workdir_path2, ignorer, executor)))
             .map_err(|err| {
               format!(
                 "Error making posix_fs to fetch local process execution output files: {}",

src/rust/engine/src/context.rs

@@ -20,7 +20,7 @@ use crate::types::Types;
 use crate::watch::InvalidationWatcher;
 use boxfuture::{BoxFuture, Boxable};
 use core::clone::Clone;
-use fs::{safe_create_dir_all_ioerror, PosixFS};
+use fs::{safe_create_dir_all_ioerror, GitignoreStyleExcludes, PosixFS};
 use graph::{EntryId, Graph, NodeContext};
 use process_execution::{
   self, speculate::SpeculatingCommandRunner, BoundedCommandRunner, ExecuteProcessRequestMetadata,
@@ -235,12 +235,24 @@ impl Core {
       })
     }
     let graph = Arc::new(Graph::new());
-    let watcher =
-      InvalidationWatcher::new(Arc::downgrade(&graph), executor.clone(), build_root.clone())?;
 
     let http_client = reqwest::Client::new();
     let rule_graph = RuleGraph::new(tasks.as_map(), root_subject_types);
 
+    let ignorer = GitignoreStyleExcludes::create(&ignore_patterns).map_err(|e| {
+      format!(
+        "Could not parse build ignore inputs {:?}: {:?}",
+        ignore_patterns, e
+      )
+    })?;
+
+    let watcher = InvalidationWatcher::new(
+      Arc::downgrade(&graph),
+      executor.clone(),
+      build_root.clone(),
+      ignorer.clone(),
+    )?;
+
     Ok(Core {
       graph: graph,
       tasks: tasks,
@@ -253,7 +265,7 @@ impl Core {
       http_client,
       // TODO: Errors in initialization should definitely be exposed as python
       // exceptions, rather than as panics.
-      vfs: PosixFS::new(&build_root, &ignore_patterns, executor)
+      vfs: PosixFS::new(&build_root, ignorer, executor)
         .map_err(|e| format!("Could not initialize VFS: {:?}", e))?,
       build_root,
       watcher,

src/rust/engine/src/watch.rs

@@ -8,14 +8,13 @@ use std::thread;
 use std::time::Duration;
 
 use crossbeam_channel::{self, Receiver, RecvTimeoutError, TryRecvError};
-use log::{debug, error, info, warn};
-use notify::{RecommendedWatcher, RecursiveMode, Watcher};
-//use parking_lot::Mutex;
 use futures::compat::Future01CompatExt;
 use futures_locks::Mutex;
-use process_execution::PlatformConstraint;
+use log::{debug, error, info, warn};
+use notify::{RecommendedWatcher, RecursiveMode, Watcher};
 use task_executor::Executor;
 
+use fs::GitignoreStyleExcludes;
 use graph::{Graph, InvalidationResult};
 use logging;
 
@@ -42,30 +41,29 @@ pub struct InvalidationWatcher {
   watcher: Arc<Mutex<RecommendedWatcher>>,
   executor: Executor,
   liveness: Receiver<()>,
-  current_platform: PlatformConstraint,
 }
 
 impl InvalidationWatcher {
   pub fn new(
     graph: Weak<Graph<NodeKey>>,
     executor: Executor,
     build_root: PathBuf,
+    ignorer: Arc<GitignoreStyleExcludes>,
   ) -> Result<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
     // wouldn't have the build_root as a prefix, and so we would miss invalidating certain nodes.
     // We canonicalize the build_root once so this isn't a problem.
     let canonical_build_root =
       std::fs::canonicalize(build_root.as_path()).map_err(|e| format!("{:?}", e))?;
-    let current_platform = PlatformConstraint::current_platform_constraint()?;
     let (watch_sender, watch_receiver) = crossbeam_channel::unbounded();
     let mut watcher: RecommendedWatcher = Watcher::new(watch_sender, Duration::from_millis(50))
       .map_err(|e| format!("Failed to begin watching the filesystem: {}", 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 current_platform == PlatformConstraint::Darwin {
+    if cfg!(target_os = "macos") {
       watcher
         .watch(canonical_build_root.clone(), RecursiveMode::Recursive)
         .map_err(|e| {
@@ -93,26 +91,45 @@ impl InvalidationWatcher {
             let paths: HashSet<_> = ev
               .paths
               .into_iter()
-              .map(|path| {
+              .filter_map(|path| {
                 // relativize paths to build root.
-                let mut paths_to_invalidate: Vec<PathBuf> = vec![];
-                let path_relative_to_build_root = {
-                  if path.starts_with(&canonical_build_root) {
-                    path.strip_prefix(&canonical_build_root).unwrap().into()
-                  } else {
-                    path
-                  }
+                let path_relative_to_build_root = if path.starts_with(&canonical_build_root) {
+                  // Unwrapping is fine because we check that the path starts with
+                  // the build root above.
+                  path.strip_prefix(&canonical_build_root).unwrap().into()
+                } else {
+                  path
                 };
-                paths_to_invalidate.push(path_relative_to_build_root.clone());
+                // To avoid having to stat paths for events we will eventually ignore we "lie" to the ignorer
+                // to say that no path is a directory, they could be if someone chmod's or creates a dir.
+                // This maintains correctness by ensuring that at worst we have false negative events, where a directory
+                // only glob (one that ends in `/` ) was supposed to ignore a directory path, but didn't because we claimed it was a file. That
+                // directory path will be used to invalidate nodes, but won't invalidate anything because its path is somewhere
+                // out of our purview.
+                if ignorer.is_ignored_or_child_of_ignored_path(
+                  &path_relative_to_build_root,
+                  /* is_dir */ false,
+                ) {
+                  None
+                } else {
+                  Some(path_relative_to_build_root)
+                }
+              })
+              .map(|path_relative_to_build_root| {
+                let mut paths_to_invalidate: Vec<PathBuf> = vec![];
                 if let Some(parent_dir) = path_relative_to_build_root.parent() {
                   paths_to_invalidate.push(parent_dir.to_path_buf());
                 }
+                paths_to_invalidate.push(path_relative_to_build_root);
                 paths_to_invalidate
               })
               .flatten()
               .collect();
-            debug!("notify invalidating {:?} because of {:?}", paths, ev.kind);
-            InvalidationWatcher::invalidate(&graph, &paths, "notify");
+            // Only invalidate stuff if we have paths that weren't filtered out by gitignore.
+            if !paths.is_empty() {
+              debug!("notify invalidating {:?} because of {:?}", paths, ev.kind);
+              InvalidationWatcher::invalidate(&graph, &paths, "notify");
+            };
           }
           Ok(Err(err)) => {
             if let notify::ErrorKind::PathNotFound = err.kind {
@@ -139,7 +156,6 @@ impl InvalidationWatcher {
       watcher: wrapped_watcher,
       executor,
       liveness: thread_liveness_receiver,
-      current_platform,
     })
   }
 
@@ -149,7 +165,7 @@ impl InvalidationWatcher {
   pub async fn watch(&self, path: PathBuf) -> Result<(), notify::Error> {
     // Short circuit here if we are on a Darwin platform because we should be watching
     // the entire build root recursively already.
-    if self.current_platform == PlatformConstraint::Darwin {
+    if cfg!(target_os = "macos") {
       Ok(())
     } else {
       // Using a futurized mutex here because for some reason using a regular mutex