1416052 - Move some environment setup logic from client.mk to Python

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Add a wrapper to invoke client.mk;

We're about to start moving logic from client.mk into Python.

The transition will require doing things like setting
environment variables when we invoke client.mk. Prepare for
this by factoring the "call client.mk" logic into a reusable
function.

The function will obviously be temporary until client.mk is
no more. I'm not sure what the end state will be yet. But surely
having equivalent code in Python is better than having it in
make.

Differential Revision: https://phabricator.services.mozilla.com/D214

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

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Pass TOPSRCDIR into client.mk;

Differential Revision: https://phabricator.services.mozilla.com/D215

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Move config.guess logic to Python;

Instead of evaluating config.guess in client.mk, we evaluate it
in Python. The Python code also looks for CONFIG_GUESS in the
mozconfig. This still happens in client.mk courtesy of evaling
the mozconfig's relevant parts.

Since it is related, we also set HOST_OS_ARCH in the environment.

A side-effect of this change is that HOST_OS_ARCH will now be
defined for the entirety of build process tree rather than just
client.mk. I /think/ this is fine, as configure will define
HOST_OS_ARCH and its value will override the environment variable.

Differential Revision: https://phabricator.services.mozilla.com/D216

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove references to MOZ_CURRENT_PROJECT;

Actual uses of this variable were removed by 5fb427c50ca3
(bug 1412431). This is dead code.

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

Comment 5

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Generate a make file to include in client.mk;

Currently, client.mk calls `mach environment` to obtain a make file
to be evaluated in the context of client.mk. The reason it is
implemented this way is because client.mk could be an entrypoint to
the build system.

With recent changes that require the use of mach to use client.mk,
we are now guaranteed to have Python code running before client.mk
is invoked. This means we don't need to invoke `mach` from client.mk.

This commit ports the code for generating a client.mk suitable make
file from `mach environment` to the build dispatcher. We now write out
a new .mozconfig-client-mk file in the objdir. client.mk is changed
to cat this file and to include it as a native make file.

The OBJDIR environment variable is also set so client.mk knows where
to read the auto-generated file from.

This commit should be backwards compatible.

Hopefully it is obvious, but this new make file is only temporary.
As soon as the remaining mozconfig logic is moved out of client.mk,
we should be able to simplify down to a single "include" in client.mk.

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

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Move export of FOUND_MOZCONFIG to Python;

This should have the same net result.

TBH, I'm not 100% convinced we need this export. It is only needed
to send variables to sub-makes. And the only make file reading
FOUND_MOZCONFIG is client.mk. Since the code that evals the
auto-generated make file is always executed in client.mk, we
shouldn't need this export.

All this code is going away soon anyway. So I'm inclined to cargo
cult this just in case.

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

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove comment filtering;

The auto-generated make file that we include (and the `mach environment`
output that we included before that) should not contain comment lines.
I think it is safe to remove the code that filters them out.

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

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove mkdir of objdir from client.mk;

Now that mach is being used to invoke client.mk, we can perform
objdir directory creation there.

Removing the use of mkdir_deps meant that we could remove some
included make files.

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

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove reference to undefined BUILD_PROJECT_ARG;

I'm pretty sure this was related to the now removed feature for
building multiple projects.

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

Comment 10

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Check clobber state from Python;

The clobber logic is already written in Python. Now that we
always use mach in front of client.mk, we can check the clobber
state before we execute client.mk.

Since we always check the clobber state, we can remove the
CLOBBER files from various dependencies in client.mk. The
clobberer code should ensure everything is in a good state.

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

Comment 11

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove build_and_deploy target;

This was added before mach AFAICT. If people need this functionality,
we can add a mach command.

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

Comment 12

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove OBJDIR_TARGETS;

Now that we require mach to run client.mk and mach only invokes
client.mk for full builds (the "build" target) and configure
(the "configure" target), we should no longer have anyone using
client.mk as a proxy to make targets in the build backend.

So remove that feature.

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

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8927208 [details]
Bug 1416052 - Add a wrapper to invoke client.mk;

https://reviewboard.mozilla.org/r/198484/#review203836

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8927209 [details]
Bug 1416052 - Pass TOPSRCDIR into client.mk;

https://reviewboard.mozilla.org/r/198486/#review203838

Comment 15

[Nick Alexander :nalexander [he/him]]

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?

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8927211 [details]
Bug 1416052 - Remove references to MOZ_CURRENT_PROJECT;

https://reviewboard.mozilla.org/r/198490/#review203842

Comment 17

