Justin Lebar (not reading bugmail) Reporter
	Description • 14 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.

	Justin Lebar (not reading bugmail) Reporter
	Comment 1 • 14 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.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 2 • 14 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.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 3 • 14 years ago

so this is a start - will need testing and to have the unittests updated

	Justin Lebar (not reading bugmail) Reporter
	Comment 4 • 14 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. :)

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 5 • 14 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.

	dwitte@gmail.com
	Comment 6 • 14 years ago

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

	dwitte@gmail.com
	Comment 7 • 14 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.

	Justin Lebar (not reading bugmail) Reporter
	Comment 8 • 14 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

	Justin Lebar (not reading bugmail) Reporter
	Comment 9 • 14 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}

	Justin Lebar (not reading bugmail) Reporter
	Comment 10 • 14 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.

	Justin Lebar (not reading bugmail) Reporter
	Comment 11 • 14 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.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 12 • 14 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 13 • 14 years ago

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.

	Justin Lebar (not reading bugmail) Reporter
	Comment 14 • 14 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.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 15 • 14 years ago

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.

	Chris AtLee [:catlee]
	Comment 16 • 14 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 17 • 14 years ago

Sorry, I had some unrefreshed local changes.  Here's an updated version.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 18 • 14 years ago

Comment on attachment 474080 [details] [diff] [review]
[tested] improvements to the try_parser

http://hg.mozilla.org/build/buildbotcustom/rev/23afaa514868