Closed Bug 1416052 Opened 2 years ago Closed 2 years ago

Move some environment setup logic from client.mk to Python

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 1 obsolete file)

59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
59 bytes, text/x-review-board-request
nalexander
: review+
Details
No description provided.
Comment on attachment 8927210 [details]
Bug 1416052 - Move config.guess logic to Python;

https://reviewboard.mozilla.org/r/198488/#review203840

::: python/mozbuild/mozbuild/controller/building.py:1324
(Diff revision 1)
>      def _run_client_mk(self, target=None, line_handler=None, jobs=0,
>                         verbose=None, keep_going=False, append_env=None):
>          append_env = dict(append_env or {})
>          append_env['TOPSRCDIR'] = self.topsrcdir
>  
> +        append_env['CONFIG_GUESS'] = self.resolve_config_guess()

Does this supercede https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#681?
Attachment #8927210 - Flags: review+
Comment on attachment 8927212 [details]
Bug 1416052 - Generate a make file to include in client.mk;

https://reviewboard.mozilla.org/r/198492/#review203844

::: client.mk:52
(Diff revision 1)
>  # As $(shell) doesn't preserve newlines, use sed to replace them with an
>  # unlikely sequence (||), which is then replaced back to newlines by make
>  # before evaluation. $(shell) replacing newlines with spaces, || is always
>  # followed by a space (since sed doesn't remove newlines), except on the
>  # last line, so replace both '|| ' and '||'.
> -MOZCONFIG_CONTENT := $(subst ||,$(CR),$(subst || ,$(CR),$(shell $(TOPSRCDIR)/mach environment --format=client.mk | sed 's/$$/||/')))
> +MOZCONFIG_CONTENT := $(subst ||,$(CR),$(subst || ,$(CR),$(shell cat $(OBJDIR)/.mozconfig-client-mk | sed 's/$$/||/')))

Oh hai!  I was going to say that `--format=client.mk` could be removed... and I bet you do it later.  If you don't, here's a reminder to do so soon.

::: client.mk:52
(Diff revision 1)
>  # As $(shell) doesn't preserve newlines, use sed to replace them with an
>  # unlikely sequence (||), which is then replaced back to newlines by make
>  # before evaluation. $(shell) replacing newlines with spaces, || is always
>  # followed by a space (since sed doesn't remove newlines), except on the
>  # last line, so replace both '|| ' and '||'.
> -MOZCONFIG_CONTENT := $(subst ||,$(CR),$(subst || ,$(CR),$(shell $(TOPSRCDIR)/mach environment --format=client.mk | sed 's/$$/||/')))
> +MOZCONFIG_CONTENT := $(subst ||,$(CR),$(subst || ,$(CR),$(shell cat $(OBJDIR)/.mozconfig-client-mk | sed 's/$$/||/')))

Keeping `MOZCONFIG_CONTENT` is pretty hairy.  Can we simplify the piping here and below by re-reading the produced file, or creating two special-purpose files on the Python side of the house?

It looks like this is correct, but it's pretty hard to follow.

::: client.mk:52
(Diff revision 1)
>  # As $(shell) doesn't preserve newlines, use sed to replace them with an
>  # unlikely sequence (||), which is then replaced back to newlines by make
>  # before evaluation. $(shell) replacing newlines with spaces, || is always
>  # followed by a space (since sed doesn't remove newlines), except on the
>  # last line, so replace both '|| ' and '||'.
> -MOZCONFIG_CONTENT := $(subst ||,$(CR),$(subst || ,$(CR),$(shell $(TOPSRCDIR)/mach environment --format=client.mk | sed 's/$$/||/')))
> +MOZCONFIG_CONTENT := $(subst ||,$(CR),$(subst || ,$(CR),$(shell cat $(OBJDIR)/.mozconfig-client-mk | sed 's/$$/||/')))

Keeping `MOZCONFIG_CONTENT` is pretty hairy.  Can we simplify the piping here and below by re-reading the produced file, or creating two special-purpose files on the Python side of the house?

It looks like this is correct, but it's pretty hard to follow.
Attachment #8927212 - Flags: review+
Comment on attachment 8927214 [details]
Bug 1416052 - Remove comment filtering;

https://reviewboard.mozilla.org/r/198496/#review203848

I assume that all of the code that might produce a commented line is short-lived, so not worth commenting to document this new requirement?  Also, the included file doesn't inject user-provided values that could include comments, does it?
Attachment #8927214 - Flags: review+
Comment on attachment 8927215 [details]
Bug 1416052 - Remove mkdir of objdir from client.mk;

https://reviewboard.mozilla.org/r/198498/#review203850

Is it possible those included Makefiles were dependend on transitively?  I don't recall specifically asking for Makefiles when I've used `mkdir_deps`.
Attachment #8927215 - Flags: review+
Comment on attachment 8927217 [details]
Bug 1416052 - Check clobber state from Python;

