Closed
Bug 1236382
Opened 9 years ago
Closed 9 years ago
mach try doesn't handle --rebuild properly
Categories
(Testing :: General, defect)
Testing
General
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: xidorn, Assigned: chmanchester)
References
Details
Attachments
(1 file)
$ ./mach try -b do -p linux,linux64,linux64-asan -u mochitest-2 -t none --rebuild 20
mach try is under development, please file bugs blocking 1149670.
warning: failed to set color mode to win32
Specified path "mozilla-source\central\20" is not a directory under the srcdir, unable to specify tests outside of the srcdir
Assignee | ||
Comment 1•9 years ago
|
||
Hmm, this looks like a conflict between using nargs='*' and nargs=REMAINDER, so we have '20' interpreted as a path to tests. In the presence on this conflict I'd guess we're better off adding --rebuild as a regular argument and not having extra_args present at all.
Assignee | ||
Comment 2•9 years ago
|
||
Using nargs='*' in conjunction with nargs=REMAINDER creates an ambiguity when
the argument using nargs='*' is optional, it is not specified, and the user
intends their arguments to be interpreted as "extra" arguments. This commit
removes the nargs=REMAINDER argument for mach_try, and implements its most
common use, "--rebuild", as a regular argument.
Review commit: https://reviewboard.mozilla.org/r/29485/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29485/
Attachment #8703834 -
Flags: review?(james)
Comment 3•9 years ago
|
||
Agreed that this is not ideal, but I think if you want to take the approach of explicitly providing every known command line option (which may indeed be the only approach that works well), you need at least the ones exposed by try chooser (--rebuild=N --no-retry --spsProfile --retry-talos=N).
Assignee | ||
Updated•9 years ago
|
Attachment #8703834 -
Attachment description: MozReview Request: Bug 1236382 - Add --rebuild as an argument to mach try, remove the extra arguments functionality. r=jgraham → MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8703834 [details]
MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29485/diff/1-2/
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8703834 [details]
MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29485/diff/2-3/
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → cmanchester
Assignee | ||
Updated•9 years ago
|
Attachment #8703834 -
Flags: review?(james)
Assignee | ||
Updated•9 years ago
|
Attachment #8703834 -
Flags: review?(james)
Updated•9 years ago
|
Attachment #8703834 -
Flags: review?(james)
Comment 9•9 years ago
|
||
Comment on attachment 8703834 [details]
MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham
https://reviewboard.mozilla.org/r/29485/#review30077
(sorry I think I forgot to press the "Finish Review" button)
::: testing/mach_commands.py:558
(Diff revision 2)
> + if k in extra_values and v}
I guess technically this is broken for arguments that are default true but can be set to false, or cases where multiple arguments have the same dest. I don't know how much that's worth worrying about though.
::: testing/tools/autotry/autotry.py:193
(Diff revision 2)
> + }
It seems like --interactive is also a thing.
::: testing/tools/autotry/autotry.py:355
(Diff revision 2)
> + assert dest in args_by_dest
I'm not sure this assert is adding much value
::: testing/tools/autotry/autotry.py:355
(Diff revision 2)
> + assert dest in args_by_dest
I'm not sure this assert is adding much value.
::: testing/tools/autotry/autotry.py:355
(Diff revision 2)
> + assert dest in args_by_dest
I'm not sure this assert is adding much value
Assignee | ||
Comment 10•9 years ago
|
||
https://reviewboard.mozilla.org/r/29485/#review30077
> I guess technically this is broken for arguments that are default true but can be set to false, or cases where multiple arguments have the same dest. I don't know how much that's worth worrying about though.
Yeah, I think we can ignore store_false arguments for the time being. Multiple arguments with the same dest are always going to be a problem with argparse, right?
Assignee | ||
Updated•9 years ago
|
Attachment #8703834 -
Flags: review?(james)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8703834 [details]
MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29485/diff/3-4/
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/29485/#review30077
> Yeah, I think we can ignore store_false arguments for the time being. Multiple arguments with the same dest are always going to be a problem with argparse, right?
OK. Could you add a comment where these arguments are defined to indicate the cases that are known not to work, just to help future-us.
Updated•9 years ago
|
Attachment #8703834 -
Flags: review?(james) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8703834 [details]
MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham
https://reviewboard.mozilla.org/r/29485/#review30683
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•