1074656 - Merge |mach dmd| and |mach debug| into |mach run|

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Nicholas Nethercote [inactive]]

This is a better way of doing things.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Merge |mach dmd| into |mach run|

This patch merges |mach dmd| into |mach run|.

Details:

- The option is |+dmd| rather than |--dmd|... turns out |mach run| has
  |allow_all_args=True| set, which causes the argparser's |prefix_char| to be
  set to '+', which means that all options for |mach run| must be '+'-style
  ones. Bah. Something to fix later on.

- For passing args to DMD you have to define $DMD accordingly, e.g.:

    DMD='--sample-below=1' mach run +dmd

  I have an alias for running DMD and this is exactly how it works, so I think
  it's a reasonable solution! :) Esp. since the hard part of running DMD is
  setting the other env vars.

- I inlined get_run_args, because with |mach dmd| gone it was only used from
  one place.

- At glandium's request, we now use the libdmd in dist/bin/ (or equivalent)
  rather than dist/lib/.

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8497317 [details] [diff] [review]
Merge |mach dmd| into |mach run|

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

Overall I like the idea of merging |dmd| and |run|. Unfortunately I don't think this improves anything, and to me at least, makes things worse by requiring undocumented (in mach) DMD params be passed via an env var. 

The primary benefits of |mach dmd| as-is are:
 - Lets you specify DMD params directly (no env var) and documents them
 - Uses '--' params

I agree it would be nice to merge this back into |mach run| but I think we should address it's deficiencies first, which primarily are:
 - Uses '+' params
 - Has duplicated logic b/w run, debug, and dmd
 - AFAICT has no way to do argument groups [1] (so for example all DMD args could be grouped, and all debug args could be grouped)

Once we fix up |mach run| I'd definitely r+ this. Of course I'm open to discussing further!

[1] https://docs.python.org/2/library/argparse.html#argument-groups

Comment 3

[Nicholas Nethercote [inactive]]

>  - Has duplicated logic b/w run, debug, and dmd

Huh? This patch removes the duplication between |run| and |dmd|.

Comment 4

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> >  - Has duplicated logic b/w run, debug, and dmd
> 
> Huh? This patch removes the duplication between |run| and |dmd|.

But not |debug|, for the most part at least the logic for param parsing is already shared as-is for |run| and |dmd| with or without this change.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8497317 [details] [diff] [review]
Merge |mach dmd| into |mach run|

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

::: python/mozbuild/mozbuild/mach_commands.py
@@ +782,5 @@
>          description='Run the compiled program.')
>      @CommandArgument('params', default=None, nargs='...',
>          help='Command-line arguments to be passed through to the program. Not specifying a -profile or -P option will result in a temporary profile being used.')
> +    @CommandArgument('+dmd', action='store_true',
> +        help='Enable DMD. Pass arguments to DMD by setting the |DMD| environment variable.')

Please rebase on top of bug 1076649. I'd rather have the '+' done first, so that the other dmd options look normal.

@@ +839,5 @@
> +                    "MOZ_REPLACE_MALLOC_LIB": dmd_lib,
> +                    "DMD": dmd_env_var,
> +                },
> +            }
> +            append_env = env_vars[self.substs['OS_ARCH']]

env_vars.get(self.substs['OS_ARCH'], {})

@@ -928,5 @@
> -        help='The sample size to use, [1..n]. Default is 4093.')
> -    @CommandArgument('--max-frames', default=None, type=str,
> -        help='The max number of stack frames to capture in allocation traces, [1..24] Default is 24.')
> -    @CommandArgument('--show-dump-stats', action='store_true',
> -        help='Show stats when doing dumps.')

I think you should copy the dmd-specific options and make mach run error out if they are passed without --dmd.

Comment 6

[Nicholas Nethercote [inactive]]

I'm going to expand the scope of this bug to include |mach debug|. I have it working locally, and the nice thing is that you can invoke DMD *and* the debugger at the same time, which is a valid combination, but one that's not possible when they're invoked in different mach commands.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Merge |mach debug| and |mach dmd| into |mach run|

This patch merges |mach debug| and |mach dmd| into |mach run|. Apologies for
the hard-to-read patch; I couldn't see how to split it into more digestible
pieces.

Things to note:

- This patch applies on top of the patch in bug 1077272.

- The help output (see below) shows the new structure.
  
- The capitalization of the group titles in inconsistent; I suggest
  de-capitalizing "Arguments".

- -remote, -background and -noprofile are single-dash options; everything
  else is double-dash. glandium argued for this because Firefox uses
  single-dash options, and because having --remote (a |mach run| option) and
  -remote (an obscure Firefox option) could cause confusion. I dislike the
  inconsistency.

