1417684 - Move logic for invoking configure into Python

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

Code for determining what `make` to use, validating it is sane, and determining additional flags to pass to make (MOZ_MAKE_FLAGS) is currently scattered around client.mk, base.py, and baseconfig.mk.

moz.configure has logic for resolving MAKE. But because moz.configure is run from client.mk and MAKE is set in the environment variable, moz.configure's logic essentially does nothing. And this fact is obscuring that the moz.configure logic is flat out wrong (it doesn't prioritize mozmake.exe on Windows for example).

Let's take a chainsaw to all this code and implement it correctly in moz.configure.

Comment 1

[Gregory Szorc [:gps]]

This turned into a rabbit hole. Everything is so tightly coupled that I'm going to scope bloat this to moving the logic for invoking configure out of client.mk. I apologize in advance for the patches that some unlucky soul will be asked to review.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Functionality for evaluating file-based mtime dependencies;

We want to get rid of client.mk and drive the build system from
Python instead of make. This requires reinventing aspects of a
build system to determine if targets are up to date. And since our
initial intent is to port logic from client.mk to Python, we must
implement aspects of make's logic to Python.

Make works by looking at target/file presence and mtime to determine
if it is up to date. In this commit, we implement some utility Python
code for evaluating a file-based "is up to date" check using mtimes.

I figured it was worth making this code generic instead of inlining
a bunch of I/O operations in to-be-written Python code. If nothing
else, it makes the final Python code easier to read. And, who knows,
perhaps we'll reuse this dependency checking code in other operations
in the future.

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

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Stop writing FOUND_MOZCONFIG variable;

The last consumer of FOUND_MOZCONFIG was removed in 41492b2ba4e1
(bug 1417264). client.mk only takes the content of mozconfig.json
into account (which is derived from the content of the mozconfig
file) for dependency checking. So it should be safe to stop
writing this to .mozconfig.mk.

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

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Remove BUILDSTATUS messages from client.mk;

When `mach build` sees lines beginning with BUILDSTATUS, it feeds that
into the resource collection mechanism so it knows how long various
operations take and what resources they consume.

As I started writing the patches to move client.mk's configure logic
to Python, I couldn't figure out an obvious way to work in metrics
collection without making code review difficult. So the plan is to
temporarily remove this feature then restore it once configure is
being invoked from Python. I wanted this to be called out explicitly
in its own commit so it isn't lost in an upcoming large patch.

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

Comment 5

[Gregory Szorc [:gps]]

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

Comment 6

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Prepare the object directory earlier;

The code in _run_client_mk() was extracted from client.mk. So
far, the bulk of that code deals with initializing the objdir.

Future patches uncovered some wonkiness with the ordering of the
execution of this code.

In this commit, we split out the code for preparing the object
directory into its own function. We then call this function very
early in the build process.

This ensures that files like .mozconfig.json are present and
accessible more often.

FWIW, I heard glandium say he would like configure to create the
objdir. This would presumably also entail configure managing
files like .mozconfig.json. I'm not opposed to this idea. However,
changing that right now is difficult since things are changing
so fast. Let's get functionality out of client.mk then clean
things up later.

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

Comment 7

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Port client.mk code to run configure to Python;

This is a large patch and there's a lot going on. I'm sorry for anyone
who has to read it.

The build system is effectively a pipeline of stages that complete
one after the other. Here are the stages relevant to this commit:

  `mach` -> env config -> configure -> config.status -> build backend

Before this commit, everything after "env config" (finding topsrcdir,
topobjdir, and extracting specific data from an active mozconfig) was
implemented in client.mk. (And until recently pretty much everything
was implemented in client.mk and `mach` effectively ran
`make -f client.mk`.)

The goal of this commit is to move the configure pipeline stage
from client.mk to Python.

The structure of configure is kinda wonky. A lot of that is for
historical reasons. With the recently established forced requirement
to use `mach` to build, a lot of this can likely be simplified.
However, our immediate goal is to remove client.mk while preserving
its behavior as closely as possible. Cleanup and refactoring can
come later. (Although we will perform some.)

The "configure" and "js/src/configure" files derive from "configure.in"
files. In the old days, we used autoconf for the derivation. These days,
client.mk just copies the file. (But configure.in is also a valid m4
file, so autoconf can still be used!)

The "configure" files today are very thin shell scripts that invoke
the new Python-based configure: "configure.py."

When configure/configure.py runs, its main output is a "config.status"
file in the objdir. This file - an executable Python script - contains
all the data obtained by running configure.

