855593 - mach shows ac_add_app_options is not supported, but ac_add_app_options is not present in mozconfig

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://developer.mozilla.org/en-US/docs/Configuring_Build_Options#Firefox.2C_Default_Release_Configuration

I wanted to compile a version of Firefox as close to release as possible, so I took the mozconfig from that link, pasted it to .mozconfig in my root source directory, and ran ./mach build and hit an error loading mozconfig:

===
Evaluation of your mozconfig exited with an error. This could be triggered
by a command inside your mozconfig failing. Please change your mozconfig
to not error and/or to catch errors in executed commands.

mozconfig output:

------BEGIN_MK_OPTION
MOZ_BUILD_PROJECTS=i386
------END_MK_OPTION
------BEGIN_MK_OPTION
x86_64
------END_MK_OPTION
------BEGIN_MK_OPTION
MOZ_UNIFY_BDATE=1
------END_MK_OPTION
------BEGIN_MK_OPTION
MOZ_POSTFLIGHT_ALL+=build/macosx/universal/flight.mk
------END_MK_OPTION
ac_add_app_options is not supported.
===

My mozconfig from that link did not have ac_add_app_options at all, but it did reference:

http://hg.mozilla.org/mozilla-central/annotate/962f5293f87f/build/macosx/universal/mozconfig

which again references:

http://hg.mozilla.org/mozilla-central/annotate/962f5293f87f/build/macosx/universal/mozconfig.common

and this file does have ac_add_app_options.

Releng builds I presume don't hit this issue because they don't call mach?

Comment 1

[Gregory Szorc [:gps]]

The mozconfig loading code used by mach does not support ac_add_app_options. This coded landed December 4, 2012 and you are the first person to report the issue. This pretty much confirms my suspicion that hardly anybody uses ac_add_app_options. I haven't looked at the official mozconfigs, so I'm not sure if it is really necessary.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Thus, three possible ways forward?

1. mach needs a fix. (unlikely, as per comment 1?)
2. Official mozconfigs need a fix.
3. Documentation needs a fix.

Comment 3

[Gregory Szorc [:gps]]

Little bit of all 3, likely.

Back in bug 808357 we implemented mozconfig "parsing" in Python. This is what mach uses. This supplements and is different from the existing mozconfig "parsing" code used by client.mk and configure (they each use a slightly different version).

Going forward, I'd like to unify on 1 implementation - the Python one. If we need to support ac_add_app_options, the Python code will grow support to parse this. If not, we'll deprecate it.

Before any of that happens, we're talking about major refactorings to client.mk. That's not on anybody's radar. So, my guess there won't be movement on this bug for another few months. Until then, don't use ac_add_app_options.

Comment 4

[Justin Wood (:Callek)]

(In reply to Gregory Szorc [:gps] from comment #1)
> The mozconfig loading code used by mach does not support ac_add_app_options.
> This coded landed December 4, 2012 and you are the first person to report
> the issue. This pretty much confirms my suspicion that hardly anybody uses
> ac_add_app_options. I haven't looked at the official mozconfigs, so I'm not
> sure if it is really necessary.

It is necessary, and I for one have used ac_add_app_options for years.

I would argue mach needs to support it. Its just not as common as the other mozconfig options, nor is mach as widespread used yet as you might suspect (imho)

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Before any of that happens, we're talking about major refactorings to
> client.mk. That's not on anybody's radar. So, my guess there won't be
> movement on this bug for another few months. Until then, don't use
> ac_add_app_options.

It's not that I want to use ac_add_app_options - it's present in a file that gets called by the mozconfig I got from MDN. As you can see in:

https://developer.mozilla.org/en-US/docs/Configuring_Build_Options#Firefox.2C_Default_Release_Configuration

I did not intend for ac_add_app_options to be there at all. If a new contributor wants to compile Firefox, he/she will likely use this same step and hit this issue in mach.

... unless we recommend to use make -f client.mk ? Or remove this ". $topsrcdir/build/macosx/universal/mozconfig" line if we were to use mach?

Comment 6

[Gregory Szorc [:gps]]

I'm not sure why we advertise the release configs like that. My opinion is the build system should have a reasonable default without a mozconfig file and we shouldn't burden new developers to create mozconfig files. If you look at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build, the mozconfig page is behind a link. I'm guessing a lot of people don't follow it.

As I said in my first comment, you are the first person to report this since December. I have an IRC notification for "mach" and am in #introduction and I do not recall this ever coming up before. Therefore, I'm going to assert it's not a big deal.

Also, ac_add_app_options is only used by people who build multiple apps with client.mk. This workflow is not currently supported by mach. Again, very few people have complained about this. I'm not opposed to supporting it, but the return on investment appears to be low. There are far bigger fish to fry.

Convince me I am wrong and I will reconsider this position.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Convince me I am wrong and I will reconsider this position.

Nope, I understand your position, and I guess I can just remove that line and work around it for the moment.

Comment 8

[Mike Hommey [:glandium]]

Note that using ac_add_app_options is the only way to do a universal build on mac at the moment, so mach not supporting it just makes mac developers who want to do universal builds not use mach.

That nobody complained so far is more of a sad acknowledgment that mach is not widely used than about the fact that no one using ac_add_app_options.

Comment 9

[(not currently active) Ted Mielczarek]

...or that most people aren't doing universal builds locally.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: improved error message

I'm not sure what it would take to actually support ac_add_app_options in mozbuild, but for now we can at least improve this error message, for those people trying to build with the release mozconfigs.

Comment 11

[Gregory Szorc [:gps]]

It should be relatively simple to support ac_add_app_options in mozconfig_loader. Let's just do that.

Since it has come up twice within a week, that's bumped up the priority in my mind.

Matt: Do you want to take the handle the full fix? If not, it's something I should be able to do pretty quickly.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This adds support for ac_add_app_options in mozbuild.mozconfig.

This is good enough to make |mach build| work, though it's not great for for actually using configure_args.  For that we would currently need to read the mozconfig to find the objdir, then read $objdir/config.status to get MOZ_BUILD_APP, then read the mozconfig again to get the right configure_args.

To solve that problem when it comes up, we might want read_mozconfig to return an object instead of a static dictionary.  The object could have methods like "configure_args" that accept data from ConfigEnvironment.

Comment 13

[Mike de Boer [:mikedeboer]]

I just stumbled upon this yesterday and it cost me a considerable amount of time to figure out WHY I couldn't do a universal build using Mach. I'm talking hours of trying, failing, pinging ppl on IRC, getting no response due to the time difference, etc. It's only a matter of time 'till the next dev gets into the same position.

Firefox has a considerable amount of options that require testing with an official vs. Nightly (unbranded) build. Since Mach is the advocated way to build the tree, it needs to support the build options that are actively used in the m-c tip as well.

I hope you understand, your effort will be much appreciated by me and your future colleagues!

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 732570 [details] [diff] [review]
patch

Review of attachment 732570 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: python/mozbuild/mozbuild/mozconfig.py
@@ +232,4 @@
>  
> +        if moz_build_app is not None:
> +            result['configure_args'].extend(self._expand(o) for o in
> +                parsed['ac_app'][moz_build_app])

I was going to say that I don't like merging the app options into configure_args and losing the original information. I was going to say that I would prefer for this class to focus solely on parsing/loading and not worry about logic behind mozconfigs. However, we already do things like substitute @TOPSRCDIR@, so we're already inconsistent.

So, meh. Worst case we refactor things later. That's always the solution, anyway :)

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/445d8eecdd80

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/445d8eecdd80