mach doesn't allow first argument within --debugger-args / --debugparams to have an = sign

RESOLVED FIXED in Firefox 45

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: dbaron, Assigned: glandium)

Tracking

Trunk
mozilla45
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox45 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

I think this is a recent regression since this worked for me quite recently, but I don't see code-wise how it regressed recently.

./mach run --debug --debugger valgrind --debugparams "--tool=memcheck --leak-check=no --trace-children=yes --num-callers=50 --error-limit=no --fullpath-after= --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access" file:///home/...

now gives an error:
The --debugger_args you passed require a real shell to parse them.
(We can't handle the = character.)

glandium points out on IRC that this is because the two calls to pymake.process.clinetoargv in https://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py should be adding a dummy first parameter before the call and then removing it afterwards, since clinetoargv is intended to parse an entire command line rather than just the arguments, and thus rejects things that it can't handle like "NSPR_LOG_FILE=nspr.log ./firefox".
(Or there should be a better way to call clinetoargv without having to do that.)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1181495
(Assignee)

Updated

2 years ago
Blocks: 1224452
(Assignee)

Comment 3

2 years ago
Created attachment 8689930 [details] [diff] [review]
Add a simplified version of pymake's clinetoargv to mozbuild and use it

Pymake's clinetoargv is very specific to pymake's use case, yet has been abused
as a replacement for shlex because shlex doesn't handle things properly for our
use cases.

Using pymake's clinetoargv, however, has shortcomings, and we're better off
importing its code in mozbuild, simplifying it a little, and using that
instead.

Plus, less dependencies on pymake will help kill it for good some day.

I'm also going to use the function added here to fix bug 1224452.
Assignee: nobody → mh+mozilla
Attachment #8689930 - Flags: review?(gps)
(Assignee)

Comment 4

2 years ago
Created attachment 8689933 [details] [diff] [review]
Add a simplified version of pymake's clinetoargv to mozbuild and use it

Might as well clean up the code as well... Now also flake8-happy.
Attachment #8689930 - Attachment is obsolete: true
Attachment #8689930 - Flags: review?(gps)
Attachment #8689933 - Flags: review?(gps)
(Assignee)

Comment 5

2 years ago
Created attachment 8689969 [details] [diff] [review]
Add a simplified version of pymake's clinetoargv to mozbuild and use it

A small fixup for extra_environment_variables.
Attachment #8689933 - Attachment is obsolete: true
Attachment #8689933 - Flags: review?(gps)
Attachment #8689969 - Flags: review?(gps)

Comment 6

2 years ago
Comment on attachment 8689969 [details] [diff] [review]
Add a simplified version of pymake's clinetoargv to mozbuild and use it

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

I had to download this patch and manually diff against build/pymake/pymake/process.py. Next time you copy a bunch of code, please create multiple patches so reviewers can look at a proper diff. Fortunately it looked like mostly cosmetic improvements and removal of the glob feature. Any more and I would have have cancelled the review.

Anyway, big +1 on killing the pymake dependency.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +910,2 @@
>                  print("The --debugger_args you passed require a real shell to parse them.")
> +                print("(We can't handle the %r character.)" % (e.char,))

While you're here, you could kill the () around the 1-tuple.
Attachment #8689969 - Flags: review?(gps) → review+

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c0c87d8cd2b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Depends on: 1286993

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.