Closed Bug 1320194 Opened 8 years ago Closed 7 years ago

Invoke emitter directly when resolving tests so we don't generate the entire backend

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(6 files, 2 obsolete files)

Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend
58 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter
58 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1320194 - Fix bug preventing more than two backends in a HybridBackend
59 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests
59 bytes, text/x-review-board-request
mshal
: review+
Details
make-check.txt
10.00 KB, text/plain
Details
Bug 1320194 - Use 'wildcard' instead of dummy rules when re-generating build backends,
59 bytes, text/x-review-board-request
glandium
: review+
Details 
Currently when resolving tests we run 'make tests-deps':
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/testing.py#188

This requires an objdir, which means "source-check" tests (python unittests for example) can't run in their own build independent tasks. To get around this I believe we could first explicitly generate the full backend, but doing that is still a lot unnecessary extra processing.

It would be much better to invoke the emitter directly, factor out the relevant code in the backend into a separate module and consume the frontend directly from the test resolver.
You still need a config.status (or some file containing the necessary configuration), and you still need a virtualenv.
Yeah I was just realizing that myself.. I guess we could generate config.status and then not invoke it so that we only consume TestManifest data.. Or possibly we could make a TestOnlyBackend or something like that. But this seems like less of a benefit than I was originally hoping for :/

Would it be possible to get the reader/emitter to whitelist/blacklist which variables it cares about? I think if we were only emitting manifest files, the only config we'd need is topsrcdir.
That said, I think this is too much scope bloat for the purposes of getting python unittests into their own task. I'm just going to generate the full backend for now and leave this open for future optimization. I'll upload the patches I have so far for posterity (though they do rely on config.status, which defeats the purpose of this in the first place).
Attachment #8814457 - Attachment is obsolete: true
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
With the above patches I was able to run mozbase in its own task purely from the srcdir:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f1222cfe04c9213f8d901bd7fa160ece6ec444b

This series creates a new TestManifestBackend(PartialBackend) as well as informally introduces the concept of a PartialEmitter. Please feel free to give feedback, what's here is probably going to be pretty close to what I'll eventually put up for review. The only major thing left is figuring out how to only re-generate the test metadata files if something relevant has changed.
Blocks: 1003417
This is a request for feedback on the overall patch series (rather than doing 3 feedback flags). Chris, I know you already expressed dislike for creating a new emitter class (which is in the second commit), but figured I'd leave it for now so you can see it before I re-factor everything.
Flags: needinfo?(cmanchester)
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review98074

