Closed
Bug 1074656
Opened 10 years ago
Closed 10 years ago
Merge |mach dmd| and |mach debug| into |mach run|
Categories
(Core :: DMD, defect)
Core
DMD
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 1 obsolete file)
15.34 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
This is a better way of doing things.
Assignee | ||
Comment 1•10 years ago
|
||
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/.
Attachment #8497317 -
Flags: review?(mh+mozilla)
Attachment #8497317 -
Flags: review?(erahm)
Comment 2•10 years ago
|
||
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
Attachment #8497317 -
Flags: review?(erahm) → feedback-
Assignee | ||
Comment 3•10 years ago
|
||
> - Has duplicated logic b/w run, debug, and dmd
Huh? This patch removes the duplication between |run| and |dmd|.
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #8497317 -
Flags: review?(mh+mozilla) → feedback-
Assignee | ||
Comment 6•10 years ago
|
||
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.
Summary: Merge |mach dmd| into |mach run| → Merge |mach dmd| and |mach debug| into |mach run|
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8501438 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8497317 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
> 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•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
||
I just found out that firefox actually accepts *both* forms (single and double dash).
Comment 12•10 years ago
|
||
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>
Attachment #8501438 -
Flags: review?(gps) → review+
Assignee | ||
Comment 13•10 years ago
|
||
> 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.
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8cd2e3b1704
Assignee | ||
Comment 15•10 years ago
|
||
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".
https://hg.mozilla.org/mozilla-central/rev/b8cd2e3b1704
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•