1412460 - Misc mozbuild and client.mk cleanup

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

Misc patches to move us along towards removing client.mk.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - 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/194240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194240/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - 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/194242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194242/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - 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.

We needed to muck around with the log manager because the underlying
logger for output changed from "mach" (which is registered by default)
to something under the "mozbuild" hierarchy.

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Update client.mk comments to match reality;

We don't want to encourage people to use client.mk.

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

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Remove redundant configure dependency checking in client.mk;

moz.configure invokes autoconf to generate old-configure. As part of this
it checks mtimes of dependencies. The logic in client.mk is old and
redundant. So remove it.

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

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Remove unused CONFIG_CACHE variable;

This likely became unused with the port to moz.configure.

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

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Don't write make variables in configure.d;

So the file can be more easily consumed without this variable defined.

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

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Simplify config.guess invocation;

The config.guess file is checked into source control. I'm not sure
under what conditions we wouldn't fail to find this file. So the
$(wildcard) shouldn't be necessary.

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

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Always evaluate config environment;

I /think/ the only scenario where we wouldn't be able to evaluate
a config environment after configure was the case where
MOZ_BUILD_PROJECTS was in play. Since we removed support for that
feature, let's drop support for treating the config environment
as optional.

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

Comment 10

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Remove everything target;

Our march towards nuking client.mk continues.

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

Comment 11

[Gregory Szorc [:gps]]

Attachment: Bug 1412460 - Remove targets related to VCS operations;

These targets no longer exist. I'm not sure when they were removed.
But this is obviously dead code.

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

Comment 12

[Gregory Szorc [:gps]]

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

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

Comment 13

[Gregory Szorc [:gps]]

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

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

Comment 14

[Gregory Szorc [:gps]]

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

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

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8923038 [details]
Bug 1412460 - Update client.mk comments to match reality;

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8923039 [details]
Bug 1412460 - Remove redundant configure dependency checking in client.mk;

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

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8923040 [details]
Bug 1412460 - Remove unused CONFIG_CACHE variable;

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

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8923041 [details]
Bug 1412460 - Don't write make variables in configure.d;

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

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8923042 [details]
Bug 1412460 - Simplify config.guess invocation;

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

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8923043 [details]
Bug 1412460 - Always evaluate config environment;

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

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8923044 [details]
Bug 1412460 - Remove everything target;

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

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8923045 [details]
Bug 1412460 - Remove targets related to VCS operations;

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

Comment 23

[Michael Shal [:mshal]]

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

https://reviewboard.mozilla.org/r/194240/#review200640

Comment 24

[Michael Shal [:mshal]]

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

https://reviewboard.mozilla.org/r/194242/#review200652

Comment 25

[Michael Shal [:mshal]]

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

https://reviewboard.mozilla.org/r/194244/#review200662

::: python/mozbuild/mozbuild/mach_commands.py:179
(Diff revision 2)
> -        line_handler = line_handler or on_line
> -
> -        options = ' '.join(shell_quote(o) for o in options or ())
> -        append_env = {b'CONFIGURE_ARGS': options.encode('utf-8')}
>  
> -        # Only print build status messages when we have an active
> +        self.log_manager.register_structured_logger(logging.getLogger('mozbuild'))

I don't quite follow - why do we want to call this in mach_commands for configure() but in the BuildDriver for build()? Can't we do it in BuildDriver for both?

Comment 26

[Michael Shal [:mshal]]

Comment on attachment 8923038 [details]
Bug 1412460 - Update client.mk comments to match reality;

https://reviewboard.mozilla.org/r/194246/#review200664

Comment 27

[Michael Shal [:mshal]]

Comment on attachment 8923039 [details]
Bug 1412460 - Remove redundant configure dependency checking in client.mk;

https://reviewboard.mozilla.org/r/194248/#review200670

Comment 28

[Michael Shal [:mshal]]

Comment on attachment 8923040 [details]
Bug 1412460 - Remove unused CONFIG_CACHE variable;

https://reviewboard.mozilla.org/r/194250/#review200672

Comment 29

[Michael Shal [:mshal]]

Comment on attachment 8923041 [details]
Bug 1412460 - Don't write make variables in configure.d;

https://reviewboard.mozilla.org/r/194252/#review200674

::: commit-message-67eff:1
(Diff revision 2)
> +Bug 1412460 - Don't write make variable in configure.d; r?build

nit: 'make variables', or 'a make variable'

Comment 30

[Michael Shal [:mshal]]

Comment on attachment 8923042 [details]
Bug 1412460 - Simplify config.guess invocation;

https://reviewboard.mozilla.org/r/194254/#review200676

Comment 31

[Michael Shal [:mshal]]

Comment on attachment 8923043 [details]
Bug 1412460 - Always evaluate config environment;

https://reviewboard.mozilla.org/r/194256/#review200684

Comment 32

[Michael Shal [:mshal]]

Comment on attachment 8923044 [details]
Bug 1412460 - Remove everything target;

https://reviewboard.mozilla.org/r/194258/#review200686

We also have an everything target in rules.mk, which does almost the same thing. I've no idea if anyone uses it, though.

Comment 33

[Michael Shal [:mshal]]

Comment on attachment 8923045 [details]
Bug 1412460 - Remove targets related to VCS operations;

https://reviewboard.mozilla.org/r/194260/#review200688

::: client.mk:342
(Diff revision 2)
>  # This makefile doesn't support parallel execution. It does pass
>  # MOZ_MAKE_FLAGS to sub-make processes, so they will correctly execute
>  # in parallel.
>  .NOTPARALLEL:
>  
> -.PHONY: checkout \
> +.PHONY: \