::: python/mozbuild/mozbuild/testing.py:189
(Diff revision 2)
>          MozbuildObject.__init__(self, *args, **kwargs)
>  
>          # If installing tests is going to result in re-generating the build
>          # backend, we need to do this here, so that the updated contents of
>          # all-tests.pkl make it to the set of tests to run.
> -        self._run_make(target='run-tests-deps', pass_thru=True,
> +        if self.test_configuration_changed():

I think you might be able to do self._run_make(target='backend.TestManifestBackend') and take advantage of the Makefile logic to re-generate the build backend if any of the inputs have changed (https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/Makefile.in#115 through line 131)

The test_configuration_changed() and generate_test_metadata() functions can then go away I believe.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review98074

> I think you might be able to do self._run_make(target='backend.TestManifestBackend') and take advantage of the Makefile logic to re-generate the build backend if any of the inputs have changed (https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/Makefile.in#115 through line 131)
> 
> The test_configuration_changed() and generate_test_metadata() functions can then go away I believe.

`run-tests-deps` depends on BUILD_BACKEND_FILES, so I think the current set up would do this as well if we had the TestManifestBackend enabled.
Looking at the patches I think I'll stick with my feedback that we can and should achieve this without introducing new concepts to the emitter. Is there a compelling reason to add a PartialEmitter I'm not thinking of? The current set-up suggests the backend as the place to divide concerns, and I think that's serving us pretty well so far.
Flags: needinfo?(cmanchester)
Oh, I remember why I refactored the emitter now! The TreeMetaDataEmitter uses some data from ConfigEnvironment, whereas the TestManifestEmitter can work with nothing but a frontend.reader.EmptyConfig.. So because we don't have a build, we can't emit stuff that depends on having a ConfigEnvironment. I could probably refactor fix it to not assume there is a valid ConfigEnvironment though.
Attachment #8815029 - Attachment is obsolete: true
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review98074

> `run-tests-deps` depends on BUILD_BACKEND_FILES, so I think the current set up would do this as well if we had the TestManifestBackend enabled.

The problem with this is that it won't work with a clobbered objdir:

Exception: Process executed with non-0 exit code 2: [u'/usr/bin/gmake', u'-j8', u'-s', u'-w', u'backend.TestManifestBackend']

  File "/home/ahal/hg/mozilla-central/python/mach_commands.py", line 118, in python_test
    resolver = self._spawn(TestResolver)
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozbuild/base.py", line 623, in _spawn
    topobjdir=self.topobjdir)
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozbuild/testing.py", line 191, in __init__
    self._run_make(target='backend.TestManifestBackend')
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozbuild/base.py", line 551, in _run_make
    return fn(**params)
  File "/home/ahal/hg/mozilla-central/python/mozbuild/mozbuild/base.py", line 606, in _run_command_in_objdir
    return self.run_process(cwd=self.topobjdir, **args)
  File "/home/ahal/hg/mozilla-central/python/mach/mach/mixin/process.py", line 147, in run_process
    raise Exception('Process executed with non-0 exit code %d: %s' % (status, args))

Which defeats the main motivation behind this patch. The python functions will work even if the objdir doesn't exist.
Attachment #8814456 - Flags: review?(cmanchester)
Attachment #8816285 - Flags: review?(cmanchester)
I need more feedback before I can continue.. Flagging you Chris since you already took a look at this. Feel free to treat this as a feedback and/or redirect review to gps if you want.
A good way to see this locally is to clobber, then run:
./mach python-test python/mozlint

Since those tests don't depend on an objdir for anything they'll just work. However, running:
./mach python-test python/mozbuild/mozbuild

yields no results because those tests do depend on an objdir. This works due to how moz.build files are processed. Without an objdir, we only process a small subset of moz.build, which will need to contain all PYTHON_UNITTEST_MANIFESTS variables that don't depend on an objdir.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

::: python/mozbuild/mozbuild/testing.py:243
(Diff revision 4)
> +        try:
> +            config = self.config_environment
> +        except BuildEnvironmentNotFoundException:
> +            config = EmptyConfig(self.topsrcdir)
> +            config.topobjdir = self.topobjdir

Thinking on this some more, I'm really hesitant to pass an empty config here and expect things to work. It isn't going to for suites like mochitests that conditionally include manifests based on a config, so this can't be extended to running those from the srcdir in the future.

If we run |./mach configure| on the tester with the TestManifest backend enabled and --disable-compile-environment set I expect it would complete in a matter of seconds, and would save us from re-inventing make's dependency tracking in Python.
Attachment #8816285 - Flags: review?(cmanchester)
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review103552

::: python/mozbuild/mozbuild/backend/common.py:187
(Diff revision 5)
> +        # Temporarily compose a partial TestManifestBackend, soon test manifest
> +        # processing will no longer be part of the build and this will be removed.
> +        self._test_backend = TestManifestBackend(self.environment)
> +

I know this is a temporary commit, but I think the way to achieve this would be to enable the TestManifest backend by default in configure so it always runs alongside the main backend, or maybe make it a hybrid backend like FasterMake+RecursiveMake.
Attachment #8814456 - Flags: review?(cmanchester)
If we're intent on not running configure, the other approach I could see here would be just running the reader and getting our tests to run from that to let us do this for mozbase tests, and deferring the question of how to do this for other suites until later.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

> Thinking on this some more, I'm really hesitant to pass an empty config here and expect things to work. It isn't going to for suites like mochitests that conditionally include manifests based on a config, so this can't be extended to running those from the srcdir in the future.
> 
> If we run |./mach configure| on the tester with the TestManifest backend enabled and --disable-compile-environment set I expect it would complete in a matter of seconds, and would save us from re-inventing make's dependency tracking in Python.

Passing in an empty config actually works fine, even with mochitest. Mach will bail out if there is no build_obj before we even get to the TestResolving phase, but if it didn't then the resolver would simply return 0 results and the mach command would presumably print an appropriate error message. The reason is because just like my previous patch, we're only emitting MANIFEST objects (via the emitfn argument I added to TreeMetadata.emit), and MANIFEST objects don't need any special build configuration to process.

The problem with running configure is that developers don't have `--disable-compile-environment` set locally, so when the TestManifestBackend changes, it'll still generate the full backend. The nice thing about this patch is that it also speeds up local development, seems a shame to lose that. I actually do a lot of my development on top of this patch series now because it's much nicer when modifying tests :)
(In reply to Chris Manchester (:chmanchester) from comment #28)
> If we're intent on not running configure, the other approach I could see
> here would be just running the reader and getting our tests to run from that
> to let us do this for mozbase tests, and deferring the question of how to do
> this for other suites until later.

I still need to do more testing, but I'm about 90% sure that this works for all current suites as is. Would it help if the TestResolver lived outside mozbuild? I was about 50/50 on whether to leave it there or move it somewhere under /testing.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

For some extra context, I started down this path at gps' suggestion in this irc conversation:
http://logs.glob.uno/?c=mozilla%23build&s=18+Nov+2016&e=18+Nov+2016&h=ahal%3A#c31355

Not sure if where I ended up is close to what he was talking about though.. Gps, mind taking a look at this patch series and letting me know if this is close to what you meant?
Attachment #8816285 - Flags: feedback?(gps)
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

> Passing in an empty config actually works fine, even with mochitest. Mach will bail out if there is no build_obj before we even get to the TestResolving phase, but if it didn't then the resolver would simply return 0 results and the mach command would presumably print an appropriate error message. The reason is because just like my previous patch, we're only emitting MANIFEST objects (via the emitfn argument I added to TreeMetadata.emit), and MANIFEST objects don't need any special build configuration to process.
> 
> The problem with running configure is that developers don't have `--disable-compile-environment` set locally, so when the TestManifestBackend changes, it'll still generate the full backend. The nice thing about this patch is that it also speeds up local development, seems a shame to lose that. I actually do a lot of my development on top of this patch series now because it's much nicer when modifying tests :)

Historical context: making test manifests conditional on variables is mostly a cargo cult from Makefiles. And, IIRC having separate directories to declare multiple test manifests was a by-product of the days when you could only have a single manifest of a certain type in a directory and/or because the `make`-based mechanisms for running tests were so bad that it was needed for ergonomics.

Given advancements in our build and test infrastructure and knowledge learned from moz.build, I think requiring that test manifests be unconditionally defined in moz.build files is a good idea. I would support efforts to establish a static analysis rule that required test manifests not be behind conditionals. At that point, directory based moz.build reading with an "empty" config would be sufficient to discover all tests and filtering could be done from metadata in the manifests based on mozinfo attributes.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103978

This approach looks pretty good to me modulo the caveats about an incomplete build config, which we can't really do that much about at the moment. But I'm optimistic this won't bite too many people. The testing-related mach commands should all be behind a conditional that requires a build context to run (when required). This should protect us from most gotchas with regards to an incomplete build config.

::: python/mozbuild/mozbuild/testing.py:221
(Diff revision 4)
> +        backend_path = os.path.join(self.topobjdir, 'backend.TestManifestBackend.in')
> +        if not os.path.isfile(backend_path):
> +            return True
> +
> +        with open(backend_path, 'rb') as fh:
> +            paths = [p.strip() for p in fh.readlines()]

Nit: you can drop the `.readlines()` because using a file object as an iterator has the same effect and avoids loading all the lines into a temporary list instance.

::: python/mozbuild/mozbuild/testing.py:226
(Diff revision 4)
> +                # touch the backend file in case it didn't change
> +                with io.open(backend_path, 'ab'):
> +                    os.utime(backend_path, None)

This should be done after the backend has been generated, not before. Otherwise, if e.g. backend generation fails, the next invocation will think the backend is up to date and won't generate it.

I grok that the backend may not touch the file. You can move this code to `generate_test_metadata()`.

::: python/mozbuild/mozbuild/testing.py:243
(Diff revision 4)
> +        try:
> +            config = self.config_environment
> +        except BuildEnvironmentNotFoundException:
> +            config = EmptyConfig(self.topsrcdir)
> +            config.topobjdir = self.topobjdir

Until we fix issues with the empty config, we should print a message saying the build config couldn't be loaded and that test manifests may be incomplete. As long as the message is somewhat alarming, that should hopefully cover us against people getting confused about the behavior.
Attachment #8816285 - Flags: feedback?(gps) → feedback+
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

> Historical context: making test manifests conditional on variables is mostly a cargo cult from Makefiles. And, IIRC having separate directories to declare multiple test manifests was a by-product of the days when you could only have a single manifest of a certain type in a directory and/or because the `make`-based mechanisms for running tests were so bad that it was needed for ergonomics.
> 
> Given advancements in our build and test infrastructure and knowledge learned from moz.build, I think requiring that test manifests be unconditionally defined in moz.build files is a good idea. I would support efforts to establish a static analysis rule that required test manifests not be behind conditionals. At that point, directory based moz.build reading with an "empty" config would be sufficient to discover all tests and filtering could be done from metadata in the manifests based on mozinfo attributes.

I don't think manifests hidden behind conditional variables are a problem, because if there is no build, then those entire moz.build files get filtered out implicitly by the DIRS variable:
https://dxr.mozilla.org/mozilla-central/source/moz.build#83

Without an objdir, we never include app.mozbuild, which in turn never includes toolkit.mozbuild, which in turn never includes testing/mochitest/moz.build. So as far as the resolver is concerned, there are no mochitest manifests without a build.

The one caveat to this approach is that any manifests that we want defined without a build need to somehow get included elsewhere. That's why I moved the inclusion of testing/mozbase/moz.build from toolkit.mozbuild into the root moz.build in this patch.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

> I don't think manifests hidden behind conditional variables are a problem, because if there is no build, then those entire moz.build files get filtered out implicitly by the DIRS variable:
> https://dxr.mozilla.org/mozilla-central/source/moz.build#83
> 
> Without an objdir, we never include app.mozbuild, which in turn never includes toolkit.mozbuild, which in turn never includes testing/mochitest/moz.build. So as far as the resolver is concerned, there are no mochitest manifests without a build.
> 
> The one caveat to this approach is that any manifests that we want defined without a build need to somehow get included elsewhere. That's why I moved the inclusion of testing/mozbase/moz.build from toolkit.mozbuild into the root moz.build in this patch.

Yeah - I wrote this comment before I looked at the code: I thought you were going to use filesystem-based traversal to load moz.build files!

But the "null" traversal starting from root is probably just as good anyway.

FWIW, I think there have been mumblings of doing away with DIRS as a traversal-based mechanism and just maintaining a single list of dirs/moz.build files to execute. Then, each moz.build would have to conditionalize its definitions explicitly. One advantage of this is you could load the full set of things then filter by what is actually used. But that is way beyond the scope of this change :)
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

> Yeah - I wrote this comment before I looked at the code: I thought you were going to use filesystem-based traversal to load moz.build files!
> 
> But the "null" traversal starting from root is probably just as good anyway.
> 
> FWIW, I think there have been mumblings of doing away with DIRS as a traversal-based mechanism and just maintaining a single list of dirs/moz.build files to execute. Then, each moz.build would have to conditionalize its definitions explicitly. One advantage of this is you could load the full set of things then filter by what is actually used. But that is way beyond the scope of this change :)

Yeah, adding a conditional in the event DIRS disappears would be fine too. This patch actually does this exact thing to python/moz.build to put the mozbuild unittests behind having a build app.
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review103552

> I know this is a temporary commit, but I think the way to achieve this would be to enable the TestManifest backend by default in configure so it always runs alongside the main backend, or maybe make it a hybrid backend like FasterMake+RecursiveMake.

I can't seem to get this working, I'm trying to add TestManifestBackend to the defaults here:
https://dxr.mozilla.org/mozilla-central/source/moz.configure#140

However, if I append it to the all_backends list, then I get an `Unhandled object of type TestManifest` exception. And if I try to make a hybrid `TestManifest+FasterMake+RecursiveMake` then it gets stuck in an infinite loop for some reason. Do you have any ideas on how this should work?
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review103552

> I can't seem to get this working, I'm trying to add TestManifestBackend to the defaults here:
> https://dxr.mozilla.org/mozilla-central/source/moz.configure#140
> 
> However, if I append it to the all_backends list, then I get an `Unhandled object of type TestManifest` exception. And if I try to make a hybrid `TestManifest+FasterMake+RecursiveMake` then it gets stuck in an infinite loop for some reason. Do you have any ideas on how this should work?

Each backend invocation is expected to ack each object emitted from the emitter. That's where this error comes from. Not sure where the infinite loop is happening. ^C during execution to get a stack trace?
Chris, I've replied to your review comments but left them open. I think I've been doing a bad job explaining things.. so let me know if my replies addressed your concern or not.
The loop is broad. Basically it has problems with the backend.in file, thinks the build configuration has changed and re-runs configure:

 0:03.78 Reticulating splines...
 0:16.90 Finished reading 1188 moz.build files in 3.43s
 0:16.90 Read 73 gyp files in parallel contributing 0.00s to total wall time
 0:16.90 Processed into 7956 build config descriptors in 3.45s
 0:16.90 TestManifest+RecursiveMake backend executed in 2.85s
 0:16.90   2061 total backend files; 2033 created; 1 updated; 27 unchanged; 0 deleted
 0:16.90 Total wall time: 13.12s; CPU time: 13.02s; Efficiency: 99%; Untracked: 3.39s
 0:17.29 cat: backend.TestManifest+FasterMake+RecursiveMakeBackend.in: No such file or directory
 0:17.31 Build configuration changed. Regenerating backend.
 0:17.49 Reticulating splines...
 0:30.04 Finished reading 1188 moz.build files in 3.44s
 0:30.04 Read 73 gyp files in parallel contributing 0.00s to total wall time
 0:30.05 Processed into 7956 build config descriptors in 3.34s
 0:30.05 TestManifest+RecursiveMake backend executed in 2.49s
 0:30.05   2060 total backend files; 0 created; 2 updated; 2058 unchanged; 0 deleted
 0:30.05 Total wall time: 12.58s; CPU time: 12.47s; Efficiency: 99%; Untracked: 3.30s
 0:30.33 cat: backend.TestManifest+FasterMake+RecursiveMakeBackend.in: No such file or directory
 0:30.33 Build configuration changed. Regenerating backend.
 0:30.51 Reticulating splines...

I'd rather not spend a ton of time on this as the change was only meant to be temporary (to avoid breaking the intermediate commit). The current patch is hacky, but it works well enough, and the hack is removed in the next commit.
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review103552

> Each backend invocation is expected to ack each object emitted from the emitter. That's where this error comes from. Not sure where the infinite loop is happening. ^C during execution to get a stack trace?

Sorry, forgot to reply in mozreview. Here was the reply in bugzilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=1320194#c42
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review104390

My concerns haven't been addresssed here, but given the discussion I think gps would be a better reviewer for this series.
Attachment #8816285 - Flags: review?(cmanchester)
Attachment #8814456 - Flags: review?(gps)
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review103552

> Sorry, forgot to reply in mozreview. Here was the reply in bugzilla:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1320194#c42

Given this is a temporary change that exists only in a single commit, I don't want to waste too much time getting this suggestion working, so dropping this issue. Please re-open if you disagree.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review103550

> Yeah, adding a conditional in the event DIRS disappears would be fine too. This patch actually does this exact thing to python/moz.build to put the mozbuild unittests behind having a build app.

I believe the original concern in this issue was that this would not extend to mochitest once we are running everything in the srcdir. I believe the answer to this concern was that we would never pass in an empty config for mochitests because the mach command will error out before we ever get to that point. Plus, we could always refactor the conditions moz.build files if we needed to, or even disallow conditional inclusions of manifests outright.

Given that, I don't see any further work required here so dropping the issue. Please re-open if you disagree.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

For some reason you are flagged in mozreview, but not bugzilla.
Attachment #8816285 - Flags: review?(gps)
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review103552

> Given this is a temporary change that exists only in a single commit, I don't want to waste too much time getting this suggestion working, so dropping this issue. Please re-open if you disagree.

While we strive for commit level bisectability, I would not continue to sink time into this.
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review105340

This seems like a pretty straightforward refactor. Thanks for adding the extra test.
Attachment #8814456 - Flags: review?(gps) → review+
Depends on: 1331062
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review105348

I'm still looking through the code, but I found a show-stopper.

I performed an artifact build with this series applied then tried to `mach test browser/components/search/test/browser_ddg.js`. This fails with multiple Python module import errors. First with `pytoml` (I fixed this in bug 1331062). Then with `gyp`.

The underlying reason for the module import failure is that `mach`'s Python environment is not the same as the build system's virtualenv. They overlap 95% but differ in subtle ways.

`mach`'s Python environment is not capable of running a lot of build system code. Instead, it needs to run through the virtualenv. You can do this by activating the virtualenv in the mach process (https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/python/mozbuild/mozbuild/mach_commands.py#661). Alternatively, you can invoke `make` to do build backend foo.

I think this issue is related to a previous point raised by chmanchester. So I may make a recommendation once I've fully absorbed this code...
Attachment #8816285 - Flags: review?(gps)
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review105354

::: python/mozbuild/mozbuild/testing.py:189
(Diff revision 5)
> -        self._run_make(target='run-tests-deps', pass_thru=True,
> -                       print_directory=False)
> +        if self.test_configuration_changed():
> +            self.generate_test_metadata()

I was able to make the Python import problem go away by adding a `self._activate_virtualenv()` before the call to `self.generate_test_metadata()`.

I /think/ this will preserve the property of allowing this code to work without an objdir when invoked. However, as a result of invoking `_activate_virtualenv()`, an objdir will be created to hold the virtualenv. Not sure if that is desired.

::: python/mozbuild/mozbuild/testing.py:215
(Diff revision 5)
> +    def test_configuration_changed(self):
> +        backend_path = os.path.join(self.topobjdir, 'backend.TestManifestBackend.in')
> +        if not os.path.isfile(backend_path):
> +            return True
> +
> +        with open(backend_path, 'rb') as fh:
> +            paths = [p.strip() for p in fh]
> +
> +        backend_modified = os.path.getmtime(backend_path)
> +        for path in paths:
> +            if os.path.getmtime(path) > backend_modified:
> +                return True
> +        return False

Having read the previous reviews, I agree with mshal and chmanchester that this wheel reinvention is undesirable. We have an existing mechanism for managing backends. And that code is important and somewhat fragile. So it's a wheel I would highly prefer to not reinvent.

Your new requirement is that we be able to perform backend generation without an objdir.

Using `make` here is a bit complicated. We invoke `make` in the objdir against `Makefile` files derived from `Makefile.in` files in the srcdir. You can't just invoke `make` against a `Makefile.in` because `Makefile.in` files make a lot of assumptions about file paths, variable presence, etc.

I see 2 options:

1) Move the make logic for invoking a build backend out of the root `Makefile.in` and into another `.mk` file and refactor it in such a way that it can be invoked by the root `Makefile.in` *and* from a command line invocation outside of a build context (you can pass variables into `make` as arguments).

