860898 - Add an option for non-silent make when running "mach build"

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add --show-commands to mach build

Often I want to build without the -s option to make so that I can see what commands are running. Adding a mach option for it seems like a good idea. I couldn't use --verbose because that appears to be taken already and does something else (prints out some environment variables) so I used --show-commands.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 736463 [details] [diff] [review]
Add --show-commands to mach build

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

-v --verbose is taken by the global argument parser. I /think/ argparse allows you to do craziness such as:

$ mach -v build -v

I'd much prefer -v for the option because that's a pretty common convention. If we can't define -v on the build command, perhaps we can overload the global -v to not use -s.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Allow subcommands to have their own verbose option

(In reply to Gregory Szorc [:gps] from comment #1)
> -v --verbose is taken by the global argument parser. I /think/ argparse
> allows you to do craziness such as:
> 
> $ mach -v build -v

The new patch now supports this, by virtue of changing verbose from store_true to append_const and having separate const values for the global and subcommand versions. I also had to beef up the argument consumption to deal with this.

Apologies if the code is ugly; I'm a python n00b so please point out any and all style improvements. In particular I'm sure there's a better/more robust way of doing the stripping.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 736463 [details] [diff] [review]
Add --show-commands to mach build

Whoops, forgot to obsolete the old patch.

Comment 4

[Gregory Szorc [:gps]]

Nifty solution! While looking at the patch, I realized that the work I did in bug 856392 should have the handy side-effect of allowing the same argument to existing multiple times. e.g. |mach build -v| should "just work" and there should be no issue with the global options being consumed by the sub-command's parser. At least I think that's how it will work.

I'd like to get this functionality checked in. But, it does overlap somewhat with bug 856392. And, I do apologize for that - I didn't realize my work in that bug would impact you here.

If you'd really like to get this in, we can check it in then I can clean up the mess later. Or, you can wait. Your call.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I don't mind waiting, as long as you can verify that the patch on bug 856392 will make that syntax "just work". I took a quick look at the patch there but couldn't quickly tell if that would be the case.

Comment 6

[Gregory Szorc [:gps]]

I'm not 100% sure bug 856392 will "just work." But, I think it will. Essentially what I did is replace the "static" argument parser with a dynamic one. As soon as a named command is encountered in the arguments list it will feed all remaining arguments to the sub-command's argument parser. So, the parsers for the global arguments and the command's arguments are completely separate and "verbose" thus shouldn't get swallowed like it currently does. In theory.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

So I applied your patch from bug 856392 locally and it doesn't seem to affect the behaviour here. That is, | mach --verbose build | does the expected thing with global verbose flag, and | mach build --verbose | errors out (now with awesome clippy face) unless the build subcommand declares a verbose arg. If the subcommand does declare a verbose arg, it doesn't error out, but the verbose arg is consumed at the global level and so the subcommand never sees it.

My patch applies cleanly even with your patch applied, and does the expected thing with all of | mach {--verbose,} build {--verbose,} |, so I think it's worth reviewing/landing.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 736922 [details] [diff] [review]
Allow subcommands to have their own verbose option

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

I agree with your logic that we should go ahead and land this given that the refactoring in the other bug didn't fix things.

However, the use of lists and special constant values to denote where things apply doesn't jive with me. It feels very fragile.

Given the patch is essentially a giant hack around something that's easily correctable, I'm going to implement a proper patch to the argument parses that will separate global arguments from command arguments and will not swallow "verbose". I apologize for the confusion. You just had the misfortune of being the first to fall into an existing pit. And, I'm partially responsible for pushing you in because of my insistence that we use --verbose instead of something else. But, I hope you understand that once an argument is added it's hard to change things and I'd rather not carry more cruft than necessary.

I'll work on the prerequisite patch now.

::: python/mach/mach/main.py
@@ +285,5 @@
> +        # value in args.
> +        stripped = {}
> +        for argkey in vars(args):
> +            argval = getattr(args, argkey)
> +            if argkey in CONSUMED_ARGUMENTS.keys():

No need to .keys() here. |x in some_dict| tests for key presence already.

@@ +286,5 @@
> +        stripped = {}
> +        for argkey in vars(args):
> +            argval = getattr(args, argkey)
> +            if argkey in CONSUMED_ARGUMENTS.keys():
> +                if CONSUMED_ARGUMENTS[argkey] == None:

if CONSUMED_ARGUMENTS[argkey] is None

== None is frowned upon (and most will say you should just |if not val|. But in this case I think we could have values with 0 or empty lists (which always evaluate as falsy).

@@ +292,5 @@
> +                    continue
> +                for removeelem in CONSUMED_ARGUMENTS[argkey]:
> +                    # remove the specified elements
> +                    if removeelem in argval:
> +                        argval.remove(removeelem)

First, idioms like this should use a list comprehension. Second, argval may not have a .remove(), no?

@@ +293,5 @@
> +                for removeelem in CONSUMED_ARGUMENTS[argkey]:
> +                    # remove the specified elements
> +                    if removeelem in argval:
> +                        argval.remove(removeelem)
> +                if len(argval) == 0:

If you are testing whether a list has elements, the preferred method is |if some_list|.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Yeah I agree this patch is more or less a giant hack and I'd rather wait for your proper patch to separate the arguments. This isn't urgent at all so it's better to do it right.

Comment 10

[Gregory Szorc [:gps]]

And I have a new patch in bug 856392. Please try it out.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add verbose option for build command

Updated patch that uses the stuff in bug 856392.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 738264 [details] [diff] [review]
Add verbose option for build command

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

I love it when a patch gets simpler!

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb295fdd420a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb295fdd420a