When "config.status" runs ("configure" will run it automatically
after writing it), it reads moz.build files and generates files
to be consumed by a build backend to build the configured application.
Since we always produce a make backend today, the most visible output
from config.status is a "Makefile" in the objdir.

This commit ports the logic for invoking configure from client.mk
to Python.

When reviewing the code, it may help to look at removals from client.mk
from bottom to top. Here's a summary of what was going on.

client.mk has two primary targets: "build" and "configure."
The recipe for "configure::" is pretty basic: run
$(topsrcdir)/configure from the objdir and touch $(objdir)/Makefile.
Of course, there are a lot of dependencies required to get there. More
on those later.

The relationship between the "build" and "configure" targets is a bit
obscure (original lines 142-155 in client.mk). But the gist of it is:

* If Makefile exists, Makefile depends on config.status and
  config.status depends on $(CONFIG_STATUS_DEPS).
* If Makefile doesn't exist, Makefile depends on $(CONFIG_STATUS_DEPS).
* Makefile target is produced by running "make -f client.mk configure"
* "build" depends on Makefile and config.status.

Essentially, this is expressing that Makefile is built by
config.status and these both depend on $(CONFIG_STATUS_DEPS).
When "build" is evaluated, both Makefile and config.status must exist
and be up to date. If not, we evaluate the "configure" target of
client.mk in a separate invocation of client.mk (I suspect it isn't
an "internal" target dependency for legacy reasons and to keep things
simple).

The "configure" target itself has a couple of dependencies:

* The configure files themselves (which are copied from configure.in)
* .mozconfig.json (which is derived from the active mozconfig)

The "config.status" target is a bit more interesting. It lists a lot
of random files throughout the repo. Basically things that when changed
have a significant impact on the build configuration.

In addition, the included configure.d file also extended the
dependencies for config.status.

Logically, the "configure" and "config.status" dependencies should
probably be the same. I think the reason they are separate is
historical reasons. If you look at $(EXTRA_CONFIG_DEPS), those are
files related to m4. Those made sense as dependencies when configure
was produced by running autoconf. They should have been removed
when we started producing configure via `cp`. But they weren't.
And, it appears moz.configure isn't tracking these m4 files in its
auto-generated configure.d file. So we need to carry those files
with us in the Python port (I intend to fix this in another commit).

The new Python code is hopefully easier to read.

Because invoking configure represents a distinct stage in the
build pipeline and because building.py was already quite large,
I put the code in a new module.

Because I don't like the class-centric design of the MozbuildObject
base class (I wish I had a time machine to uninvent that "god"
object), I've stepped out of my way to avoid using it explicitly
in the new code.

