Add --pants-use-gitignore option #9310

gshuflin · 2020-03-16T08:55:23Z

Problem

Cf. issue #5682, we would like pants to pay attention to a top-level .gitignore filee in a pants-controlled repository if the user passes in a --pants-gitignore flag.

Solution

Pants already uses the Rust ignore crate to handle explicit ignore strings passed in the command line, so we can modify this code to also read .gitignore files when we try to do a file access, read the lines contained therein, and use that to decide whether a path should be ignored or not.

stuhood

Thanks Greg! As discussed offline, this would be significantly simplified by only supporting gitignore files at the root of the repository, and I think that that would be a reasonable restriction for the near future.

stuhood · 2020-03-17T03:24:07Z

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

+      .into_iter()
+      .map(|(file_text, path)| {
+        let mut builder = GitignoreBuilder::new(&root);
+        for line in file_text.lines() {


Can use the GitignoreBuilder::add method, which adds the entire content of a .gitignore file: https://docs.rs/ignore/0.4.12/ignore/gitignore/struct.GitignoreBuilder.html#method.add

gshuflin · 2020-03-17T08:54:14Z

Thanks Greg! As discussed offline, this would be significantly simplified by only supporting gitignore files at the root of the repository, and I think that that would be a reasonable restriction for the near future.

Yup, that probably would've been simpler :) That said, toolchain cares about supporting more complicated .gitignore setups than just a single .gitignore in the root, and as of this update to the PR I believe I have a commit that does the right things with nested .gitignore files.

Eric-Arellano

Neat!

We don't yet have a way to override a gitignore, do we? I wonder if there's a use case for "I want to use 99% of my gitignore files, except for this one thing." Probably premature optimization to handle that now, so long as we have a path forward to add this in the future.

Eric-Arellano · 2020-03-18T20:50:24Z

src/rust/engine/src/context.rs

-      // TODO: Errors in initialization should definitely be exposed as python
-      // exceptions, rather than as panics.


I imagine we want to preserve this comment by moving it up to the let vfs clause.

Eric-Arellano · 2020-03-18T20:51:47Z

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

@@ -538,9 +538,11 @@ impl Snapshot {
    future::result(digest_hint.ok_or_else(|| "No digest hint provided.".to_string()))
      .and_then(move |digest| Snapshot::from_digest(store, digest, workunit_store))
      .or_else(|_| {
+        let use_gitignore = false;


Iirc, this is only used by V1 code? V2 code that uses Snapshots will use Gitignore?

Eric-Arellano · 2020-03-18T20:52:25Z

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

@@ -687,7 +736,40 @@ impl PosixFS {
  }

  pub fn is_ignored(&self, stat: &Stat) -> bool {
-    self.ignore.is_ignored(stat)
+    println!("Are we actually invoking is_ignored? Stat: {:?}", stat);


Leftover, I think.

Eric-Arellano · 2020-03-18T20:52:48Z

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

@@ -495,6 +536,7 @@ impl PathGlobs {
      }
    }
    let include = PathGlob::spread_filespecs(include_globs.as_slice())?;
+    //TODO edited here


Eric-Arellano · 2020-03-18T20:54:29Z

src/python/pants/option/global_options.py

@@ -433,6 +433,14 @@ def register_bootstrap_options(cls, register):
            "Patterns use the gitignore syntax (https://git-scm.com/docs/gitignore). "
            "The `--pants-distdir` and `--pants-workdir` locations are inherently ignored.",
        )
+        register(
+            "--pants-gitignore",


I recommend --pants-use-gitignore or even --pants-use-gitignores. --pants-gitignore made me think that this would be a list of file paths to the gitignores you want to use.

in that vein prefixing with --pants seems redundant. why not just --use-gitignore

I guess to be consistent with --pants-ignore makes sense...we don't want to add more mental load remembering option scopes than we have to.

Hm, Henry's comment makes me think we should use --pants-ignore-use-gitignore. It's horribly verbose, but very explicitly expresses how the two options relate.

I'm not concerned about verbosity because this only gets set one time in a pants.toml and will usually never be thought of again. I think we want to optimize for clarity over verbosity here.

Eric-Arellano · 2020-03-18T20:54:40Z

src/python/pants/option/global_options.py

+            "--pants-gitignore",
+            advanced=True,
+            type=bool,
+            default=False,


What's the rationale for defaulting to False?

+1 .. should default to on, assuming it works in the absence of a .gitignore file.

Eric-Arellano · 2020-03-18T20:55:04Z

src/python/pants/engine/BUILD

@@ -254,7 +254,7 @@ python_library(

 python_library(
  name='console',
-  sources=['console.py'],
+  sources=['console.py', 'console2.py'],


Eric-Arellano · 2020-03-18T20:55:12Z

src/python/pants/engine/.gitignore

+/native_engine.so
+console.py


Eric-Arellano · 2020-03-18T20:55:16Z

.gitignore

@@ -1,3 +1,4 @@
+console2.py


stuhood · 2020-03-18T21:07:35Z

We don't yet have a way to override a gitignore, do we? I wonder if there's a use case for "I want to use 99% of my gitignore files, except for this one thing." Probably premature optimization to handle that now, so long as we have a path forward to add this in the future.

We do support that: see the linked ticket, assuming that the gitignore is applied "before" the literals passed to --pants-ignore.

stuhood

I'm about 98% sure that the recursive upward parsing of .gitignore files will be a performance bottleneck: assuming so, I see two options:

do the simpler thing, and only support the root .gitignore (for now)
incorporate looking for .gitignore files directly into scandir_sync calls, and propagate ignore information down the tree of calls

stuhood · 2020-03-18T21:10:51Z

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

@@ -687,7 +736,40 @@ impl PosixFS {
  }

  pub fn is_ignored(&self, stat: &Stat) -> bool {
-    self.ignore.is_ignored(stat)
+    println!("Are we actually invoking is_ignored? Stat: {:?}", stat);
+    if self.heed_gitignore_files {


This method is going to be called for every path that is matched or walked, so I don't think that re-parsing the git-ignore in each case is going to be viable from a performance perspective.

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

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

stuhood

Thanks! Just nits.

stuhood · 2020-04-03T21:33:24Z

src/python/pants/option/global_options.py

+            "--pants-gitignore",
+            advanced=True,
+            type=bool,
+            default=False,


+1 .. should default to on, assuming it works in the absence of a .gitignore file.

stuhood · 2020-04-03T21:34:10Z

src/python/pants/option/global_options.py

+            type=bool,
+            default=False,
+            help="Make use of .gitignore files when determining whether to ignore filesystem "
+            "operations performed by pants. This may be used together with `--pants-ignore`.",


This should indicate that the gitignore is prepended to --pants-ignore. The other option should possibly also reference this one, and explain how you can exclude things (see the comments on the ticket).

Also, it looks like this will only respect the root .gitignore file (which I think is totally reasonable for now). Should mention that here and/or link to a followup ticket.

Eric-Arellano · 2020-04-08T00:58:33Z

src/python/pants/option/global_options.py

+            "specified here apply after rules specified in a .gitignore file.",
+        )
+        register(
+            "--pants-use-gitignore",


Bump that I recommend --pants-ignore-use-gitignore for a consistent namespace with --pants-ignore. It's verbose, but this is an advanced option that will almost never actually be changed, and when it is, only by one Pants admin.

I think the namespace consistency is worth the verbosity.

[ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

and start threading option through to the engine [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

[ci skip-jvm-tests] # No JVM changes made. [ci skip-jvm-tests] # No JVM changes made. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

[ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

[ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

[ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

[ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made. # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests] # No JVM changes made.

[ci skip-jvm-tests]

gshuflin requested review from illicitonion and stuhood March 16, 2020 08:55

stuhood reviewed Mar 17, 2020

View reviewed changes

gshuflin force-pushed the gitignore2 branch from d880af6 to 9856cb3 Compare March 17, 2020 08:51

gshuflin marked this pull request as ready for review March 17, 2020 08:52

gshuflin force-pushed the gitignore2 branch from 9856cb3 to 4ef77aa Compare March 18, 2020 00:48

Eric-Arellano reviewed Mar 18, 2020

View reviewed changes

stuhood requested changes Mar 18, 2020

View reviewed changes

gshuflin force-pushed the gitignore2 branch from 4ef77aa to 3047dcb Compare March 30, 2020 17:53

hrfuller reviewed Mar 30, 2020

View reviewed changes

src/rust/engine/fs/src/posixfs_tests.rs Outdated Show resolved Hide resolved

gshuflin force-pushed the gitignore2 branch from 77cecd9 to fdf030f Compare March 30, 2020 18:21

gshuflin changed the title ~~Add --pants-gitignore option~~ Add --pants-use-gitignore option Mar 30, 2020

gshuflin force-pushed the gitignore2 branch 2 times, most recently from 2f71c6b to 71a107f Compare March 31, 2020 23:09

stuhood approved these changes Apr 3, 2020

View reviewed changes

gshuflin force-pushed the gitignore2 branch 7 times, most recently from f0ff089 to 13c1a56 Compare April 8, 2020 00:40

Eric-Arellano approved these changes Apr 8, 2020

View reviewed changes

gshuflin force-pushed the gitignore2 branch 5 times, most recently from 5a55877 to ee8abe6 Compare April 10, 2020 00:24

gshuflin force-pushed the gitignore2 branch 4 times, most recently from 7d87b16 to 21b06f1 Compare April 10, 2020 04:03

gshuflin added 8 commits April 9, 2020 23:12

Override ignored .class in gitignore file

0cbf1a7

[ci skip-jvm-tests]

gshuflin force-pushed the gitignore2 branch from 21b06f1 to e1ca1a1 Compare April 10, 2020 06:13

Also override /pants.pex ignore.

908feef

[ci skip-jvm-tests]

gshuflin force-pushed the gitignore2 branch from e1ca1a1 to 908feef Compare April 10, 2020 07:30

gshuflin merged commit a7eb868 into pantsbuild:master Apr 10, 2020

gshuflin deleted the gitignore2 branch April 10, 2020 09:01

gshuflin mentioned this pull request Apr 10, 2020

Support nested .gitignore files in --pants-ignore-use-gitignore #5682

Closed

stuhood mentioned this pull request Sep 18, 2023

Support nested .gitignore files in --pants-ignore-use-gitignore #19860

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --pants-use-gitignore option #9310

Add --pants-use-gitignore option #9310

gshuflin commented Mar 16, 2020 •

edited

Loading

stuhood left a comment •

edited

Loading

stuhood Mar 17, 2020

gshuflin commented Mar 17, 2020

Eric-Arellano left a comment

Eric-Arellano Mar 18, 2020

Eric-Arellano Mar 18, 2020

Eric-Arellano Mar 18, 2020

Eric-Arellano Mar 18, 2020

Eric-Arellano Mar 18, 2020

hrfuller Apr 3, 2020

hrfuller Apr 3, 2020

Eric-Arellano Apr 3, 2020

Eric-Arellano Mar 18, 2020

stuhood Apr 3, 2020

Eric-Arellano Mar 18, 2020

Eric-Arellano Mar 18, 2020

Eric-Arellano Mar 18, 2020

stuhood commented Mar 18, 2020

stuhood left a comment

stuhood Mar 18, 2020

stuhood left a comment

stuhood Apr 3, 2020

stuhood Apr 3, 2020

Eric-Arellano Apr 8, 2020

		// TODO: Errors in initialization should definitely be exposed as python
		// exceptions, rather than as panics.

		/native_engine.so
		console.py

Add --pants-use-gitignore option #9310

Add --pants-use-gitignore option #9310

Conversation

gshuflin commented Mar 16, 2020 • edited Loading

Problem

Solution

stuhood left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented Mar 17, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Mar 18, 2020

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented Mar 16, 2020 •

edited

Loading

stuhood left a comment •

edited

Loading