Closed Bug 1412460 Opened 3 years ago Closed 3 years ago

Misc mozbuild and client.mk cleanup

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(11 files)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
Misc patches to move us along towards removing client.mk.
Comment on attachment 8923035 [details]
Bug 1412460 - Move build output processing code out of mach_commands.py;

https://reviewboard.mozilla.org/r/194240/#review200640
Attachment #8923035 - Flags: review+
Attachment #8923035 - Flags: review?(core-build-config-reviews)
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
Attachment #8923036 - Flags: review+
Attachment #8923036 - Flags: review?(core-build-config-reviews)
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?
Attachment #8923037 - Flags: review+
Attachment #8923037 - Flags: review?(core-build-config-reviews)
Comment on attachment 8923038 [details]
Bug 1412460 - Update client.mk comments to match reality;

https://reviewboard.mozilla.org/r/194246/#review200664
Attachment #8923038 - Flags: review+
Attachment #8923038 - Flags: review?(core-build-config-reviews)
Comment on attachment 8923039 [details]
Bug 1412460 - Remove redundant configure dependency checking in client.mk;

https://reviewboard.mozilla.org/r/194248/#review200670
Attachment #8923039 - Flags: review+
Attachment #8923039 - Flags: review?(core-build-config-reviews)
Comment on attachment 8923040 [details]
Bug 1412460 - Remove unused CONFIG_CACHE variable;

https://reviewboard.mozilla.org/r/194250/#review200672
Attachment #8923040 - Flags: review+
Attachment #8923040 - Flags: review?(core-build-config-reviews)
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'
Attachment #8923041 - Flags: review+
Attachment #8923041 - Flags: review?(core-build-config-reviews)
Comment on attachment 8923042 [details]
Bug 1412460 - Simplify config.guess invocation;

https://reviewboard.mozilla.org/r/194254/#review200676
Attachment #8923042 - Flags: review+
Attachment #8923042 - Flags: review?(core-build-config-reviews)
Comment on attachment 8923043 [details]
Bug 1412460 - Always evaluate config environment;

https://reviewboard.mozilla.org/r/194256/#review200684
Attachment #8923043 - Flags: review+
Attachment #8923043 - Flags: review?(core-build-config-reviews)
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.
Attachment #8923044 - Flags: review+
Attachment #8923044 - Flags: review?(core-build-config-reviews)
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
Attachment #8923045 - Flags: review+
Attachment #8923045 - Flags: review?(core-build-config-reviews)
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 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.
Pushed by gszorc@mozilla.com:
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
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bed4a0ce05ba
Backed out changesets f47abc8062fc, 73df5a910488, 75807d1ab22e, and 53a93e195f91 ; a=bustage
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad5833b33090
Backed out changeset c6c4fb4b0fef ; a=bustage
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.
Attachment #8923040 - Flags: review?(core-build-config-reviews)
Attachment #8923041 - Flags: review?(core-build-config-reviews)
Attachment #8923042 - Flags: review?(core-build-config-reviews)
Attachment #8923044 - Flags: review?(core-build-config-reviews)
Attachment #8923045 - Flags: review?(core-build-config-reviews)
Attachment #8923038 - Flags: review?(core-build-config-reviews)
Attachment #8923043 - Flags: review?(core-build-config-reviews)
Attachment #8923039 - Flags: review?(core-build-config-reviews)
Attachment #8923037 - Flags: review?(core-build-config-reviews)
Attachment #8923036 - Flags: review?(core-build-config-reviews)
Attachment #8923035 - Flags: review?(core-build-config-reviews)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.