The new code in configure.py is relatively straightforward. We
have two new high-level APIs: one to ensure configure is up to
date (running it if not) and one to always run configure. There is
definitely some wonkiness. Such as having to set the MAKE
environment variable (we'll fix this later). Otherwise, it is
pretty straightforward with regards to capturing file dependencies
and invoking configure.

There are a few notable changes in behavior.

First, we now pull in all loaded .py modules as dependencies for
configure. This will include python.mozbuild.*, which is desirable.
Less desirable is the inclusion of other modules, like random
mach_commands.py files. I think we'll eventually want to move the
"run configure if necessary" code to a separate process to mitigate
this. We may even want to move the logic to configure itself and
just always invoke configure, having it no-op as appropriate.

Second, since we no longer run the configure process from the context
of client.mk, variables set in client.mk - including from the included
mozconfig-derived make files - are no longer set in the configure
environment. I think this is fine. configure has logic for locating
the mozconfig that matches what mach is doing. So I'm pretty sure
the end result is the same. The only variable we do need to set
though is MAKE. That's because moz.configure's make detection logic
is poor. This will be improved in subsequent commits and the MAKE
hack will go away.

The new code documents a handful of known areas for improvement.
I don't think any of these are show stoppers. Perfect is the
enemy of good. Subsequent commits will improve matters
significantly...

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

Comment 8

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Stop looking for mingw32-make;

Support for mingw32-make was added in bug 1100925 in order to
support msys2. That project is effectively dead. I'm about to
refactor this code and don't want to deal with extra requirements.
So delete references to mingw32-make.

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

Comment 9

[Gregory Szorc [:gps]]

Attachment: Bug 1417684 - Reimplement make detection in moz.configure;

Previously, moz.configure assembled a list of possible make binary
names and did a check_prog, taking the first one that resolved. This
logic was inadequate and littered with bugs.

Until very recently, moz.configure was always invoked with client.mk.
And client.mk exposed MAKE to the configure invocation. And this MAKE
was derived by Python code in mozbuild that knows how to find a
suitable make. So even though moz.configure had logic for resolving
MAKE, chances are that mozbuild was really doing all the work.

A bug in the old implementation was that the MAKE environment
variable was supplemented by other candidate binaries. Generally
speaking, if a variable is passed into configure, it should be
used explicitly. But the previous code didn't do that. The new code
only looks at MAKE if it is defined.

The old code was also failing to look for mozmake on Windows. We
actually require mozmake on Windows. Basically, the previous code
was relying on the fact that MAKE was defined and pointing to
mozmake for it to work. If we were to undefine MAKE and invoke the
old code, builds on Windows would break.

The old code was also not validating the make binary. The Python
code in base.py for locating a make binary (and client.mk until
recently) invokes baseconfig.mk to validate the make binary. So,
it was possible for the old moz.configure code to use a make binary
that can't actually be used with the build system.

The new moz.configure code is much more robust.

If MAKE is defined, it only looks at that binary. Otherwise, it
falls back to searching for various make binary names. The search
order is similar to base.py. The lone exception is mozmake is last
instead of between "make" and "gnumake." Since base.py's logic has
been invoking client.mk and that make becomes MAKE and is used by
moz.configure, I'm not anticipating any significant change in
behavior.

We also validate the make binary in moz.configure. The logic is
very similar to that in base.py. Basically, we invoke baseconfig.mk
(which does most validation) and special case the Xcode license
check failure. Eventually, we can likely move more logic from
baseconfig.mk into moz.configure. And once we no longer run
client.mk to invoke the build system, we can remove the "find make"
code from base.py and drive everything from moz.configure. But
these are for other commits.

To prove the new code works, we stop passing the mozbuild derived
MAKE into configure. moz.configure (or the mozconfig) is now in
the driver's seat for finding a make binary.

We still may not actually use the make found by moz.configure for
invoking client.mk or the build backend. But this was always a
problem (at least for `mach build`). We'll need to fix this
in subsequent commits.

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

Comment 10

[Gregory Szorc [:gps]]

This series fails spectacularly on Try. There's obviously some work to do.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8929295 [details]
Bug 1417684 - Functionality for evaluating file-based mtime dependencies;

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

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8929296 [details]
Bug 1417684 - Stop writing FOUND_MOZCONFIG variable;

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

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8929298 [details]
Bug 1417684 - Always evaluate config environment;

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

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8929299 [details]
Bug 1417684 - Prepare the object directory earlier;

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

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8929300 [details]
Bug 1417684 - Port client.mk code to run configure to Python;

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8929300 [details]
Bug 1417684 - Port client.mk code to run configure to Python;

Everything up to this one should be reviewable. I'm still excavating the rabbit hole for moving the configure logic. I also think I can split up this commit and make everything easier to review.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 8929296 [details]
Bug 1417684 - Stop writing FOUND_MOZCONFIG variable;

https://reviewboard.mozilla.org/r/200614/#review207048

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8929295 [details]
Bug 1417684 - Functionality for evaluating file-based mtime dependencies;

https://reviewboard.mozilla.org/r/200612/#review207046

This looks sensible. Thanks for writing tests!

::: python/mozbuild/mozbuild/dependsutils.py:29
(Diff revision 2)
> +    * The mtime of all targets is newer than the newest mtime of dependencies.
> +    """
> +    if not target_files:
> +        raise ValueError('must specify targets')
> +
> +    stats = batch_stat(target_files + depends_files)

Your function documentation claims these are both iterables but you're treating them as sets here. If they're always sets then document that instead. If you intend to support arbitrary iterables, you'd presumably have to wrap them both in `set()`.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8929298 [details]
Bug 1417684 - Always evaluate config environment;

https://reviewboard.mozilla.org/r/200618/#review207052

I'm very sure you're correct given the comment you're removing. Nice to have removed universal builds, they made so many things awkward.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8929299 [details]
Bug 1417684 - Prepare the object directory earlier;

https://reviewboard.mozilla.org/r/200620/#review207056

::: python/mozbuild/mozbuild/controller/building.py:1318
(Diff revision 2)
>                             print_directory=False)
>          else:
>              install_test_files(mozpath.normpath(self.topsrcdir), self.topobjdir,
>                                 '_tests', test_objs)
>  
> -    def _run_client_mk(self, target=None, line_handler=None, jobs=0,
> +    def _prepare_objdir(self):

Can you document what the return value of this function means? I find it a little confusing that returning `True` is handled as the exceptional case. Also, it might be nice to make it return a value in all cases. Having a single return statement makes things a little weird.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.