2) Port the code for verifying the backend is up to date and running the backend into Python (you've done this in this patch) and convert `Makefile.in` to call that. But there be dragons with this approach. Practically every dependency in the make DAG chains back to the build backend being up to date. So you'd need to trick make into always invoking the backend generation process (just in case) while still treating the dependency as "up to date" so it doesn't rebuild dependencies if the backend didn't change. With our current backend integration in the root `Makefile.in`, I think this will be very difficult to do.

So I'd go with option 1.
Attachment #8816285 - Flags: review-
Thanks for the suggestions, I'll take a look. I would also prefer not to re-invent the wheel.. The python functions are mostly because I know how to do that and am pretty clueless at make. But I think this will point me in the right direction. I'll follow-up if I have more questions.
Depends on: 1331663
I found an edge case that mshal's refactor doesn't cover:

$ ./mach clobber
$ ./mach python-test python/mozlint  # this generates the TestManifest backend
$ ./mach build
$ ./mach mochitest -f plain          # no tests are found

This happens because TestManifestBackend is no longer consumed as part of the build, so when we do a build it doesn't think the backend needs to be re-generated. I think there are two solutions:

1) Keep running the TestManifestBackend as part of the build
2) Delete backend.TestManifestBackend.in if it exists as part of the build

The advantage of 1) is that it's cleaner. The advantage of 2) is that it speeds up build times by a few seconds.
I tried doing 1) by creating a FasterMake+TestManifest+RecursiveMake backend, but after fixing a bug in the HybridBackend code the builds fail with:
0:25.32 gmake[5]: *** No rule to make target '../dist/bin/.gdbinit_python', needed by '/home/ahal/hg/mozilla-central/objdirs/firefox_artifact/.gdbinit_python'.  Stop.

