1236382 - mach try doesn't handle --rebuild properly

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

$ ./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

Comment 1

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1236382 - Add commonly used arguments to mach try, remove the extra arguments functionality. r=jgraham

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/

Comment 3

[James Graham [:jgraham]]

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).

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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 7

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2fe235fe70

Comment 8

[Chris Manchester (limited bugmail, email directly)]

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/

Comment 9

[James Graham [:jgraham]]

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

Comment 10

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 11

[Chris Manchester (limited bugmail, email directly)]

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

[James Graham [:jgraham]]

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.

Comment 13

[James Graham [:jgraham]]

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

[Pulsebot]

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

Comment 15

[Carsten Book [:Tomcat]]

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