[Nick Alexander :nalexander [he/him]]

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.

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8927213 [details]
Bug 1416052 - Move export of FOUND_MOZCONFIG to Python;

https://reviewboard.mozilla.org/r/198494/#review203846

Comment 19

[Nick Alexander :nalexander [he/him]]

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?

Comment 20

[Nick Alexander :nalexander [he/him]]

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`.

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8927216 [details]
Bug 1416052 - Remove reference to undefined BUILD_PROJECT_ARG;

https://reviewboard.mozilla.org/r/198500/#review203852

Comment 22

[Nick Alexander :nalexander [he/him]]

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`.

Comment 23

[Nick Alexander :nalexander [he/him]]

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|.

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8927219 [details]
Bug 1416052 - Remove OBJDIR_TARGETS;

https://reviewboard.mozilla.org/r/198506/#review203862

WAT

Comment 25

[Gregory Szorc [:gps]]

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 26

[Gregory Szorc [:gps]]

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 27

[Gregory Szorc [:gps]]

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 28

[Gregory Szorc [:gps]]

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 29

[Gregory Szorc [:gps]]

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.

Comment 30

[Gregory Szorc [:gps]]

Attachment: Bug 1416052 - Remove make validation from client.mk;

MozbuildObject._run_make() also invokes baseconfig.mk as part of
validating the make binary that will be executed.

Since mach calls this Python code and mach is now required to
invoke client.mk, the validation in client.mk is redundant and can
be removed.

autoconf.mk includes baseconfig.mk. So even once we eliminate
client.mk, the make backend itself will still validate make.
We should probably move this validation to moz.configure (if it
isn't already there). But that can be done another time.

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

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8927208 [details]
Bug 1416052 - Add a wrapper to invoke client.mk;

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

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8927209 [details]
Bug 1416052 - Pass TOPSRCDIR into client.mk;

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

Comment 33

[Gregory Szorc [:gps]]

Comment on attachment 8927210 [details]
Bug 1416052 - Move config.guess logic to Python;

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

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8927211 [details]
Bug 1416052 - Remove references to MOZ_CURRENT_PROJECT;

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

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8927212 [details]
Bug 1416052 - Generate a make file to include in client.mk;

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

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8927213 [details]
Bug 1416052 - Move export of FOUND_MOZCONFIG to Python;

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

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8927214 [details]
Bug 1416052 - Remove comment filtering;

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

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8927215 [details]
Bug 1416052 - Remove mkdir of objdir from client.mk;

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

Comment 39

[Gregory Szorc [:gps]]

Comment on attachment 8927216 [details]
Bug 1416052 - Remove reference to undefined BUILD_PROJECT_ARG;

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

Comment 40

[Gregory Szorc [:gps]]

Comment on attachment 8927217 [details]
Bug 1416052 - Check clobber state from Python;

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

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8927218 [details]
Bug 1416052 - Remove build_and_deploy target;

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

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8927219 [details]
Bug 1416052 - Remove OBJDIR_TARGETS;

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

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8927217 [details]
Bug 1416052 - Check clobber state from Python;

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

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8927218 [details]
Bug 1416052 - Remove build_and_deploy target;

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

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8927219 [details]
Bug 1416052 - Remove OBJDIR_TARGETS;

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

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8928011 [details]
Bug 1416052 - Remove make validation from client.mk;

https://reviewboard.mozilla.org/r/199246/#review204616

Comment 47

[Pulsebot]

Pushed by [e-mail]:
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

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b19b455ae87a
Fixup for test_clobber.py; r=me

Comment 49

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/13e44d942d70
https://hg.mozilla.org/mozilla-central/rev/7ffce7d50018
https://hg.mozilla.org/mozilla-central/rev/a88781c8655a
https://hg.mozilla.org/mozilla-central/rev/ee45241c58eb
https://hg.mozilla.org/mozilla-central/rev/4a26fbabb7d8
https://hg.mozilla.org/mozilla-central/rev/67c03dc70bfd
https://hg.mozilla.org/mozilla-central/rev/45d15eccf550
https://hg.mozilla.org/mozilla-central/rev/1cd169ff8a0d
https://hg.mozilla.org/mozilla-central/rev/d05549af9bf7
https://hg.mozilla.org/mozilla-central/rev/51d998c76d9a
https://hg.mozilla.org/mozilla-central/rev/b6adf66f34c6
https://hg.mozilla.org/mozilla-central/rev/2a90d49c5573
https://hg.mozilla.org/mozilla-central/rev/d7c5faf9f301
https://hg.mozilla.org/mozilla-central/rev/b19b455ae87a

Comment 50

[Ionuț Goldan [:igoldan]]

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