I'm going to throw something together for 2), and ask for feedback and go from there.
The latest review series *solves* comment 54 by manually deleting any TestManifest backend files found in the objdir from CommonBackend.

But a try run revealed another problem on Windows, after running ./mach python-test, we hang somewhere while generating the TestManifestBackend:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8e386284253fb196f9d07dbdaf500bc515f61aa

Here's a log snippet:
> 15:06:05     INFO - Starting 'mach python-test' with -j16
> 15:06:08     INFO - cat: backend.TestManifestBackend.in: No such file or directory
> 15:06:08     INFO - Build configuration changed. Regenerating backend.
> command timed out: 10800 seconds without output 

Both mac and linux work fine though.
Attached file make-check.txt — 
Here's the backtrace I get when I ran it in my Windows VM and saw the hang. I hit ctrl-C to stop it, so the KeyboardInterrupt stuff probably isn't relevant. I'll take a look tomorrow, but I figured I'd post it now in case anyone wants to take a look.
Just calling multiprocessing.freeze_support in gen_test_backend.py seems to work locally. I'll do a try push as well.
I can confirm this fixes it, thanks! Actually, the freeze_support() call turned out to be unnecessary, as long as everything in  gen_test_backend.py is behind the if __name__ == '__main__' guard.

I don't really understand why multiprocessing is coming into play here, but oh well.
Oh, strange. Well just adding the main guard works for me too.

