1415614 - Move Python code for invoking build system out of mach_commands.py

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1415614 - Don't assign mach logger to instance variable;

We don't use this attribute outside of this function. Besides, you can
easily obtain a handle on the logger by calling
``logging.getLogger('mach')``.

Review commit: https://reviewboard.mozilla.org/r/197838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197838/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1415614 - Add an API to log all structured messages;

Currently, marking a logger as a structured logger will require a
subsequent function call in order for the logger to be hooked up
to active handlers. This behavior is not intuitive and makes it
easy to not have handlers for newly-registered loggers. This means
messages may not be logged anywhere.

In addition, we have to manually specify which named loggers to
enable structured logging for. This can be annoying.

We change the behavior of register_structured_logger() to
automatically add existing terminal and json handlers to the
logger being marked as structured.

We also introduce an API to enable structured logging for all
loggers. Existing consumers of registered_structured_logger()
in mozbuild have been updated to use this API. A new consumer
has been added for the `mach configure` command because it should
have been there before.

Review commit: https://reviewboard.mozilla.org/r/197840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197840/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1415614 - Move build output processing code out of mach_commands.py;

In general, we shouldn't have so much business logic in mach_commands.py
files.

I'd like to move more of the low-level "perform a build" logic out of
mach_commands.py to facilitate some future work. That code makes use of
this output processing code. So as the first step in moving the build
code, we move its dependencies.

As part of this, I also cleaned up some random unused imports around
lines that were touched.

No meaningful code changes were performed as part of the code move.

Review commit: https://reviewboard.mozilla.org/r/197842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197842/

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1415614 - Move code for invoking build system out of mach_commands.py;

The goal is to move this lower-level code somewhere where it can be
reused.

The only meaningful code change as part of the move was handling
of the mach context. Before, we could access self._mach_context.
After, we need to pass it as an argument. This also uncovered a bug
where "telemetry_handler" could be None but we called it
unconditionally. We now guard this call with an "if."

Review commit: https://reviewboard.mozilla.org/r/197844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197844/

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1415614 - Move configure invocation out of mach_commands.py;

While we're here, also move the low-level code to invoke configure
to a proper Python module.

Review commit: https://reviewboard.mozilla.org/r/197846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197846/

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8926618 [details]
Bug 1415614 - Don't assign mach logger to instance variable;

https://reviewboard.mozilla.org/r/197838/#review203136

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8926619 [details]
Bug 1415614 - Add an API to log all structured messages;

https://reviewboard.mozilla.org/r/197840/#review203138

r- just for me to look again.  Explain why:
- the existing logging in http://searchfox.org/mozilla-central/source/python/mach/mach/main.py#438 is separate
- we require commands to "opt in" to this type of structured logging.  It's almost as easy to forget `enable_all_structured_loggers` as it was to get the old incantations wrong.  Can we not make that just be part of `mach`?  Like, every command is ready to create/pass through structured logs?

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8926620 [details]
Bug 1415614 - Move build output processing code out of mach_commands.py;

https://reviewboard.mozilla.org/r/197842/#review203140

lgtm... but the file you added the code to is now 1k lines!  Consider pushing it to a new file.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8926621 [details]
Bug 1415614 - Move code for invoking build system out of mach_commands.py;

https://reviewboard.mozilla.org/r/197844/#review203142

Same comment about large files.

::: python/mozbuild/mozbuild/controller/building.py:1227
(Diff revision 1)
> +            #    print(EXCESSIVE_SWAP_MESSAGE)
> +
> +            print('To view resource usage of the build, run |mach '
> +                'resource-usage|.')
> +
> +            telemetry_handler = getattr(mach_context,

Consider pushing this out into a separate function/module, and not invoking it if we're not going to send it to `telemetry_handler`.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8926622 [details]
Bug 1415614 - Move configure invocation out of mach_commands.py;

https://reviewboard.mozilla.org/r/197846/#review203144

stamp!

::: python/mozbuild/mozbuild/controller/building.py:1297
(Diff revision 1)
> +        status = self._run_make(srcdir=True, filename='client.mk',
> +            target='configure', line_handler=line_handler, log=False,
> +            print_directory=False, allow_parallel=False, ensure_exit_code=False,
> +            append_env=append_env)
> +
> +        if not status:

Are these lines likely to be meaningful to other consumers of the driver?

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8926618 [details]
Bug 1415614 - Don't assign mach logger to instance variable;

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

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8926619 [details]
Bug 1415614 - Add an API to log all structured messages;

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

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8926620 [details]
Bug 1415614 - Move build output processing code out of mach_commands.py;

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

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8926621 [details]
Bug 1415614 - Move code for invoking build system out of mach_commands.py;

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

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8926622 [details]
Bug 1415614 - Move configure invocation out of mach_commands.py;

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8926622 [details]
Bug 1415614 - Move configure invocation out of mach_commands.py;

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

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 8926619 [details]
Bug 1415614 - Add an API to log all structured messages;

https://reviewboard.mozilla.org/r/197840/#review203514

::: python/mach/mach/logging.py:294
(Diff revision 2)
> +
> +            if json:
> +                for handler in self.json_handlers:
> +                    logger.removeHandler(handler)
> +
> +        # Now add requested handlers to root logger.

This through line 304 could just do:

self.structured_loggers = []
self.register_structured_logger(self.root_logger, terminal, json)

That would avoid the redundant terminal / json handling code.

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8926619 [details]
Bug 1415614 - Add an API to log all structured messages;

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8926620 [details]
Bug 1415614 - Move build output processing code out of mach_commands.py;

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

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8926621 [details]
Bug 1415614 - Move code for invoking build system out of mach_commands.py;

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

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8926622 [details]
Bug 1415614 - Move configure invocation out of mach_commands.py;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197846/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e99a07820f59
Don't assign mach logger to instance variable; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/ab3c555c62ea
Add an API to log all structured messages; r=mshal
https://hg.mozilla.org/integration/autoland/rev/36302dd731b3
Move build output processing code out of mach_commands.py; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/fe26063c2885
Move code for invoking build system out of mach_commands.py; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2f0bc3e7c2eb
Move configure invocation out of mach_commands.py; r=nalexander

Comment 23

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e99a07820f59
https://hg.mozilla.org/mozilla-central/rev/ab3c555c62ea
https://hg.mozilla.org/mozilla-central/rev/36302dd731b3
https://hg.mozilla.org/mozilla-central/rev/fe26063c2885
https://hg.mozilla.org/mozilla-central/rev/2f0bc3e7c2eb