941245 - mozpack.files.FileFinder should ignore some directories

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

I need to teach FileFinder how to ignore directories as part of bug 939367.

Technically, we could perform filtering after traversal. But there are perf concerns when traversing very large directories.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Allow FileFinder to ignore some directories

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8335540 [details] [diff] [review]
Allow FileFinder to ignore some directories

Review of attachment 8335540 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozpack/files.py
@@ +536,5 @@
> +        ``ignored_dirs`` accepts an iterable of directories to ignore. While
> +        filtering could be performed on the consumer side, preventing traversal
> +        of large directories inside the finder can be a significant performance
> +        win. Please ensure the ignored directory paths are normalized to
> +        ``base`` or behavior may not work as expected.

That last sentence is both unclear and weird. We do find('subdir'), not find('/full/path/to/subdir'), why would the ignore list use full paths?

@@ +564,5 @@
>          Ignores file names starting with a '.' under the given path. If the
>          path itself has leafs starting with a '.', they are not ignored.
>          '''
> +        base = os.path.join(self.base, path)
> +        if base.startswith(self.ignored_dirs):

It would be better to use any(mozpack.path.match(base, d + '/**') for d in self.ignored_dirs). It would both solve the problem that your startswith would match if path is a/bc and ignored_dirs contains a/b, and allow wildcards in ignored directories.

@@ +582,5 @@
>          srcpath = os.path.join(self.base, path)
>          if not os.path.exists(srcpath):
>              return
>  
> +        if os.path.dirname(srcpath).startswith(self.ignored_dirs):

same as above. Also, there's no reason, really, to limit that to directories. Just put an ignore list. If a directory matches, it's not recursed. If a file matches, it's skipped.

::: python/mozbuild/mozpack/test/test_files.py
@@ +735,5 @@
> +        self.do_check('foo/*', ['foo/bar', 'foo/baz'])
> +        self.do_check('foo/**', ['foo/bar', 'foo/baz'])
> +        self.do_check('foo/qux/**', [])
> +        self.do_check('foo/qux/*', [])
> +        self.do_check('foo/qux/bar', [])

self.do_check('fooz', ['fooz'])

Comment 3

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> > +        self.do_check('foo/*', ['foo/bar', 'foo/baz'])
> > +        self.do_check('foo/**', ['foo/bar', 'foo/baz'])
> > +        self.do_check('foo/qux/**', [])
> > +        self.do_check('foo/qux/*', [])
> > +        self.do_check('foo/qux/bar', [])
> 
> self.do_check('fooz', ['fooz'])

err
self.do_check('foo/quxz', ['foo/quxz'])

Comment 4

[Mike Hommey [:glandium]]

Ah that doesn't work either because the file doesn't exist, but you get the point.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Allow FileFinder to ignore entries

Reimplemented with support for files and patterns in ignore expressions.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Allow FileFinder to ignore some directories

Using mozpack.path.match() everywhere instead of resolving ignore
patterns at __init__ time.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Allow FileFinder to ignore patterns

Removed bit rot.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8345097 [details] [diff] [review]
Allow FileFinder to ignore patterns

Review of attachment 8345097 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozpack/files.py
@@ +613,5 @@
>                  if mozpack.path.match(p, mozpack.path.join(*pattern)):
>                      yield p, f
>          elif '*' in pattern[0]:
> +            full = os.path.join(self.base, base)
> +            if not os.path.exists(full):

You're not using full anywhere else anymore.

@@ +624,4 @@
>              # See above comment w.r.t. sorted() and idempotent behavior.
>              for p in sorted(os.listdir(os.path.join(self.base, base))):
> +                if mozpack.path.match(base, p):
> +                    return

I don't think this test is doing something useful. (hint: none of the arguments is derived from self.ignore)

::: python/mozbuild/mozpack/test/test_files.py
@@ +736,5 @@
> +        self.do_check('foo/**', ['foo/bar', 'foo/baz'])
> +        self.do_check('foo/qux/**', [])
> +        self.do_check('foo/qux/*', [])
> +        self.do_check('foo/qux/bar', [])
> +        self.do_check('fooz', ['fooz'])

That's not very relevant with foo/qux as ignored pattern. You'd need something like foo/quxz

Comment 9

[Gregory Szorc [:gps]]

Attachment: Allow FileFinder to ignore patterns

Made tests a bit more robust. Removed extra check that did nothing.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8346310 [details] [diff] [review]
Allow FileFinder to ignore patterns

Review of attachment 8346310 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozpack/test/test_files.py
@@ +739,5 @@
> +        self.do_check('foo/**', ['foo/bar', 'foo/baz', 'foo/quxz'])
> +        self.do_check('foo/qux/**', [])
> +        self.do_check('foo/qux/*', [])
> +        self.do_check('foo/qux/bar', [])
> +        self.do_check('fooz', ['fooz'])

self.do_check('foo/quxz', ['foo/quxz']) for some more completeness.

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f19da0dfb3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6f19da0dfb3