- You have to specify the new --debug option to use the debugger.

- If you specify any of the debugger options without --debug, they are ignored.
  Likewise with the DMD options and --dmd. We could instead abort in those
  cases.

- You can now invoke the debugger and DMD together, which is nice.

- It's 54 fewer lines of code, which is nice.


> usage: mach [global arguments] run [command arguments]
> 
> Run the compiled program, possibly under a debugger or DMD.
> 
> Global Arguments:
>   -v, --verbose         Print verbose output.
>   -l FILENAME, --log-file FILENAME
>                         Filename to write log data to.
>   --log-interval        Prefix log line with interval from last message rather
>                         than relative time. Note that this is NOT execution
>                         time if there are parallel operations.
>   --log-no-times        Do not prefix log lines with times. By default, mach
>                         will prefix each output line with the time since
>                         command start.
>   -h, --help            Show this help message.
> 
> Command Arguments for the compiled program:
>   params                Command-line arguments to be passed through to the
>                         program. Not specifying a -profile or -P option will
>                         result in a temporary profile being used.
>   -remote, -r           Do not pass the -no-remote argument by default.
>   -background, -b       Do not pass the -foreground argument by default on
>                         Mac.
>   -noprofile, -n        Do not pass the -profile argument by default.
> 
> Command Arguments for debugging:
>   --debug               Enable the debugger. Not specifying a --debugger
>                         option will result in the default debugger being used.
>                         The following arguments have no effect without this.
>   --debugger DEBUGGER   Name of debugger to use.
>   --debugparams params  Command-line arguments to pass to the debugger itself;
>                         split as the Bourne shell would.
>   --slowscript          Do not set the JS_DISABLE_SLOW_SCRIPT_SIGNALS env
>                         variable; when not set, recoverable but misleading
>                         SIGSEGV instances may occur in Ion/Odin JIT code.
> 
> Command Arguments for DMD:
>   --dmd                 Enable DMD. The following arguments have no effect
>                         without this.
>   --sample-below SAMPLE_BELOW
>                         Sample blocks smaller than this. Use 1 for no
>                         sampling. The default is 4093.
>   --max-frames MAX_FRAMES
>                         The maximum depth of stack traces. The default and
>                         maximum is 24.
>   --show-dump-stats     Show stats when doing dumps.
>   --mode {normal,test,stress}
>                         Mode of operation. The default is normal.

Comment 8

[Nicholas Nethercote [inactive]]

> having --remote (a |mach run| option) and -remote (an obscure Firefox option) could cause confusion.

glandium also just said on IRC: "i think we can make a case to kill -remote" (as in the Firefox option)

Comment 9

[Mike Hommey [:glandium]]

I also said that -background and -noprofile with single-dash make sense because they are related to firefox's -foreground and -profile single-dash options.

Comment 10

[Nicholas Nethercote [inactive]]

And I argue that single-dash, long options are essentially broken and that we shouldn't emulate them :) Let's see what gps thinks.

Comment 11

[Mike Hommey [:glandium]]

I just found out that firefox actually accepts *both* forms (single and double dash).

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8501438 [details] [diff] [review]
Merge |mach debug| and |mach dmd| into |mach run|

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

I agree with the sentiment that single dash multi-character arguments should go away. However, I feel that's beyond the scope of this bug, as the existing mach commands deal with single dashes just fine.

I think you should land this working patch now and file a follow-up to improve the dash situation. I think adjusting the mach commands would be excellent fodder for the bug where you change Firefox's help message to advertise/prefer the double dash forms.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +886,2 @@
>  
> +            extra_env['MOZ_CRASHREPORTER_DISABLE'] = '1'

It's kinda crazy that the firefox binary has all these environment variables that control run-time behavior. It bothers me because they are not easily discoverable (https://www.google.com/search?q=JS_DISABLE_SLOW_SCRIPT_SIGNALS) and people don't realize what tools are available to help them debug Firefox. You can make the argument that is what |mach run| is for. But at the same time, I think a binary should document its options better.

I think it would be a worthy goal (outside of this bug) to expose these as command line arguments somehow. I understand maybe not wanting to ship with them enabled. Maybe we could just disable them for "official" builds?

</ivory tower perspective>

Comment 13

[Nicholas Nethercote [inactive]]

> I think you should land this working patch now and file a follow-up to
> improve the dash situation. I think adjusting the mach commands would be
> excellent fodder for the bug where you change Firefox's help message to
> advertise/prefer the double dash forms.

Good idea. I filed bug 1080302 about this.

Comment 14

[Nicholas Nethercote [inactive]]

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

Comment 15

[Nicholas Nethercote [inactive]]

I also updated https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_gdb, which was the only interesting documentation I could find when I googled for "mach debug".

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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