Closed Bug 1415614 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

No description provided.
Assignee: nobody → gps
Status: NEW → ASSIGNED
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?
Attachment #8926619 - Flags: review-
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.
Attachment #8926620 - Flags: review+
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`.
Attachment #8926621 - Flags: review+
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?
Attachment #8926622 - Flags: review+
Attachment #8926618 - Flags: review?(core-build-config-reviews)
Attachment #8926619 - Flags: review?(core-build-config-reviews)
Attachment #8926620 - Flags: review?(core-build-config-reviews)
Attachment #8926621 - Flags: review?(core-build-config-reviews)
Attachment #8926622 - Flags: review?(core-build-config-reviews)
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.
Attachment #8926619 - Flags: review+
Attachment #8926619 - Flags: review?(core-build-config-reviews)
Attachment #8926619 - Flags: review?(core-build-config-reviews)
Pushed by gszorc@mozilla.com:
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.