Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11236012:
Add script for pub buildbots. (Closed)
|
Created:
8 years, 2 months ago by Bob Nystrom Modified:
8 years, 1 month ago CC:
reviews_dartlang.org, nweiz Visibility:
Public. |
DescriptionAdd script for pub buildbots.
Also modify annotated_steps to invoke it on the right bots.
Committed: https://code.google.com/p/dart/source/detail?r=14032
Patch Set 1 #
Total comments: 5
Patch Set 2 : Split reusable code out of pub buildbot script. #
Total comments: 4
Patch Set 3 : Reapply patch from old client. #Patch Set 4 : Refactor compiler buildbot to use bot.py. #Patch Set 5 : Move compiler buildbot script to tools/bots/. #
Total comments: 5
Patch Set 6 : Respond to review. #
Total comments: 2
Messages
Total messages: 15 (0 generated)
This is the buildbot script for the pub bots. Right now, it just runs the tests for pub itself. Once that's nice and stable I'm thinking of moving over some of the other "package-like" tests to it: args, dartdoc, etc. The bots themselves are already up and running (though failing, naturally) here: http://chromegw.corp.google.com/i/client.dart.fyi/console
https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py File client/tools/pub_buildbot.py (right): https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py#... client/tools/pub_buildbot.py:40: # For testing the bot locally, allow the user to pass in a buildbot name. :-D https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py#... client/tools/pub_buildbot.py:43: 'bot you would like to emulate (ex: web-chrome-win7)', default=None) nit: change the name to a pub-like bot name instead of web-chrome... https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py#... client/tools/pub_buildbot.py:96: def BuildSDK(mode, system): A lot of this is pretty much exactly copied code from the original script. Can we refactor to either 1) make this script import the other and call it's BuildSDK function, or 2) make both extend a "super-annotated-steps" or 3) at the very least, this script seems pretty similar in structure to the original -- add the pub stuff to the original script?
OK, I completely shuffled stuff around. There are now two modules: tools/bots/pub.py This is a tiny script that does just the pub-bot-specific stuff: parse the bot name and run the pub tests. It uses... tools/bots/bot.py This is intended to be a general module of bot-running functionality. I tried to write in a way that it would be easy to refactor utils/compiler/buildbot.py to use it. But I didn't actually do that refactoring. I'm afraid to poke at buildbot.py since it's not my code and it's easy to break the bots in non-visible. So maybe I can leave it like this where if someone (or me, later) who knows buildbot.py wants to start using bot.py, it should be easy. What do you think? https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py File client/tools/pub_buildbot.py (right): https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py#... client/tools/pub_buildbot.py:43: 'bot you would like to emulate (ex: web-chrome-win7)', default=None) On 2012/10/19 21:35:53, Emily Fortuna wrote: > nit: change the name to a pub-like bot name instead of web-chrome... Done. https://codereview.chromium.org/11236012/diff/1/client/tools/pub_buildbot.py#... client/tools/pub_buildbot.py:96: def BuildSDK(mode, system): On 2012/10/19 21:35:53, Emily Fortuna wrote: > A lot of this is pretty much exactly copied code from the original script. Can > we refactor to either 1) make this script import the other and call it's > BuildSDK function, or 2) make both extend a "super-annotated-steps" or 3) at the > very least, this script seems pretty similar in structure to the original -- add > the pub stuff to the original script? I don't want to add pub stuff to the original script because that script is subtly intertwined with the compiler tests in a lot of ways. I do definitely want to reuse code across these scripts though.
If we want to refactor stuff into a generalized file we should really make sure that we import that from everywhere to not have duplicated code, since you have most people involved in that code on the review I am sure we can get it right Additional general comments: Since all client/tools/buildbot_annotated_steps.py does in the case of pub is to call another script we might as well call this directly from the buildbot (I basically tell it what script to call, so we can easily change that for the pub bots) If we want to do this refactoring I suggest: rename tools/bots to tools/buildbot rename tools/bots/bot.py to tools/buildbot/utils.py rename tools/bots/pub.py to tools/buildbot/pub_annotated_steps.py move client/tools/buildbot_annotated_steps.py to tools/buildbot/client_annotated_steps.py If you don't want to do all this, please just place your script(s) inside client/tools https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... File client/tools/buildbot_annotated_steps.py (right): https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... client/tools/buildbot_annotated_steps.py:140: ''' maybe replicate the print from the other Process* methods (i.e., print 'ProcessPub')
On 2012/10/22 06:29:05, ricow1 wrote: > If we want to refactor stuff into a generalized file we should really make sure > that we import that from everywhere to not have duplicated code, since you have > most people involved in that code on the review I am sure we can get it right > Additional general comments: > Since all client/tools/buildbot_annotated_steps.py does in the case of pub is to > call another script we might as well call this directly from the buildbot (I > basically tell it what script to call, so we can easily change that for the pub > bots) > If we want to do this refactoring I suggest: > rename tools/bots to tools/buildbot > rename tools/bots/bot.py to tools/buildbot/utils.py > rename tools/bots/pub.py to tools/buildbot/pub_annotated_steps.py > move client/tools/buildbot_annotated_steps.py to > tools/buildbot/client_annotated_steps.py > > If you don't want to do all this, please just place your script(s) inside > client/tools > > https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... > File client/tools/buildbot_annotated_steps.py (right): > > https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... > client/tools/buildbot_annotated_steps.py:140: ''' > maybe replicate the print from the other Process* methods (i.e., print > 'ProcessPub') +1 most of Rico's comments. Can we also pull out the functions that you put into tools/bots/bot.py out of utils/compiler/buildbot.py. You can leave a more sophisticated refactoring for later, but just cut down on the same code in different places. After that adjustment, I think you're good. Rico, I actually prefer for all the bots to have a single "jumping off point" that way it's less magical and more discoverable for the average user to be able to figure out how the bot scripts are run without having to remember a number of different names for each case.
On 2012/10/22 06:29:05, ricow1 wrote: > If we want to refactor stuff into a generalized file we should really make sure > that we import that from everywhere to not have duplicated code, since you have > most people involved in that code on the review I am sure we can get it right Done. I was hoping to minimize the scope of this patch, but I think I have it working now. However, I would definitely really appreciate a thorough review of my changes to utils/compiler/buildbots/compiler.py (now tools/bots/compiler.py). I tested my changes by running the original script and my new one and comparing the total output. The only differences were timing and the ordering of some tests, which is expected. So that's good. But I don't know if there are edge cases, failure modes, or configuration details I'm not aware of. > Additional general comments: > Since all client/tools/buildbot_annotated_steps.py does in the case of pub is to > call another script we might as well call this directly from the buildbot (I > basically tell it what script to call, so we can easily change that for the pub > bots) Works for me. I hooked it into annotated steps because that was what I knew how to do, but if you want to have the bot invoke pub.py and compiler.py directly, I'm fine with that. Is there any necessary setup that annotated steps does? > If we want to do this refactoring I suggest: > rename tools/bots to tools/buildbot I called it "bots" so that we don't have a directory under tools that starts with "build". Otherwise, it messes up your shell completion when you go to invoke build.py. :) > rename tools/bots/bot.py to tools/buildbot/utils.py I kind of like the name "bot". "utils" is sort of meaningless. Do you care strongly here? > rename tools/bots/pub.py to tools/buildbot/pub_annotated_steps.py How come? > move client/tools/buildbot_annotated_steps.py to > tools/buildbot/client_annotated_steps.py I moved this to tools/bots/compiler.py. Is that OK? These aren't really "client" tests any more. They're the dart2js ones.
Made lots of changes. https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... File client/tools/buildbot_annotated_steps.py (right): https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... client/tools/buildbot_annotated_steps.py:140: ''' On 2012/10/22 06:29:05, ricow1 wrote: > maybe replicate the print from the other Process* methods (i.e., print > 'ProcessPub') Done.
On 2012/10/22 20:23:33, Emily Fortuna wrote: > On 2012/10/22 06:29:05, ricow1 wrote: > > If we want to refactor stuff into a generalized file we should really make > sure > > that we import that from everywhere to not have duplicated code, since you > have > > most people involved in that code on the review I am sure we can get it right > > Additional general comments: > > Since all client/tools/buildbot_annotated_steps.py does in the case of pub is > to > > call another script we might as well call this directly from the buildbot (I > > basically tell it what script to call, so we can easily change that for the > pub > > bots) > > If we want to do this refactoring I suggest: > > rename tools/bots to tools/buildbot > > rename tools/bots/bot.py to tools/buildbot/utils.py > > rename tools/bots/pub.py to tools/buildbot/pub_annotated_steps.py > > move client/tools/buildbot_annotated_steps.py to > > tools/buildbot/client_annotated_steps.py > > > > If you don't want to do all this, please just place your script(s) inside > > client/tools > > > > > https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... > > File client/tools/buildbot_annotated_steps.py (right): > > > > > https://codereview.chromium.org/11236012/diff/4001/client/tools/buildbot_anno... > > client/tools/buildbot_annotated_steps.py:140: ''' > > maybe replicate the print from the other Process* methods (i.e., print > > 'ProcessPub') > > +1 most of Rico's comments. Can we also pull out the functions that you put into > tools/bots/bot.py out of utils/compiler/buildbot.py. You can leave a more > sophisticated refactoring for later, but just cut down on the same code in > different places. After that adjustment, I think you're good. Done. Yay for code reuse! > Rico, I actually prefer for all the bots to have a single "jumping off point" > that way it's less magical and more discoverable for the average user to be able > to figure out how the bot scripts are run without having to remember a number of > different names for each case. I'm agnostic, though I lean towards minimizing my (already frighteningly large) changes. PTAL and let me know what you think of the latest. Thanks!
looking good! Just one concern and then I'm happy with committing it https://codereview.chromium.org/11236012/diff/4001/tools/bots/bot.py File tools/bots/bot.py (right): https://codereview.chromium.org/11236012/diff/4001/tools/bots/bot.py#newcode31 tools/bots/bot.py:31: - compiler: 'dart2js' or None when the builder has an incorrect name Some of these comments should be updated if this is going to be general purpose. https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py File tools/bots/compiler.py (right): https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py#new... tools/bots/compiler.py:101: with bot.BuildStep(step_name, swallow_error=True): Fancy! (I had to look up how to use this) BUT, I'm not 100% certain that this is maintaining the original semantics. The script returns failure if the exit code is nonzero, whereas the new code only reports failure if an exception is thrown (and test.dart can return a nonzero exit code with no additional information, I believe)
Thanks! https://codereview.chromium.org/11236012/diff/4001/tools/bots/bot.py File tools/bots/bot.py (right): https://codereview.chromium.org/11236012/diff/4001/tools/bots/bot.py#newcode31 tools/bots/bot.py:31: - compiler: 'dart2js' or None when the builder has an incorrect name On 2012/10/23 23:09:49, Emily Fortuna wrote: > Some of these comments should be updated if this is going to be general purpose. Good catch. Done. https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py File tools/bots/compiler.py (right): https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py#new... tools/bots/compiler.py:101: with bot.BuildStep(step_name, swallow_error=True): On 2012/10/23 23:09:49, Emily Fortuna wrote: > Fancy! (I had to look up how to use this) I had to look it up to create it! I wanted a nice automatic way of printing the fail steps when a process borked. > BUT, I'm not 100% certain that this is maintaining the original semantics. The > script returns failure if the exit code is nonzero, whereas the new code only > reports failure if an exception is thrown (and test.dart can return a nonzero > exit code with no additional information, I believe) I believe it is, but please do double check. In the original code, TestStep() returns the status code but TestCompiler(), which calls it, always just ignores it and then returns zero. As far as I can tell, in the original code, the exit status from running test.dart is never used. I guess it relies on the test output messages? With my change, what *should* happen is that bot.RunProcess() will see a non-zero exit status, and raise an OSError. with bot.BuildStep() will catch that, print the failure and (because swallow_error is True), eat the error and continue. Does that sound right?
https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py File tools/bots/compiler.py (left): https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py#old... tools/bots/compiler.py:191: if exit_code != 0: My concern is we check the exit code from running test.dart is here, but with __exit__ only an exception is checked for.
https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py File tools/bots/compiler.py (left): https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py#old... tools/bots/compiler.py:191: if exit_code != 0: On 2012/10/24 03:00:04, Emily Fortuna wrote: > My concern is we check the exit code from running test.dart is here, but with > __exit__ only an exception is checked for. Exactly right. If you look at bot.RunProcess(), though, it should translate a non-zero exit code to an exception, so that this gets hit. The previous code was doing tons of "if exit_code != 0: return exit_code" to early out and to me that looked like hand-implemented exception unwinding. So I just made it use exceptions directly. :)
lgtm! https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py File tools/bots/compiler.py (left): https://codereview.chromium.org/11236012/diff/1004/tools/bots/compiler.py#old... tools/bots/compiler.py:191: if exit_code != 0: On 2012/10/24 16:22:28, Bob Nystrom wrote: > On 2012/10/24 03:00:04, Emily Fortuna wrote: > > My concern is we check the exit code from running test.dart is here, but with > > __exit__ only an exception is checked for. > > Exactly right. If you look at bot.RunProcess(), though, it should translate a > non-zero exit code to an exception, so that this gets hit. > > The previous code was doing tons of "if exit_code != 0: return exit_code" to > early out and to me that looked like hand-implemented exception unwinding. So I > just made it use exceptions directly. :) Gotcha. Carry on then!
https://codereview.chromium.org/11236012/diff/12/tools/bots/pub.py File tools/bots/pub.py (right): https://codereview.chromium.org/11236012/diff/12/tools/bots/pub.py#newcode3 tools/bots/pub.py:3: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Us the dart authors copyright note, I assume that the copyright note in the original annotated steps file is a copy of something from chromium?
https://codereview.chromium.org/11236012/diff/12/tools/bots/pub.py File tools/bots/pub.py (right): https://codereview.chromium.org/11236012/diff/12/tools/bots/pub.py#newcode3 tools/bots/pub.py:3: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/10/25 06:49:30, ricow1 wrote: > Us the dart authors copyright note, I assume that the copyright note in the > original annotated steps file is a copy of something from chromium? Yeah, I think so. Fixed here: https://codereview.chromium.org/11260052/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