One other issue I found is in the test_test_manifest.py test - the manifest_path key was using different slashes from the paths loaded by the pickle.load() call, so it resulted in a KeyError. I was able to work around it with this patch:

diff --git a/python/mozbuild/mozbuild/test/backend/test_test_manifest.py b/python/mozbuild/mozbuild/test/backend/test_test_manifest.py
index a4ca872..3a791d9 100644
--- a/python/mozbuild/mozbuild/test/backend/test_test_manifest.py
+++ b/python/mozbuild/mozbuild/test/backend/test_test_manifest.py
@@ -57,14 +57,14 @@ class TestTestManifestBackend(BackendTester):
         self.assertTrue(os.path.exists(test_defaults_path))
 
         with open(test_defaults_path, 'rb') as fh:
-            o = pickle.load(fh)
+            o = {mozpath.normpath(k): v for k, v in pickle.load(fh).iteritems()}
 
         self.assertEquals(set(mozpath.relpath(k, env.topsrcdir) for k in o.keys()),
                           set(['dir1/xpcshell.ini',
                                'xpcshell.ini',
                                'mochitest.ini']))
 
-        manifest_path = os.path.join(env.topsrcdir, 'xpcshell.ini')
+        manifest_path = mozpath.normpath(os.path.join(env.topsrcdir, 'xpcshell.ini'))
         self.assertIn('here', o[manifest_path])
         self.assertIn('support-files', o[manifest_path])

