Misc mozbuild and client.mk cleanup

RESOLVED FIXED in Firefox 58

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(11 attachments)

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
Assignee

Description

2 years ago
Misc patches to move us along towards removing client.mk.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
mozreview-review
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 24

2 years ago
mozreview-review
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 25

2 years ago
mozreview-review
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 26

2 years ago
mozreview-review
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 27

2 years ago
mozreview-review
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 28

2 years ago
mozreview-review
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 29

2 years ago
mozreview-review
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 30

2 years ago
mozreview-review
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 31

2 years ago
mozreview-review
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 32

2 years ago
mozreview-review
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 33

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 45

2 years ago
mozreview-review-reply
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.
Assignee

Comment 46

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 57

2 years ago
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

Comment 58

2 years ago
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bed4a0ce05ba
Backed out changesets f47abc8062fc, 73df5a910488, 75807d1ab22e, and 53a93e195f91 ; a=bustage

Comment 59

2 years ago
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad5833b33090
Backed out changeset c6c4fb4b0fef ; a=bustage
Assignee

Comment 60

2 years ago
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)

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.