Closed
Bug 649402
Opened 13 years ago
Closed 13 years ago
Make trysyntax mandatory
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: armenzg, Assigned: lsblakk)
References
()
Details
(Whiteboard: [trychooser])
Attachments
(3 files, 2 obsolete files)
843 bytes,
patch
|
catlee
:
review+
ted
:
feedback+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
rail
:
review+
lsblakk
:
checked-in+
|
Details | Diff | Splinter Review |
1.96 KB,
patch
|
ted
:
review+
lsblakk
:
checked-in+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → lsblakk
Reporter | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Attachment #532467 -
Flags: review?(catlee)
Assignee | ||
Comment 4•13 years ago
|
||
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.
Attachment #532467 -
Attachment is obsolete: true
Attachment #532467 -
Flags: review?(catlee)
Attachment #532468 -
Flags: review?(catlee)
Updated•13 years ago
|
Attachment #532468 -
Attachment is patch: false
Comment 5•13 years ago
|
||
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():
Attachment #532468 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #532468 -
Attachment is obsolete: true
Attachment #533725 -
Flags: review?(catlee)
Attachment #533725 -
Flags: feedback?(ted.mielczarek)
Updated•13 years ago
|
Attachment #533725 -
Flags: review?(catlee) → review+
Comment 7•13 years ago
|
||
Comment on attachment 533725 [details] [diff] [review] take 2 - make try mandatory with an hg hook Seems reasonable.
Attachment #533725 -
Flags: feedback?(ted.mielczarek) → feedback+
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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!)
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #537513 -
Flags: review?(catlee)
Assignee | ||
Comment 11•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #537513 -
Flags: review?(catlee) → review?(rail)
Updated•13 years ago
|
Attachment #537513 -
Flags: review?(rail) → review+
Assignee | ||
Comment 12•13 years ago
|
||
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
Attachment #537513 -
Flags: checked-in+
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #538033 -
Flags: review?(ted.mielczarek)
Comment 14•13 years ago
|
||
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).
Attachment #538033 -
Flags: review?(ted.mielczarek) → review+
Comment 15•13 years ago
|
||
With this change what's the syntax that will be required to run all the tests that are run on mozilla-central?
Comment 16•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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!)
Assignee | ||
Comment 21•13 years ago
|
||
(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.
Assignee | ||
Comment 22•13 years ago
|
||
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.
Assignee | ||
Comment 23•13 years ago
|
||
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
Attachment #538033 -
Flags: checked-in+
Assignee | ||
Comment 24•13 years ago
|
||
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)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•