But I'm not sure if that's just hiding a real underlying issue.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48feaf953ae5a41af380cfd3260c333adfb96a93
Thanks, yeah I had a similar fix going on try too. That's a new test that is added with this patch series, so I don't think anything underlying is wrong, it was just poorly tested. I'll put the new series up for review tomorrow once I've finished cleaning some things up and some try results come back in.
Comment on attachment 8831701 [details]
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests

https://reviewboard.mozilla.org/r/108248/#review109888

::: python/mozbuild/mozbuild/backend/common.py:331
(Diff revision 2)
>                  'shared_libraries': [s.to_dict() for s in self._binaries.shared_libraries],
>                  'programs': [p.to_dict() for p in self._binaries.programs],
>              }
>              json.dump(d, fh, sort_keys=True, indent=4)
>  
> +        # Clean up any previous external_backend files that might exist.

I want to call this out, it seems kind of hacky but I couldn't think of any better way to handle it. It's aimed to solve this edge case:

1. mach clobber
2. mach python-test python/mozbuild/dumbmake
3. mach build
4. mach mochitest

In step 4, the backend does not get regenerated because it was previously generated in step 2 and doesn't know anything has changed.
concurrent.futures is likely using multiprocessing under the hood. And the only place we use concurrent.futures in this code is GYP evaluation. That's a nice rabbit hole you found there. Try hacking up the GYP reading code to use a ThreadPoolExecutor instead of ProcessPoolExecutor and see if the failure reproduces.
Comment on attachment 8816285 [details]
Bug 1320194 - Add ability to specify custom emitter function in TreeMetadataEmitter

https://reviewboard.mozilla.org/r/97072/#review109910
Attachment #8816285 - Flags: review?(gps) → review+
Comment on attachment 8831700 [details]
Bug 1320194 - Fix bug preventing more than two backends in a HybridBackend

https://reviewboard.mozilla.org/r/108246/#review109912

Good catch.
Attachment #8831700 - Flags: review?(gps) → review+
Comment on attachment 8831701 [details]
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests

https://reviewboard.mozilla.org/r/108248/#review109936

I very quickly looked at this and don't see any obvious show stoppers. Since it sounds like another revision is incoming and I have older reviews in my queue, I'm going to hold off on a full review. Maybe by my end of day. But no promises.
No worries, whenever you have time. Just want to clarify, barring review issues I don't plan on any further revisions for this bug. There is follow up work to this, but I'll be tackling that in other bugs.
Blocks: 1335873
Blocks: 1311991
Depends on: 1336559
Blocks: 1336559
No longer depends on: 1336559
Attachment #8831701 - Flags: review?(mshal)
Comment on attachment 8831701 [details]
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests

https://reviewboard.mozilla.org/r/108248/#review112112

This looks really close! I'm excited to have this land. However, I'd like us to revisit how we regenerate the TestManifest backend in the 'clobber; python-test; build; mochitest' scenario. If forcing the config.status dependency isn't feasible and we can't find another solution, I think we'll have to stick with this.

Also I noticed an issue on one of the other commits while reviewing this - we seem to be missing dependencies on the moz.build files. I'll file it there.

::: python/moz.build:29
(Diff revision 2)
>      'mach/mach/test/python.ini',
>      'mozbuild/dumbmake/test/python.ini',
> +    'mozlint/test/python.ini',
> +]
> +
> +if CONFIG['MOZ_BUILD_APP']:

By moving these tests inside the MOZ_BUILD_APP conditional, that prevents us from running it easily as a separate task along with the rest of the tests, right?

If so, please file a followup bug. I think most of these tests can work without an objdir if we just stub out the buildconfig module, since they generally rely on that only for topsrcdir / topobjdir. (Feel free to assign it to me)

::: python/mozbuild/mozbuild/backend/common.py:331
(Diff revision 2)
>                  'shared_libraries': [s.to_dict() for s in self._binaries.shared_libraries],
>                  'programs': [p.to_dict() for p in self._binaries.programs],
>              }
>              json.dump(d, fh, sort_keys=True, indent=4)
>  
> +        # Clean up any previous external_backend files that might exist.

