Closed Bug 1236382 Opened 4 years ago Closed 4 years ago

mach try doesn't handle --rebuild properly

Categories

(Testing :: General, defect)

defect
Not set

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
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.
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)
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).
Duplicate of this bug: 1241833
Duplicate of this bug: 1244126
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
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/
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: nobody → cmanchester
Attachment #8703834 - Flags: review?(james)
Attachment #8703834 - Flags: review?(james)
Attachment #8703834 - Flags: review?(james)
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
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?
Attachment #8703834 - Flags: review?(james)
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/
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.
Attachment #8703834 - Flags: review?(james) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c654c659bac1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.