649402 - Make trysyntax mandatory

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Armen [:armenzg]]

I know there are other improvements to come but we have been asked that imposing the syntax could improve the load on the try server and help with the terrible wait times until the new test pool comes up.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

So in order to try this, with the option to backout if it's too chaotic, instead of blocking on the interactive chooser - I will write a patch so that a push to try with no syntax generates a different email-on-push than what is currently created.

if you push with try syntax: you will get your builds, normal email (though there is a bug on having more info about what you're going to get in that initial email)

if you push without try syntax: you will get an email that says "try syntax not present" or something like that - No Builds will come of this, basically, and then links to trychooser and try_syntax wiki page to help guide the pusher.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

look at doing it as a hook instead of in the scheduler/email side of things - see bug 559964 where some of try parser is being incorporated.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: hg hook to make try syntax mandatory

There's no reason to implement any of the try_parser code here since even just using try: gets you a default set and I think that's enough to get the habit of using try syntax happening.  It will reduce unnecessary try talos runs at minimum, but also hopefully the links provided by the hook will get people looking at the trychooser and starting to think about what they need to request builds on.

The only issue with this hook is that it can only look at the tip changeset, so that means try syntax has to be present in the topmost commit...some people who currently use try and try syntax might find this frustrating to their current usage but I'm not sure how to get around it.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: hg hook to make try syntax mandatory

this is the correct version of the hook I was testing. It will not work with 'Try: ' (case sensitive) and it does work with multi-line commit messages.

Comment 5

[Chris AtLee [:catlee]]

Comment on attachment 532468 [details]
hg hook to make try syntax mandatory

If all you're looking for is "try:" in the comments, it would be much easier to do

if "try:" in repo.changetxt('tip').description():

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: take 2 - make try mandatory with an hg hook

you're right! | if "try: " not in | is much easier and concise.  

so this works, and it gives links to the info you might need if you were surprised by the "try syntax use is now mandatory" response.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 533725 [details] [diff] [review]
take 2 - make try mandatory with an hg hook

Seems reasonable.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Note to self - add a line saying "We're trying not to waste CPU cycles anymore, please use try syntax to ask for exactly what you want"

Comment 9

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

per irc with ehsan, catlee, jprmc: 

The objective here is to reduce load on testpool. Our feeling is that most people do not need "-a/-all", and are using so incorrectly. Therefore, please remove the "-a" option in trychooser, and people who do really need all, can specify using the longer syntax.

This can be done here, as part of this rollout, or can be broken out into separate followup bug... whatever makes most sense to lsblakk (doing the actual work!)

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: removing the -a option from try_parser

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

I've updated the trychooser site to remove the -a option, will land that fix in try_parser once catlee reviews it.  Also landed the hook and filed a bug for IT to enable it.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 537513 [details] [diff] [review]
removing the -a option from try_parser

Landed on default, updated https://wiki.mozilla.org/ReleaseEngineering/TryChooser to remove references to -a/--do-everything (and did some other updating).  This is ready to be swept up in the next reconfig.

http://hg.mozilla.org/build/buildbotcustom/rev/68f3606bd6a9

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: adding a couple of tests to make sure try_mandatory.hook does what it should

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 538033 [details] [diff] [review]
adding a couple of tests to make sure try_mandatory.hook does what it should

Review of attachment 538033 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! In the future, you don't need to get review on test changes, feel free to just push them (as long as the tests pass).

Comment 15

[Jeff Muizelaar [:jrmuizel]]

With this change what's the syntax that will be required to run all the tests that are run on mozilla-central?

Comment 16

[(no longer active)]

(In reply to comment #15)
> With this change what's the syntax that will be required to run all the tests
> that are run on mozilla-central?

try: -b do -p all -u all -t all

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #16)
> (In reply to comment #15)
> > With this change what's the syntax that will be required to run all the tests
> > that are run on mozilla-central?
> 
> try: -b do -p all -u all -t all

Isn't that just the same as try: -a?

Comment 18

[(no longer active)]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > With this change what's the syntax that will be required to run all the tests
> > > that are run on mozilla-central?
> > 
> > try: -b do -p all -u all -t all
> 
> Isn't that just the same as try: -a?

try: -a will be removed, IINM.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

So the goal is to save work under the assumption that 'try: -b do -p all -u all -t all' is harder to remember than 'try: -a' causing people to think more?

Comment 20

[(no longer active)]

(In reply to comment #19)
> So the goal is to save work under the assumption that 'try: -b do -p all -u all
> -t all' is harder to remember than 'try: -a' causing people to think more?

Most people do not need to use try: -a.  The goal is to make sure that people do not use it without thinking (or realizing) what it means.  If you need to really get all results, you can just use the longer syntax (which is quite easy too -- I typed it from memory!)

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to comment #19)
> So the goal is to save work under the assumption that 'try: -b do -p all -u
> all -t all' is harder to remember than 'try: -a' causing people to think
> more?

The goal is indeed to get people to think about what they need.  Also, for folks who choose to use http://people.mozilla.org/~lsblakk/trychooser/ to build their request, it's already been updated to reflect the new world without try: -a in it.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

also, the default is still to not have talos runs on every try push - and so to ask for '-t all' one hopefully intends to use/examine those results.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 538033 [details] [diff] [review]
adding a couple of tests to make sure try_mandatory.hook does what it should

http://hg.mozilla.org/hgcustom/hghooks/rev/4156bf664534

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Hook is in place.
http://hg.mozilla.org/try/pushloghtml shows lots of try syntax usage (and lots of try: -b do -p all -u all -t none)