Yeah, it is pretty ugly :(. I think the underlying problem here is that when generating the TestManifest backend in step 2), the mozbuild python code tries to read config.status, but gets an ENOENT and continues on without the config. However, even though the TestManifest backend would change if the file were created, we don't store a dependency on it in the backend.TestManifestBackend.in file.

We could instead force the dependency on config.status with something like this in gen_test_backend.py:

backend.backend_input_files.add(mozpath.join(config.topobjdir, 'config.status'))

However, since config.status doesn't exist if you just do successive runs of 'mach python-test', it will try to rebuild the backend every time you run the command. I'm not sure if that means we can't do that as a solution or not. What are your thoughts? Is it worth trying just adding TestManifestBackend as one of the default generated backends again?
Attachment #8831701 - Flags: review?(mshal) → review-
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review112188

::: python/mozbuild/mozbuild/backend/test_manifest.py:16
(Diff revision 8)
> +
> +from mozbuild.backend.base import PartialBackend
> +from mozbuild.frontend.data import TestManifest
> +
> +
> +class TestManifestBackend(PartialBackend):

By making this a PartialBackend, we end up skipping the logic that adds moz.build files as dependencies:

https://dxr.mozilla.org/mozilla-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/python/mozbuild/mozbuild/backend/base.py#135

I think we still want to have those as dependencies on the TestManifest backend, right? Otherwise if you add a new test manifest to a moz.build file I don't think it will get picked up.

I'm not sure what the correct solution here is. Is there a reason you don't want this to just inherit from CommonBackend instead?
Attachment #8831701 - Flags: review?(gps)
Comment on attachment 8814456 [details]
Bug 1320194 - Refactor test metadata related backend code into a partial TestManifestBackend

https://reviewboard.mozilla.org/r/95688/#review112188

> By making this a PartialBackend, we end up skipping the logic that adds moz.build files as dependencies:
> 
> https://dxr.mozilla.org/mozilla-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/python/mozbuild/mozbuild/backend/base.py#135
> 
> I think we still want to have those as dependencies on the TestManifest backend, right? Otherwise if you add a new test manifest to a moz.build file I don't think it will get picked up.
> 
> I'm not sure what the correct solution here is. Is there a reason you don't want this to just inherit from CommonBackend instead?

Good catch! I think the main reason was to avoid unnecessary overhead.. though wouldn't making it depend on CommonBackend mean we wouldn't be able to mix this in with RecursiveMake if we wanted to? Also, does CommonBackend consume every type of object? Otherwise doing that will raise.

I tested locally, and simply adding `self.backend_input_files |= obj.context_all_paths` to the TestManifestBackend's `consume_object()` seems to work great. Is that an acceptable workaround?
Comment on attachment 8831701 [details]
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests

https://reviewboard.mozilla.org/r/108248/#review112112

> Yeah, it is pretty ugly :(. I think the underlying problem here is that when generating the TestManifest backend in step 2), the mozbuild python code tries to read config.status, but gets an ENOENT and continues on without the config. However, even though the TestManifest backend would change if the file were created, we don't store a dependency on it in the backend.TestManifestBackend.in file.
> 
> We could instead force the dependency on config.status with something like this in gen_test_backend.py:
> 
> backend.backend_input_files.add(mozpath.join(config.topobjdir, 'config.status'))
> 
> However, since config.status doesn't exist if you just do successive runs of 'mach python-test', it will try to rebuild the backend every time you run the command. I'm not sure if that means we can't do that as a solution or not. What are your thoughts? Is it worth trying just adding TestManifestBackend as one of the default generated backends again?

Yeah, when adding 'config.status' it means we'll always re-generate the backend when running without an objdir.. Though because there are very few moz.build files being processed in this case, it's pretty quick (~1-2 seconds). I think given the two choices I'd prefer just adding TestManifestBackend back into the build, as that feels cleaner (but is also more work).

How should we do this? By creating a 'TestManifest+RecursiveMake' backend? I'm not sure how this plays with the FasterMake+TestManifest backend.. Do we need to add TestManifest to every possible combination?

