1089911 - mk_add_options should barf on unknown args

Status: NEW

(Firefox Build System :: General, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

mchew-22096:mozilla-central mchew$ cat mozconfig 
mk_add_options AUTO_CLOBBER=1
mk_add_options MOZ_MAKE_FLAGS="-j4"
ac_add_options --enable-debug
ac_add_options --disable-optimize

mchew-22096:mozilla-central mchew$ ./mach build
 0:00.57 /usr/bin/make -f client.mk MOZ_PARALLEL_BUILD=4 -s
 0:01.30 
 0:01.30 The CLOBBER file has been updated, indicating that an incremental
 0:01.30 build since your last build will probably not work. A full/clobber
 0:01.30 build is required.
 0:01.30 
 0:01.30 The reason for the clobber is:
 0:01.30 
 0:01.30  Bug 946065 needs a CLOBBER
 0:01.30 
 0:01.30 Clobbering can be performed automatically. However, we didn't
 0:01.30 automatically clobber this time because:
 0:01.30 
 0:01.30   Automatic clobbering is not enabled
 0:01.30   (add "mk_add_options AUTOCLOBBER=1" to your mozconfig).
 0:01.30 
 0:01.30 The easiest and fastest way to clobber is to run:
 0:01.30 
 0:01.30  $ mach clobber
 0:01.30 
 0:01.30 If you know this clobber doesn't apply to you or you're feeling lucky
 0:01.30 -- Well, are ya? -- you can ignore this clobber requirement by
 0:01.30 running:
 0:01.30 
 0:01.30  $ touch /Users/mchew/mozilla-central/obj-x86_64-apple-darwin13.4.0/CLOBBER
 0:01.30 make: *** [/Users/mchew/mozilla-central/obj-x86_64-apple-darwin13.4.0/CLOBBER] Error 1
 0:01.35 230 compiler warnings present.

Comment 1

[(not currently active) Ted Mielczarek]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #0)
> mchew-22096:mozilla-central mchew$ cat mozconfig 
> mk_add_options AUTO_CLOBBER=1

I'm sure this will seem obvious in retrospect, but you seem to have an extraneous underscore in the variable name.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

D'oh! Is there a way for mk_add_options to barf on unknown args?

Comment 3

[(not currently active) Ted Mielczarek]

Maybe. It's difficult given the way it's implemented. Currently we just take every line that starts with mk_add_options, massage the contents slightly and write out a file that gets included from client.mk. You can set arbitrary make variables this way. I suspect if we tried to whitelist a subset that we'd break some weird usages, but maybe that shouldn't matter.

Comment 4

[u459114]

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mozconfig_loader#38

What if we create a table here which lists args that we permit, and throw an error while a unknown arg given and terminate build process, so that we know something wrong.

Comment 5

[u459114]

I don't know whether we have a plan to replace .mozconfig by python format, just like replace Makefile.in by moz.build. It makes things easier

Comment 6

[(not currently active) Ted Mielczarek]

Having a whitelist of supported variables sounds like a simple way to make this work. Like I said in comment 3 we'll almost certainly find some things that we didn't know people were using, but I think that's okay. We'll need to support at least everything that's listed in an in-tree mozconfig:
https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=mk_add_options&redirect=true

That does show that we're using it in in-tree configs to do arbitrary variable exports, so maybe we should just whitelist any usage of mk_add_options "export ...".

Comment 7

[u459114]

Attachment: (WIP) 0001-validate-mk_add_options.patch

Comment 8

[u459114]

Comment on attachment 8549508 [details] [diff] [review]
(WIP) 0001-validate-mk_add_options.patch

Hi Ted,
Here is a simple prototype. Any suggestion?

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8549508 [details] [diff] [review]
(WIP) 0001-validate-mk_add_options.patch

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

I think this code should be in python/mozbuild/mozbuild/mozconfig.py.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8549508 [details] [diff] [review]
(WIP) 0001-validate-mk_add_options.patch

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

This approach seems fine, although you should follow gps' suggestion above.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +518,5 @@
> +            \s* [?:]?= \s*.$''' ,    # Assignment operator surrounded by optional
> +                                    # spaces
> +            re.VERBOSE)
> +        # A table to list permitted varaible name in client.mk
> +        valid_variable_table = ("AUTOCLOBBER", "PATH", "CLIENT_PY_ARGS")

You could use a set literal here, just replace the () with {}. Also nit: this file uses single-quotes for strings.

I assume you haven't tried to build out the full list of supported variables here since this is just a WIP? This will definitely need to be run past building all platforms on Try to catch things we're using in production, and I'm sure it'll still break people anyway if you land it.

@@ +521,5 @@
> +        # A table to list permitted varaible name in client.mk
> +        valid_variable_table = ("AUTOCLOBBER", "PATH", "CLIENT_PY_ARGS")
> +
> +        status = True
> +        for o in self._mozconfig['make_extra']:

I would probably pass this variable down into this function instead of using the member variable.

@@ +530,5 @@
> +            name = match.group('var')
> +            if not name in valid_variable_table:
> +                # Output error msg.
> +                # How about do string fuzz matching here, tells
> +                # user best matched string, according to his input

Doing fuzzy matching with a recommendation would be a nice improvement, but I wouldn't block on that, you can file it as a follow-up.