https://reviewboard.mozilla.org/r/198502/#review203854

::: python/mozbuild/mozbuild/controller/building.py:1342
(Diff revision 1)
>          if 'mingw' in append_env['CONFIG_GUESS']:
>              append_env['HOST_OS_ARCH'] = 'WINNT'
>  
>          mozconfig = self.mozconfig
> +
> +        # Check clobber state.

Can we push this to a helper?  It's not going to disappear with `client.mk`, and this file is _huge_... although I guess this function isn't so large.

::: python/mozbuild/mozbuild/controller/building.py:1346
(Diff revision 1)
> +
> +        # Check clobber state.
> +        auto_clobber = any([
> +            os.environ.get('AUTOCLOBBER', False),
> +            mozconfig['env']['added'].get('AUTOCLOBBER', False),
> +            'AUTOCLOBBER=1' in mozconfig['make_extra'] or [],

Consider parenthesizing this, 'cuz I sure don't know if `in` binds more tightly than `or`.
Attachment #8927217 - Flags: review+
Comment on attachment 8927218 [details]
Bug 1416052 - Remove build_and_deploy target;

https://reviewboard.mozilla.org/r/198504/#review203858

All Fennec developers have internalized |mach build && mach package|.
Attachment #8927218 - Flags: review+
Comment on attachment 8927210 [details]
Bug 1416052 - Move config.guess logic to Python;

https://reviewboard.mozilla.org/r/198488/#review203840

> Does this supercede https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#681?

Oh, hmmm. It certainly looks like client.mk's invocation of baseconfig.mk is redundant with `_make_path()`. So I may just nuke that code from client.mk! Good observation.
Comment on attachment 8927212 [details]
Bug 1416052 - Generate a make file to include in client.mk;

https://reviewboard.mozilla.org/r/198492/#review203844

> Keeping `MOZCONFIG_CONTENT` is pretty hairy.  Can we simplify the piping here and below by re-reading the produced file, or creating two special-purpose files on the Python side of the house?
> 
> It looks like this is correct, but it's pretty hard to follow.

I intend to clean this up drastically later. End state is likely - as you guessed it - doing everything on the Python side. I'm very much taking the microcommit approach so subtle change in behavior isn't missed during review and is easier to bisect should regressions arise.
Comment on attachment 8927214 [details]
Bug 1416052 - Remove comment filtering;

https://reviewboard.mozilla.org/r/198496/#review203848

Yes, assume all this code is short-lived and the Python side will be refactored in the near future once client.mk is gone.

I'll update the commit message to reflect the small possibility of a multi-line value containing comments.
Comment on attachment 8927215 [details]
Bug 1416052 - Remove mkdir of objdir from client.mk;

https://reviewboard.mozilla.org/r/198498/#review203850

When I wrote this commit, I looked at all ``$(`` patterns in client.mk, looking for functions or variables that may be defined in the removed files. I didn't see anything obvious. I also didn't see any side-effects in these make files that should impact the build backend's `make` environment.
Comment on attachment 8927217 [details]
Bug 1416052 - Check clobber state from Python;

https://reviewboard.mozilla.org/r/198502/#review203854

> Can we push this to a helper?  It's not going to disappear with `client.mk`, and this file is _huge_... although I guess this function isn't so large.

Yes, this file is large. I intend to refactor things later. I hesitate to do it now because I'm not sure what the final state will be like. I'm not sure what the final state will be like because I haven't written those patches yet. But rest assured, there will be a refactor after client.mk goes away.
Attachment #8928011 - Flags: review?(core-build-config-reviews)
Blocks: 1417264
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13e44d942d70
Add a wrapper to invoke client.mk; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/7ffce7d50018
Pass TOPSRCDIR into client.mk; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/a88781c8655a
Remove make validation from client.mk; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/ee45241c58eb
Move config.guess logic to Python; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/4a26fbabb7d8
Remove references to MOZ_CURRENT_PROJECT; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/67c03dc70bfd
Generate a make file to include in client.mk; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/45d15eccf550
Move export of FOUND_MOZCONFIG to Python; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/1cd169ff8a0d
Remove comment filtering; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/d05549af9bf7
Remove mkdir of objdir from client.mk; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/51d998c76d9a
Remove reference to undefined BUILD_PROJECT_ARG; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/b6adf66f34c6
Check clobber state from Python; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/2a90d49c5573
Remove build_and_deploy target; r=nalexander
https://hg.mozilla.org/integration/autoland/rev/d7c5faf9f301
Remove OBJDIR_TARGETS; r=nalexander
Blocks: 1417414
Blocks: 1417387
Looks like we improved some build times:

== Change summary for alert #10764 (as of Tue, 14 Nov 2017 23:10:05 GMT) ==

Improvements:

 13%  build times linux64 opt artifact taskcluster-c4.4xlarge     124.61 -> 108.74

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10764
Attachment #8927099 - Attachment is obsolete: true
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.