Maybe we should just add 'config.status' for now and file a follow-up?
(In reply to Andrew Halberstadt [:ahal] from comment #77)
> I tested locally, and simply adding `self.backend_input_files |=
> obj.context_all_paths` to the TestManifestBackend's `consume_object()` seems
> to work great. Is that an acceptable workaround?

Good idea, that's a much simpler fix. I don't see what we'd be missing by doing it that way.
(In reply to Andrew Halberstadt [:ahal] from comment #78)
> Yeah, when adding 'config.status' it means we'll always re-generate the
> backend when running without an objdir.. Though because there are very few
> moz.build files being processed in this case, it's pretty quick (~1-2
> seconds). I think given the two choices I'd prefer just adding
> TestManifestBackend back into the build, as that feels cleaner (but is also
> more work).

Looking into the config.status path some more, the reason we always rebuild when it is not present is because of the way I implemented missing files in bug 1242663. However, I think an alternate solution there would be to use $(wildcard) instead of creating dummy rules on every file. so if our dependency file contains:

config.status
foo
bar

The current rules do:

backend.TestManifestBackend: config.status foo bar
    re-create backend
config.status foo bar:

Since config.status is missing, make runs the empty rule to try to create it (which does nothing), but that triggers the backend.TestManifestBackend rule. With wildcard this would look like:

backend.TestManifestBackend: $(wildcard config.status foo bar)
    re-create backend

Which means make only checks dependencies on the files that actually exist (presumably foo and bar in this case, but not config.status). But when config.status is later created, we'll know to recreate the TestManifestBackend then. This patch seems to do what we want here (along with manually adding config.status to the backend_input_files):

diff --git a/build/rebuild-backend.mk b/build/rebuild-backend.mk
index c93e422..a1c64c0 100644
--- a/build/rebuild-backend.mk
+++ b/build/rebuild-backend.mk
@@ -25,9 +25,7 @@ $(subst .,%,$(BUILD_BACKEND_FILES)):
 	$(PYTHON) $(BACKEND_GENERATION_SCRIPT)
 
 define build_backend_rule
-$(1)_files := $$(shell cat $(1).in)
-$(1): $$($(1)_files)
-$$($(1)_files):
+$(1): $$(wildcard $$(shell cat $(1).in))
 
 endef
 $(foreach file,$(BUILD_BACKEND_FILES),$(eval $(call build_backend_rule,$(file))))

glandium: do you see any issues with this approach?

ahal, can you double-check that this fixes the issue you're seeing?

> How should we do this? By creating a 'TestManifest+RecursiveMake' backend?
> I'm not sure how this plays with the FasterMake+TestManifest backend.. Do we
> need to add TestManifest to every possible combination?

That would be best answered by glandium as well. Though if there aren't any issues with changing the backend dependencies to a wildcard, IMO that's the best way forward.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8831701 [details]
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests

https://reviewboard.mozilla.org/r/108248/#review112112

> By moving these tests inside the MOZ_BUILD_APP conditional, that prevents us from running it easily as a separate task along with the rest of the tests, right?
> 
> If so, please file a followup bug. I think most of these tests can work without an objdir if we just stub out the buildconfig module, since they generally rely on that only for topsrcdir / topobjdir. (Feel free to assign it to me)

Correct, filed bug 1338415
(In reply to Michael Shal [:mshal] from comment #80)
> diff --git a/build/rebuild-backend.mk b/build/rebuild-backend.mk
> index c93e422..a1c64c0 100644
> --- a/build/rebuild-backend.mk
> +++ b/build/rebuild-backend.mk
> @@ -25,9 +25,7 @@ $(subst .,%,$(BUILD_BACKEND_FILES)):
>  	$(PYTHON) $(BACKEND_GENERATION_SCRIPT)
>  
>  define build_backend_rule
> -$(1)_files := $$(shell cat $(1).in)
> -$(1): $$($(1)_files)
> -$$($(1)_files):
> +$(1): $$(wildcard $$(shell cat $(1).in))
>  
>  endef
>  $(foreach file,$(BUILD_BACKEND_FILES),$(eval $(call
> build_backend_rule,$(file))))
> 
> glandium: do you see any issues with this approach?
> 
> ahal, can you double-check that this fixes the issue you're seeing?

Thanks, this works great! It solves both the edge case in comment 69 and doesn't repeatedly regenerate the backend without an objdir. Should I add 'config.status' as an input file in the TestManifestBackend class itself, or in the gen_test_manifest.py script?
(In reply to Andrew Halberstadt [:ahal] from comment #82)
> Thanks, this works great! It solves both the edge case in comment 69 and
> doesn't repeatedly regenerate the backend without an objdir. Should I add
> 'config.status' as an input file in the TestManifestBackend class itself, or
> in the gen_test_manifest.py script?

I tested it locally by adding it to gen_test_manifest.py, but I think TestManifestBackend makes more sense because it already does things with backend_input_files.
Let's just treat this as a review instead of needinfo.

Mike, hope you don't mind but I uploaded that change in your name (feel free to make sure I didn't sneak anything in ;)).. Also some of the changes we talked about (like adding dependencies on moz.build and config.status files) got amended to the review that gps already r+'ed. So please take a quick look at the latest interdiff or two (and keep in mind that mozreview sucks at rebases + interdiffs, so the obviously unrelated stuff isn't related).
Flags: needinfo?(mh+mozilla)
(And should have specified.. but Mike refers to mshal in the last comment)
Comment on attachment 8831701 [details]
Bug 1320194 - Generate all-tests.pkl and related files when resolving tests

https://reviewboard.mozilla.org/r/108248/#review113384

Looks great! Thanks for seeing this through. The other patch looks fine with the config.status addition too, though I wish we had a more automated way to handle that.
Attachment #8831701 - Flags: review?(mshal) → review+
Comment on attachment 8836223 [details]
Bug 1320194 - Use 'wildcard' instead of dummy rules when re-generating build backends,

https://reviewboard.mozilla.org/r/111692/#review113986

This opens the door to a fix for bug 910655 if we listed all the possible Makefiles in the backend file being read. Whether that's worth doing or not is a discussion to have in that bug.
Attachment #8836223 - Flags: review?(mh+mozilla) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03804db09cf3
Use 'wildcard' instead of dummy rules when re-generating build backends, r=glandium
https://hg.mozilla.org/integration/autoland/rev/c07984424f62
Refactor test metadata related backend code into a partial TestManifestBackend r=gps
https://hg.mozilla.org/integration/autoland/rev/0fc91140e941
Fix bug preventing more than two backends in a HybridBackend r=gps
https://hg.mozilla.org/integration/autoland/rev/b5bcf4edfbb0
Add ability to specify custom emitter function in TreeMetadataEmitter r=gps
https://hg.mozilla.org/integration/autoland/rev/b4344af7fc7c
Generate all-tests.pkl and related files when resolving tests r=mshal
Blocks: 1340162
Depends on: 1341502
Depends on: 1342937
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: