TryChooser syntax is unfamiliar

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: lsblakk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
(Originally mentioned in bug 591686.)

Many (most?) Linux programs conform to a standard command-line idiom, but TryChooser doesn't.  This is a source of some confusion, at least for me.

Why is it --build and not --b when all the other flags are just one letter?  And
why do the one-letter flags get two dashes instead of just one?  Maybe there's a good reason for this.

I'd be much more at home with something like:

  -b --build {d,debug,o,opt,all} (can be combined as |-b do| or |--build debug,opt|)
  -d --desktop {linux,linux64,osx,osx64,win32,none,all} 
  etc.
(Reporter)

Updated

8 years ago
Blocks: 572808
(Reporter)

Comment 1

8 years ago
Also, |--build all| is confusing, because it triggers lots more than just builds.  Why not make a separate --do-everything flag?

At the very least, combining --build all with other flags should be an error.
(Assignee)

Comment 2

8 years ago
it's --build because originally I had the options be 'o' for opt, 'd' for debug, and 'b' when you wanted both opt & debug.  That changed to 'do' because someone asked for 'do' instead.  All the args get -- instead of - so that they are optional to argparse which is what was used to get this up and running quickly.

Since 'b' is no longer a --build option, it can certainly be changed to --b instead.  Moving the m-c style run to a --do-everything flag is also something that can be looked into.
(Assignee)

Comment 3

8 years ago
Created attachment 470468 [details] [diff] [review]
WIP - changing the try syntax to be more consistent

so this is a start - will need testing and to have the unittests updated
Assignee: nobody → lsblakk
(Reporter)

Comment 4

8 years ago
> All the args get -- instead of - so that they are optional to argparse

I played around with argparse and didn't notice a difference between

  parser.add_argument('--build', '-b')

and

  parser.add_argument('--b')

in terms of arguments being optional versus required.  At least, it didn't complain when I left off the flag in either case.  But I'm probably missing something.

Even with -- in front of the short arguments, the patch above makes me happy.  Thanks. :)
(Assignee)

Comment 5

8 years ago
> I played around with argparse and didn't notice a difference between
> 
>   parser.add_argument('--build', '-b')
> 
> and
> 
>   parser.add_argument('--b')
> 
> in terms of arguments being optional versus required.  At least, it didn't
> complain when I left off the flag in either case.  But I'm probably missing
> something.

good to know, i will test this then since if all args could be passed as -{b,p,m,u,t} that would be a nice reduction of chars needed in hg commit message.

Comment 6

8 years ago
Nice change. I'd suggest '--all' as a shorter version of '--do-everything', but no biggie.

Comment 7

8 years ago
Another thing that might be nice is to have '--u mochitests' be an alias for all of them. In most cases, people probably want all mochi, unless they're debugging a particular failure.
(Reporter)

Comment 8

8 years ago
Comment on attachment 470468 [details] [diff] [review]
WIP - changing the try syntax to be more consistent

Drive by review:

>+    parser.add_argument('--do-everything',
>+                        default='false',
>+                        dest='do_everything',
>+                        help='override try defaults to get an m-c comparable build run')

If you do instead

  parser.add_argument('--do-everything'
                      action='store_true',
                      dest='do_everything'
                      help='...')

then you can replace

>+    # Check for the m-c override to do all builds, tests, and talos
>+    if options.do_everything == 'true':

with

  if options.do_everything:

and we can use a bare "--do-everything" in the command line instead of "--do-everything true".

See http://docs.python.org/library/argparse.html#action
(Reporter)

Comment 9

8 years ago
Perhaps I'm still misreading the docs, but I ran

  try: --build d --p macosx64 --m none --u mochitests-2/5 --t none

which I expected to give me debug mochitest-2 on Mac64.  Instead, I didn't get any unit tests.

This syntax was motivated by the line:

  Unittest Suites: all || none || --u {crashtest,reftest,mochitests-{1/5,2/5,3/5,4/5,5/5},mochitest-other,xpcshell,etc}
(Reporter)

Comment 10

8 years ago
(In reply to comment 9)

Actually, I hear that try is really backed up on OSX right now; perhaps I didn't mess up the syntax after all.
(Reporter)

Comment 11

8 years ago
Not to hijack my own bug, but perhaps an even nicer way of fixing these problems would be to have tryserver *ask* what you'd like run when you push a build.  This could take the form of an interactive precommit hook on try.
Bug 591688 and bug 594236 track what you're suggesting.  This week I will be working on doing a round of improvements and clarifications to the syntax based on what came up in this bug. There's still valid changes to come from what you raised here.  Let me get those in and then we can look at what the next priority should be in terms of making try more self-serve.
Created attachment 473290 [details] [diff] [review]
[needs testing] round 2 of reworking try_parser syntax

Alright, this one passes local testing and I'll run it tomorrow in staging to make sure that working with an actual repo does the expected behaviour.

So new now is:

--do-everything, -all  which is the override and works if you just state it in the syntax

all the options now have short versions (-b,-p,etc)

you can now specify 'mochitests' in your unittests to get all mochitests or you can do a short-form of mochitest-n to get mochitests-n/5 -- note the mochitest-o for mochitest-other, that is in the dict so that it's caught in a 'mochitests' request.

Feedback welcome, I'd like to get this in soon (no downtime required) so that we can get folks the new functionality and hopefully more familiar syntax.
Attachment #470468 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #473290 - Flags: feedback?(catlee)
(Reporter)

Comment 14

8 years ago
(In reply to comment #13)
> --do-everything, -all  which is the override and works if you just state it in
> the syntax

I think the short form options should be a single letter for uniformity internally and with other command-line apps.  Perhaps -a would work.  Or maybe --do-everything doesn't need a short form.
Created attachment 473824 [details] [diff] [review]
[tested] improvements to the try_parser

I've tested this in staging for both build and test masters to make sure it works, the unittests on it have also been adjusted and I added a new test to make sure that doing 'try: -u mochitests' does in fact trigger all the mochitests.

Ready to land this anytime, and update the docs as well.
Attachment #473290 - Attachment is obsolete: true
Attachment #473824 - Flags: review?(catlee)
Attachment #473290 - Flags: feedback?(catlee)
Comment on attachment 473824 [details] [diff] [review]
[tested] improvements to the try_parser

Can you refresh the patch?  It doesn't apply cleanly for me.
Attachment #473824 - Flags: review?(catlee)
Created attachment 474080 [details] [diff] [review]
[tested] improvements to the try_parser

Sorry, I had some unrefreshed local changes.  Here's an updated version.
Attachment #473824 - Attachment is obsolete: true
Attachment #474080 - Flags: review?(catlee)

Updated

8 years ago
Attachment #474080 - Flags: review?(catlee) → review+
(Assignee)

Updated

8 years ago
Blocks: 593081
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.