We should also remove these lines:

https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/config/rules.mk#546 - 547

and 'checkout' from the PHONY list here:

https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/config/rules.mk#1551

Comment 34

[Gregory Szorc [:gps]]

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

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

Comment 35

[Gregory Szorc [:gps]]

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

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

Comment 36

[Gregory Szorc [:gps]]

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

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

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8923038 [details]
Bug 1412460 - Update client.mk comments to match reality;

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

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8923039 [details]
Bug 1412460 - Remove redundant configure dependency checking in client.mk;

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

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8923040 [details]
Bug 1412460 - Remove unused CONFIG_CACHE variable;

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

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8923041 [details]
Bug 1412460 - Don't write make variables in configure.d;

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

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8923042 [details]
Bug 1412460 - Simplify config.guess invocation;

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

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8923043 [details]
Bug 1412460 - Always evaluate config environment;

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

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8923044 [details]
Bug 1412460 - Remove everything target;

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

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8923045 [details]
Bug 1412460 - Remove targets related to VCS operations;

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

Comment 45

[Gregory Szorc [:gps]]

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

https://reviewboard.mozilla.org/r/194244/#review200662

> I don't quite follow - why do we want to call this in mach_commands for configure() but in the BuildDriver for build()? Can't we do it in BuildDriver for both?

I'll make this slightly better in the next version.

But we can't eliminate all code because ``build()`` is using some custom terminal output writer that replaces the built-in logger. The registering of the `mozbuild` logger we can definitely consolidate.

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8923044 [details]
Bug 1412460 - Remove everything target;

https://reviewboard.mozilla.org/r/194258/#review200686

Good catch.

I reckon we can clean this up another day. I'm getting distracted enough by all the random yaks I'm finding as part of killing client.mk.

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8923045 [details]
Bug 1412460 - Remove targets related to VCS operations;

https://reviewboard.mozilla.org/r/194260/#review200688

> We should also remove these lines:
> 
> https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/config/rules.mk#546 - 547
> 
> and 'checkout' from the PHONY list here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/config/rules.mk#1551

Good catch!

Comment 48

[Gregory Szorc [:gps]]

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

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

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8923038 [details]
Bug 1412460 - Update client.mk comments to match reality;

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

Comment 50

[Gregory Szorc [:gps]]

Comment on attachment 8923039 [details]
Bug 1412460 - Remove redundant configure dependency checking in client.mk;

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

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8923040 [details]
Bug 1412460 - Remove unused CONFIG_CACHE variable;

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

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8923041 [details]
Bug 1412460 - Don't write make variables in configure.d;

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

Comment 53

[Gregory Szorc [:gps]]

Comment on attachment 8923042 [details]
Bug 1412460 - Simplify config.guess invocation;

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

Comment 54

[Gregory Szorc [:gps]]

Comment on attachment 8923043 [details]
Bug 1412460 - Always evaluate config environment;

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

Comment 55

[Gregory Szorc [:gps]]

Comment on attachment 8923044 [details]
Bug 1412460 - Remove everything target;

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

Comment 56

[Gregory Szorc [:gps]]

Comment on attachment 8923045 [details]
Bug 1412460 - Remove targets related to VCS operations;

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

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53a93e195f91
Move build output processing code out of mach_commands.py; r=mshal
https://hg.mozilla.org/integration/autoland/rev/75807d1ab22e
Move code for invoking build system out of mach_commands.py; r=mshal
https://hg.mozilla.org/integration/autoland/rev/73df5a910488
Move configure invocation out of mach_commands.py; r=mshal
https://hg.mozilla.org/integration/autoland/rev/ad6cbaa591ae
Update client.mk comments to match reality; r=mshal
https://hg.mozilla.org/integration/autoland/rev/c6c4fb4b0fef
Remove redundant configure dependency checking in client.mk; r=mshal
https://hg.mozilla.org/integration/autoland/rev/579dabc132bf
Remove unused CONFIG_CACHE variable; r=mshal
https://hg.mozilla.org/integration/autoland/rev/027f1c5da98e
Don't write make variables in configure.d; r=mshal
https://hg.mozilla.org/integration/autoland/rev/e5329ac33130
Simplify config.guess invocation; r=mshal
https://hg.mozilla.org/integration/autoland/rev/f47abc8062fc
Always evaluate config environment; r=mshal
https://hg.mozilla.org/integration/autoland/rev/c0a8b021f29d
Remove everything target; r=mshal
https://hg.mozilla.org/integration/autoland/rev/de38db1d820b
Remove targets related to VCS operations; r=mshal

Comment 58

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bed4a0ce05ba
Backed out changesets f47abc8062fc, 73df5a910488, 75807d1ab22e, and 53a93e195f91 ; a=bustage

Comment 59

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad5833b33090
Backed out changeset c6c4fb4b0fef ; a=bustage

Comment 60

[Gregory Szorc [:gps]]

Somehow the Python refactor led to `mach build` output not getting printed in automation. How, I have no clue. So all those changes were backed out.

I also had to back out a client.mk change because it was causing local build failure. I guess I'll try to land these again in another bug.

Comment 61

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ad6cbaa591ae
https://hg.mozilla.org/mozilla-central/rev/579dabc132bf
https://hg.mozilla.org/mozilla-central/rev/027f1c5da98e
https://hg.mozilla.org/mozilla-central/rev/e5329ac33130
https://hg.mozilla.org/mozilla-central/rev/c0a8b021f29d
https://hg.mozilla.org/mozilla-central/rev/de38db1d820b