1413294 - Limit use of mk_add_options

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

The original use of mk_add_options was for injecting state into and controlling client.mk, which was our official frontend to the build system.

We've since consumed mozconfigs and the contents of mk_add_options from outside client.mk.

With the elimination of client.mk, mk_add_options will effectively become a relic of a bygone era.

I'm filing this bug to start weaning us away from mk_add_options. Will likely:

* Convert most uses of mk_add_options in in-tree mozconfigs to use ac_add_options or export.
* Ban or curtail use of MOZ_MAKE_FLAGS

Since the changes could break local mozconfigs, I'd like to have a transition story with a reasonable guarantee of longevity in place so that when we force people to change mozconfigs they only have to do it once in the near future.

Comment 1

[Gregory Szorc [:gps]]

After spending several hours going down the rabbit hole of mk_add_options, I'm unwilling to spend more time on a proper solution at this time.

As stated in the initial comment, mk_add_options no longer makes sense when we no longer use `make`. However, it is commonly being used as a mechanism to inject environment variables into configure, the build backend, and everything in between.

I would prefer to statically parse mozconfigs to resolve make variable values. I've written patches to do some of this - banning certain mk_add_options behavior in the process. However, there is a long tail of things relying on mk_add_options - including a major unknown component: end user's mozconfigs.

Rather than risk extra disruption and follow more rabbit holes to implement the correct solution, I'm going to refactor code so mk_add_options is evaluated in a standalone make context/file as part of loading mozconfigs. The mozconfig loader will parse the output of this make invocation and the result will be a dict of variables set via mk_add_options. This dict will be static and easily injected into environment variables without having to use make as part of the build system proper.

Comment 2

[Gregory Szorc [:gps]]

The commit message for https://hg.mozilla.org/mozilla-central/rev/e80c32947f33 is relevant to this bug.

Comment 3

[Mike Hommey [:glandium]]

I got myself in some sort of rabbit hole related to this with bug 1495641, and I'm just going to work around it nastily because that was too much of a rabbit hole.

There are interesting problems with mk_add_options as we use it, and we clearly need something to be done about the whole thing. This might involve moving things out of mozconfigs, or something else. Anyways, the immediate problems that I had:

- MozbuildObject.extra_environment_variables reads the exported variables from .mozconfig.mk, which come directly from mk_add_options. But .mozconfig.mk is meant for use by make, while MozbuildObject.extra_environment_variables works using a shell parser, and fails in subtle edge cases (*cough* *cough* PATH on Windows).
- MozbuildObject.extra_environment_variables has only one use currently, for a valgrind script. I wanted to add one for when running `mach static-analysis autotest`. More generally, we do need some way to conveniently set up environment variables for different kind of jobs, and we're using mozconfigs' mk_add_options for that currently.
- MozbuildObject.extra_environment_variables doesn't eliminate the newline character from each line it reads, so the variables it gets contain an extra newline character. That happens to work, but if something else than the valgrind script were to use it, it would likely hit problems.
- MozbuildObject.extra_environment_variables produces unicode strings, which doesn't work well with os.environ, which only takes str on Windows. Also, the .mozconfig.mk file is actually probably not unicode-encoded on Windows, and would probably need a mbcs-treatment.
- It's technically possible to do horribly hacky things with mk_add_options, that only work with make, and would obviously blatantly fail with MozbuildObject.extra_environment_variables.

Comment 4

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

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