1058662 - Add a mozbuild.util.CaptureDeps class that wraps open() to capture input dependencies in Python scripts

Status: NEW

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

This is something I whipped up as an experiment, thinking that we could use it inside the file_generate action in bug 883954. Im not married to it, but it might help make dependencies for file generation less error-prone.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Add a mozbuild.util.CaptureDeps class that wraps open() to capture input dependencies in Python scripts

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8479107 [details] [diff] [review]
Add a mozbuild.util.CaptureDeps class that wraps open() to capture input dependencies in Python scripts

I think this is a great start. I definitely favor automatic dependencies over manual ones. Of course, I don't think there should be any manual dependencies anywhere ever, but I digress :)

You'll probably want some way to ignore files that you're expecting to write to in case a python script both reads and writes an output file to avoid a circular dependency with actual build rule (from backend.mk I suppose).

Also, are there other ways that we read files aside from open()? You mentioned that shelling out to other processes would be impractical, but what about things like os.stat() or os.path.exists()? Not that you need to cover all the bases from the start, but it's good to think about, or at least check through some of our scripts for other access patterns.

And it might be nice to make such a feature optional (for build systems that have dependency detection built-in :)

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8479107 [details] [diff] [review]
Add a mozbuild.util.CaptureDeps class that wraps open() to capture input dependencies in Python scripts

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

::: python/mozbuild/mozbuild/test/test_util.py
@@ +445,5 @@
> +            try:
> +                os.makedirs(os.path.dirname(f))
> +            except OSError:
> +                pass
> +            open(f, 'wb').write('foo')

You maybe could use MockedOpen, although I'm not entirely sure how well that'd mix with the tests you run.

@@ +448,5 @@
> +                pass
> +            open(f, 'wb').write('foo')
> +        with CaptureDeps() as deps:
> +            open(files[0], 'r')
> +            open(files[1], 'w+')

w+ truncates the file, I doubt anyone would do that for an input dependency.

::: python/mozbuild/mozbuild/util.py
@@ +720,5 @@
> +    def __enter__(self):
> +        import __builtin__
> +        self.real_open = __builtin__.open
> +        def fake_open(name, mode='r', buffering=-1):
> +            if mode.startswith('r') or mode == 'U' or '+' in mode:

wU is also a valid mode, and doesn't denote an input file.

@@ +729,5 @@
> +
> +    def __exit__(self, exc_type, exc_val, exc_tb):
> +        import __builtin__
> +        __builtin__.open = self.real_open
> +        return False

Note context manager functions can end up being simpler.

@contextmanager
def CaptureDeps():
    stuff_from_enter()
    yield read_files
    stuff_from_exit()

Comment 4

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #3)
> Note context manager functions can end up being simpler.
> 
> @contextmanager
> def CaptureDeps():
>     stuff_from_enter()
>     yield read_files
>     stuff_from_exit()

I was originally going to do something like that but I can't remember why I changed my mind. That should work fine. Thanks for the feedback.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bug 1058662 - Add a mozbuild.util.capture_deps() context manager that wraps open() and import to capture input dependencies in Python scripts.

bug 1058662 - Add a mozbuild.util.capture_deps() context manager that wraps open() and import to capture input dependencies in Python scripts.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1058662 - Use capture_deps in GENERATED_FILES to detect imported and opened files and add them as dependencies.

Bug 1058662 - Use capture_deps in GENERATED_FILES to detect imported and opened files and add them as dependencies.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

(In reply to Chris Manchester [:chmanchester] from comment #5)
> Created attachment 8685036 [details]
> MozReview Request: bug 1058662 - Add a mozbuild.util.capture_deps() context
> manager that wraps open() and import to capture input dependencies in Python
> scripts.
> 
> bug 1058662 - Add a mozbuild.util.capture_deps() context manager that wraps
> open() and import to capture input dependencies in Python scripts.

This incorporates the last feedback and adds something for imported files. I'm still trying to come up with a better way to get every file loaded by an import statement, but this is pretty effective.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8685036 [details]
MozReview Request: bug 1058662 - Add a mozbuild.util.capture_deps() context manager that wraps open() and import to capture input dependencies in Python scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24699/diff/1-2/

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8685037 [details]
MozReview Request: Bug 1058662 - Use capture_deps in GENERATED_FILES to detect imported and opened files and add them as dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24701/diff/1-2/

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8685036 [details]
MozReview Request: bug 1058662 - Add a mozbuild.util.capture_deps() context manager that wraps open() and import to capture input dependencies in Python scripts.

https://reviewboard.mozilla.org/r/24699/#review22601

I'm not convinced we should go with the heavylifting and divert import. mozbuild.pythonutil.iter_modules_in_path is what we use everywhere. I see no reason not to use it here too.

::: python/mozbuild/mozbuild/util.py:968
(Diff revision 2)
> +        if mode.startswith('r') or 'U' in mode or mode == 'a+':

'+' in mode ?

::: python/mozbuild/mozbuild/util.py:975
(Diff revision 2)
> +    yield None

You don't need None here.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8685037 [details]
MozReview Request: Bug 1058662 - Use capture_deps in GENERATED_FILES to detect imported and opened files and add them as dependencies.

https://reviewboard.mozilla.org/r/24701/#review22605

I think this should just use iter_modules_in_path as mentioned in previous comment.

::: python/mozbuild/mozbuild/action/file_generate.py:18
(Diff revision 2)
> +from mozbuild.util import FileAvoidWrite, capture_deps

from ... import (
  foo,
  bar,
)

Comment 12

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/24699/#review22601

I don't see how to achieve this with iter_modules_in_path. Do you mean this should use mozbuild.pythonutil.iter_modules_in_path(dirname(__file__))? That's probably sufficient for a lot of cases, but it's not really the same thing.

Another less heavy way we could do this is to diff the contents of sys.modules. This would make the output depend on the set of already loaded modules, but it would be effective at least for GENERATED_FILES.

> '+' in mode ?

You commented earlier that 'w+' truncates the file, so it doesn't indicate an input dependency.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/24699/#review22601

iter_modules_in_path also depends on the modules already loaded.

Comment 14

[Mike Hommey [:glandium]]

The point of iter_modules_in_path is to pass it topsrcdir, and use it last.