Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15261003:
Add a new script bb_host_steps.py which handles all host side steps. (Closed)
|
Created:
7 years, 7 months ago by Siva Chandra Modified:
7 years, 6 months ago Reviewers:
Isaac (away) CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org, navabi1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a new script bb_host_steps.py which handles all host side steps.
BUG=154525
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204970
Patch Set 1 #Patch Set 2 : Remove a command line arg #
Total comments: 1
Patch Set 3 : Adding bb_host_steps.py #Patch Set 4 : rebase #Patch Set 5 : Use bb_host_steps.py for the fyi builder #Patch Set 6 : Fixed a path #Patch Set 7 : Remove unwanted quotes #Patch Set 8 : Do not flunk on failure for experimental targets #Patch Set 9 : Use bb_host_steps.py for 'android_dbg' #Patch Set 10 : Source buildbot_functions only once #Patch Set 11 : Fix string vs list mixup #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : Add check_webview_licenses step to 'Host Steps' #Patch Set 15 : Patch set for initial review #
Total comments: 18
Patch Set 16 : A round of response to ilevy #Patch Set 17 : rebase #Patch Set 18 : Address all of ilevy's code comments #Patch Set 19 : rebase #Patch Set 20 : pass -l20 to ninja #Patch Set 21 : Run 'gclient runhooks' #Patch Set 22 : Add '--extract-build' option to fyi-tests #Patch Set 23 : First Full CL #
Total comments: 18
Patch Set 24 : Address ilevy's nits #Patch Set 25 : rebase #
Total comments: 18
Patch Set 26 : Another round of nits #Patch Set 27 : 'Environment setup' is not a step #
Total comments: 27
Patch Set 28 : Fixed presubmit (pylint) errors #Patch Set 29 : Addressing a around of comments #Patch Set 30 : rebase #Patch Set 31 : Wrap slave_properties with quotes #
Total comments: 18
Patch Set 32 : Addressing another round of comments #Patch Set 33 : Addressing a missed comment #Patch Set 34 : yet another rebase #Messages
Total messages: 28 (0 generated)
https://chromiumcodereview.appspot.com/15261003/diff/3001/build/android/build... File build/android/buildbot/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/15261003/diff/3001/build/android/build... build/android/buildbot/buildbot_functions.sh:125: python /b/build/scripts/slave/compile.py \ Looks great! 1) Can we remove the bash and put this directly in python (please create bb_host_steps.py)? 2) Do you need to setup goma if we're using compile.py?
Oh, also you can't rely on /b/ as the top level directory. If you put this in python I believe we already have logic for finding the that folder's location.
On 2013/05/21 23:38:38, Isaac wrote: > https://chromiumcodereview.appspot.com/15261003/diff/3001/build/android/build... > File build/android/buildbot/buildbot_functions.sh (right): > > https://chromiumcodereview.appspot.com/15261003/diff/3001/build/android/build... > build/android/buildbot/buildbot_functions.sh:125: python > /b/build/scripts/slave/compile.py \ > Looks great! > > 1) Can we remove the bash and put this directly in python (please create > bb_host_steps.py)? OK. All will be done in this CL. > 2) Do you need to setup goma if we're using compile.py? No. I just put together a proof of concept.
On 2013/05/21 23:38:38, Isaac wrote: > https://chromiumcodereview.appspot.com/15261003/diff/3001/build/android/build... > File build/android/buildbot/buildbot_functions.sh (right): > > https://chromiumcodereview.appspot.com/15261003/diff/3001/build/android/build... > build/android/buildbot/buildbot_functions.sh:125: python > /b/build/scripts/slave/compile.py \ > Looks great! > > 1) Can we remove the bash and put this directly in python (please create > bb_host_steps.py)? > > 2) Do you need to setup goma if we're using compile.py? Hi, I now have added a bb_host_steps.py file and refactored the code around it to use it. The 'compile' step seems to be failing for some reason which I do not yet understand. Also, bb_run_bot.py needs to be cleaned up. I would like a review at this point to get feedback about the general direction. Also, if you know why the compile step is failing in the way it is, then that can help this CL much. I have a lot of patch sets in the CL which were staging in the changes. But you should only look at patch set 15 (named 'Patch set for initial review'). Thanks, Siva Chandra
Looks like a great start. Could you add the deletion of buildbot_functions.sh to this CL? Some comments inline. Thanks Siva!! https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_device_steps.py:13: change to import bb_utils https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_device_steps.py:302: parser.add_option('--build-properties', action='callback', let's make a function in bb_utils "GetParser()" which adds build-props, slave_props and factory_props already. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_device_steps.py:330: return OptParserError(parser, 'Unused args %s' % args) GetParser should be able to return a parser with a custom error function. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:36: COMPILER = 'goma' remove variables on lines 21-26, 34-36 and add them directly in the functions. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:109: if step == 'check_webview_licenses': can we make check_webview_licenses and findbugs a single option like '--host-tests' ? https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:111: if step == 'compile': don't we always want to compile? https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:113: if step == 'compile_experimental': add option --experimental instead of separate step https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:117: if step == 'zip_build': add '--zip-build' option https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/pyli... File build/android/pylib/constants.py (right): https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/pyli... build/android/pylib/constants.py:14: BB_BUILD_DIR = os.path.abspath( keep these out of pylib, put in bb_utils.
Thanks for the quick review. I have not made code changes, but have few points to make and get clarifications before I make code changes. I have prepared this CL with a specific idea about the various build steps. We (will after this CL lands) have three different groups of build steps: A. Build steps coming from bash functions (for example, bb_baseline_setup IMO cannot be converted to Python), B. Build steps which do not have anything to do with the devices attached to the slave (Host Steps), and C. Build steps which deal with devices attached to the slave (Test steps). A builder will have steps only from group A and group B, while a tester will have steps from all three groups. The steps from a single group are all executed before steps from a next group are executed. Also, steps from group A are executed before steps from B, and steps from group B are executed before steps from group C. Within a group, the order of steps to be executed is determined by the order in which the steps are specified in bb_run_bot.py for each group. I have structured this CL to align with the above idea. If you feel that it is incorrect to have such a grouping and/or ordering of steps, then please let me know your thoughts on how it should be. My idea was driven by the fact that ordering is important (for example, we cannot zip_build before compile), and that setting up of a new bot should only be a matter of picking and choosing different steps and stringing them together in the desired order. There is another ulterior motive: except bb_run_bot.py, all other files should be treated as libraries. This will enable us to use the 'library' of scripts downstream with bb_run_bot.py being the upstream client for this library. With the above idea in mind, I have few other comments inline below. On 2013/05/29 20:28:07, Isaac wrote: > Looks like a great start. Could you add the deletion of buildbot_functions.sh > to this CL? Will do as the last step. > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:109: if step == > 'check_webview_licenses': > can we make check_webview_licenses and findbugs a single option like > '--host-tests' ? Having a separate 'category' called 'host-tests' will not provide a way to order the steps. For example, check_webview_licenses is run before compile and findbugs is run after compile. How does one convey that a compile has to happen between these two steps? See below for a further explanation. > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:111: if step == 'compile': > don't we always want to compile? No. In a tester, we will run the 'extract_build' host step but not run the 'compile' step. > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:113: if step == 'compile_experimental': > add option --experimental instead of separate step > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:117: if step == 'zip_build': > add '--zip-build' option While I am OK to do this change, I want to put forward my view of things here. All options to bb_host_steps should be options to the steps; they should not specify the steps themselves. The reason I want it this way is because I do not want bb_host_steps.py to have the intelligence to order the steps. It should only know the list of possible steps and the intelligence to execute them. The ordering of steps should be driven by bb_run_bot.py. The code level view is that, except bb_run_bot.py, all other files should be treated as libraries. This will enable us to use the 'library' of scripts downstream (probably after extending).
On 2013/05/29 23:38:28, Siva Chandra wrote: > Thanks for the quick review. I have not made code changes, but have few points > to make and get clarifications before I make code changes. > > I have prepared this CL with a specific idea about the various build steps. We > (will after this CL lands) have three different groups of build steps: A. Build > steps coming from bash functions (for example, bb_baseline_setup IMO cannot be > converted to Python), B. Build steps which do not have anything to do with the > devices attached to the slave (Host Steps), and C. Build steps which deal with > devices attached to the slave (Test steps). A builder will have steps only from > group A and group B, while a tester will have steps from all three groups. The > steps from a single group are all executed before steps from a next group are > executed. Also, steps from group A are executed before steps from B, and steps > from group B are executed before steps from group C. Within a group, the order > of steps to be executed is determined by the order in which the steps are > specified in bb_run_bot.py for each group. bb_baseline_setup sets up the environment. It can be converted to a python function which returns a environment dict to be passed to subprocess.popen in bb_run_bot. It seems reasonable to assume group B comes before group C. The order within the group is not important, but the logic of bb_host_steps will naturally generate a consistent order if it successively checks options and then runs each step or not. > I have structured this CL to align with the above idea. If you feel that it is > incorrect to have such a grouping and/or ordering of steps, then please let me > know your thoughts on how it should be. My idea was driven by the fact that > ordering is important (for example, we cannot zip_build before compile), and > that setting up of a new bot should only be a matter of picking and choosing > different steps and stringing them together in the desired order. There is > another ulterior motive: except bb_run_bot.py, all other files should be treated > as libraries. This will enable us to use the 'library' of scripts downstream > with bb_run_bot.py being the upstream client for this library. I think bb_run_bot should also be a 'library', in the sense that it should be rewritten to either be importable by the private codebase or should read a config file, that can be the public steps or private steps. If you look at what our private code does, it's essentially identical to public steps except for the addition of private code and different build logic in gyp. > With the above idea in mind, I have few other comments inline below. > > On 2013/05/29 20:28:07, Isaac wrote: > > Looks like a great start. Could you add the deletion of buildbot_functions.sh > > to this CL? > > Will do as the last step. Please include all changes (incl buildbot_function deletions) in your CL before next review. > > > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > > build/android/buildbot/bb_host_steps.py:109: if step == > > 'check_webview_licenses': > > can we make check_webview_licenses and findbugs a single option like > > '--host-tests' ? > > Having a separate 'category' called 'host-tests' will not provide a way to order > the steps. For example, check_webview_licenses is run before compile and > findbugs is run after compile. How does one convey that a compile has to happen > between these two steps? See below for a further explanation. > I don't see how this prevents ordering the steps. You can run these tests at a specific time -- between the compile and the zip, perhaps. > > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > > build/android/buildbot/bb_host_steps.py:111: if step == 'compile': > > don't we always want to compile? > > No. In a tester, we will run the 'extract_build' host step but not run the > 'compile' step. Right, you always want to either compile or extract, and never both. This seems like a better candidate for a boolean option. > > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > > build/android/buildbot/bb_host_steps.py:113: if step == > 'compile_experimental': > > add option --experimental instead of separate step > > > > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > > build/android/buildbot/bb_host_steps.py:117: if step == 'zip_build': > > add '--zip-build' option > > While I am OK to do this change, I want to put forward my view of things here. > All options to bb_host_steps should be options to the steps; they should not > specify the steps themselves. The reason I want it this way is because I do not > want bb_host_steps.py to have the intelligence to order the steps. It should > only know the list of possible steps and the intelligence to execute them. The > ordering of steps should be driven by bb_run_bot.py. The code level view is > that, except bb_run_bot.py, all other files should be treated as libraries. This > will enable us to use the 'library' of scripts downstream (probably after > extending). I would rather the options passed to bb_host_steps have no effect on the order of the steps executed, with that order hard coded into the main function of bb_host_steps, similar to bb_device_steps.
Please review patch set 23. I have addressed all of your comments. The try jobs are running and the last I have checked, they have been doing OK. The testers are having a huge backlog though. I have not yet removed bb_baseline_setup. I would prefer to do it in a different CL. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_device_steps.py:13: On 2013/05/29 20:28:07, Isaac wrote: > change to > > import bb_utils Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_device_steps.py:302: parser.add_option('--build-properties', action='callback', On 2013/05/29 20:28:07, Isaac wrote: > let's make a function in bb_utils "GetParser()" which adds build-props, > slave_props and factory_props already. Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_device_steps.py:330: return OptParserError(parser, 'Unused args %s' % args) On 2013/05/29 20:28:07, Isaac wrote: > GetParser should be able to return a parser with a custom error function. Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:36: COMPILER = 'goma' On 2013/05/29 20:28:07, Isaac wrote: > remove variables on lines 21-26, 34-36 and add them directly in the functions. Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:109: if step == 'check_webview_licenses': On 2013/05/29 20:28:07, Isaac wrote: > can we make check_webview_licenses and findbugs a single option like > '--host-tests' ? Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:111: if step == 'compile': On 2013/05/29 20:28:07, Isaac wrote: > don't we always want to compile? Done. Compile comes from a boolean option now. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:113: if step == 'compile_experimental': On 2013/05/29 20:28:07, Isaac wrote: > add option --experimental instead of separate step Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... build/android/buildbot/bb_host_steps.py:117: if step == 'zip_build': On 2013/05/29 20:28:07, Isaac wrote: > add '--zip-build' option Done. https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/pyli... File build/android/pylib/constants.py (right): https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/pyli... build/android/pylib/constants.py:14: BB_BUILD_DIR = os.path.abspath( On 2013/05/29 20:28:07, Isaac wrote: > keep these out of pylib, put in bb_utils. Done.
On 2013/06/04 00:34:03, Siva Chandra wrote: > Please review patch set 23. I have addressed all of your comments. The try jobs > are running and the last I have checked, they have been doing OK. The testers > are having a huge backlog though. > > I have not yet removed bb_baseline_setup. I would prefer to do it in a different > CL. Removing code you are replacing should be part of the same CL. It simplifies reverts and makes everything atomic. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > File build/android/buildbot/bb_device_steps.py (right): > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_device_steps.py:13: > On 2013/05/29 20:28:07, Isaac wrote: > > change to > > > > import bb_utils > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_device_steps.py:302: > parser.add_option('--build-properties', action='callback', > On 2013/05/29 20:28:07, Isaac wrote: > > let's make a function in bb_utils "GetParser()" which adds build-props, > > slave_props and factory_props already. > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_device_steps.py:330: return OptParserError(parser, > 'Unused args %s' % args) > On 2013/05/29 20:28:07, Isaac wrote: > > GetParser should be able to return a parser with a custom error function. > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > File build/android/buildbot/bb_host_steps.py (right): > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:36: COMPILER = 'goma' > On 2013/05/29 20:28:07, Isaac wrote: > > remove variables on lines 21-26, 34-36 and add them directly in the functions. > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:109: if step == > 'check_webview_licenses': > On 2013/05/29 20:28:07, Isaac wrote: > > can we make check_webview_licenses and findbugs a single option like > > '--host-tests' ? > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:111: if step == 'compile': > On 2013/05/29 20:28:07, Isaac wrote: > > don't we always want to compile? > > Done. Compile comes from a boolean option now. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:113: if step == 'compile_experimental': > On 2013/05/29 20:28:07, Isaac wrote: > > add option --experimental instead of separate step > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/buil... > build/android/buildbot/bb_host_steps.py:117: if step == 'zip_build': > On 2013/05/29 20:28:07, Isaac wrote: > > add '--zip-build' option > > Done. > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/pyli... > File build/android/pylib/constants.py (right): > > https://chromiumcodereview.appspot.com/15261003/diff/43001/build/android/pyli... > build/android/pylib/constants.py:14: BB_BUILD_DIR = os.path.abspath( > On 2013/05/29 20:28:07, Isaac wrote: > > keep these out of pylib, put in bb_utils. > > Done.
Looking good! Here's another set of style nits to apply, and then I'll do a more thorough pass on the exact logic. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_device_steps.py:60: R = bb_utils.RunCmd This is fine, but how about we name this RunCmd? Makes it a little easier to parse the lines below for me. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:40: webview_licenses_script = 'android_webview/tools/webview_licenses.py' remove this var https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:46: opts = ['--build-tool=ninja', change this var to 'cmd' and remove compile_script var https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:52: buildbot_report.PrintNamedStep('Exerimental Compile %s' % compile_target) Experimental https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:65: zip_build_script = os.path.join(SLAVE_SCRIPTS_DIR, 'zip_build.py') remove args and zip_build_script vars https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:76: extract_build_script = os.path.join(SLAVE_SCRIPTS_DIR, 'extract_build.py') remove these vars https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:78: '--build-output-dir', '../out', pass absolute directory https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:88: findbugs_diff_script = 'build/android/findbugs_diff.py' remove these vars https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:96: script = os.path.join(constants.CHROME_DIR, 'tools', 'clang', 'scripts', don't make variable, just run directly.
PTAL https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_device_steps.py:60: R = bb_utils.RunCmd On 2013/06/04 04:34:12, Isaac wrote: > This is fine, but how about we name this RunCmd? Makes it a little easier to > parse the lines below for me. Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:40: webview_licenses_script = 'android_webview/tools/webview_licenses.py' On 2013/06/04 04:34:12, Isaac wrote: > remove this var Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:46: opts = ['--build-tool=ninja', On 2013/06/04 04:34:12, Isaac wrote: > change this var to 'cmd' and remove compile_script var Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:52: buildbot_report.PrintNamedStep('Exerimental Compile %s' % compile_target) On 2013/06/04 04:34:12, Isaac wrote: > Experimental Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:65: zip_build_script = os.path.join(SLAVE_SCRIPTS_DIR, 'zip_build.py') On 2013/06/04 04:34:12, Isaac wrote: > remove args and zip_build_script vars Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:76: extract_build_script = os.path.join(SLAVE_SCRIPTS_DIR, 'extract_build.py') On 2013/06/04 04:34:12, Isaac wrote: > remove these vars Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:78: '--build-output-dir', '../out', On 2013/06/04 04:34:12, Isaac wrote: > pass absolute directory Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:88: findbugs_diff_script = 'build/android/findbugs_diff.py' On 2013/06/04 04:34:12, Isaac wrote: > remove these vars Done. https://chromiumcodereview.appspot.com/15261003/diff/74006/build/android/buil... build/android/buildbot/bb_host_steps.py:96: script = os.path.join(constants.CHROME_DIR, 'tools', 'clang', 'scripts', On 2013/06/04 04:34:12, Isaac wrote: > don't make variable, just run directly. Done.
https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_host_steps.py:21: EXCLUDE_FILES = 'lib.target,gen,android_webview,jingle_unittests' remove this variable. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_host_steps.py:49: '--goma-dir=%s' % bb_utils.GOMA_DIR,] trailing comma https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_host_steps.py:90: def AsanTestsSetup(): How about 'UpdateClang()'? https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_run_bot.py (left): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_run_bot.py:209: if command_obj.step_name: why are you removing this? https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_run_bot.py (right): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_run_bot.py:51: commands = [Command('Preparation', common_cmd, None)] Why are you changing this? https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_run_bot.py:190: def CommandToString(command): move this function to bb_utils https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_run_bot.py:208: ['bash', '-exc', '; '.join(commands)], cwd=CHROME_SRC, why are you combining the commands? https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_utils.py (right): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_utils.py:23: GOMA_DIR = os.path.join(BB_BUILD_DIR, 'goma') remove this variable. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_utils.py:53: code, flunk_on_failure, halt_on_failure = retcode_callback(code) I don't think we need this callback - can you pass a optional warning_code arg (defaulting to 88) that specifies what code to not worry about instead? https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_utils.py:71: parser = BBOptionParser() Actually, I think it would be OK to use a regular OptionParser here. Looking at our usage, I think sys.exit() could be OK since bb_device_tests is spawned as a separate process.
PTAL. I have done all the requested code changes and answered the questions raised. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_host_steps.py:21: EXCLUDE_FILES = 'lib.target,gen,android_webview,jingle_unittests' On 2013/06/05 03:03:07, Isaac wrote: > remove this variable. Done. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_host_steps.py:49: '--goma-dir=%s' % bb_utils.GOMA_DIR,] On 2013/06/05 03:03:07, Isaac wrote: > trailing comma Done. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_host_steps.py:90: def AsanTestsSetup(): On 2013/06/05 03:03:07, Isaac wrote: > How about 'UpdateClang()'? Done. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_run_bot.py (right): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_run_bot.py:51: commands = [Command('Preparation', common_cmd, None)] On 2013/06/05 03:03:07, Isaac wrote: > Why are you changing this? This reply is for this comment and for another comment below. We had a function 'WrapWithBash' which was being called to wrap bash functions as well the invocation of bb_device_steps.py. Because of this, the step 'Environment Setup' was being executed twice: http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... If I were to WrapWithBash even the invocation of bb_host_steps.py, the step 'Environment Setup' will be run thrice on builder-tests bots. Hence, what I have done is not to wrap each script invocation with bash, but string together a single command and run just that command. The stringing is done in the main function below. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_run_bot.py:190: def CommandToString(command): On 2013/06/05 03:03:07, Isaac wrote: > move this function to bb_utils Done. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... File build/android/buildbot/bb_utils.py (right): https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_utils.py:23: GOMA_DIR = os.path.join(BB_BUILD_DIR, 'goma') On 2013/06/05 03:03:07, Isaac wrote: > remove this variable. Done. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_utils.py:53: code, flunk_on_failure, halt_on_failure = retcode_callback(code) On 2013/06/05 03:03:07, Isaac wrote: > I don't think we need this callback - can you pass a optional warning_code arg > (defaulting to 88) that specifies what code to not worry about instead? Done. https://chromiumcodereview.appspot.com/15261003/diff/89001/build/android/buil... build/android/buildbot/bb_utils.py:71: parser = BBOptionParser() On 2013/06/05 03:03:07, Isaac wrote: > Actually, I think it would be OK to use a regular OptionParser here. Looking at > our usage, I think sys.exit() could be OK since bb_device_tests is spawned as a > separate process. Done.
The solution to that is to remove the Environment setup buildbot step, not to change around the commands to run everything as a single unit. Running as a unit reduces the usefulness of being able to copy and paste individual test runs.
PTAL. The try jobs are in the queue. I have made all suggested changes.
On 2013/06/05 23:38:58, Siva Chandra wrote: > PTAL. The try jobs are in the queue. I have made all suggested changes. Note: I am getting a pylint error, "unable to import bb_utils" which I am not very sure how to fix.
You can fix pylint by adding the path to pylint call in build/android/PRESUBMIT.py
On 2013/06/06 21:00:20, Isaac wrote: > You can fix pylint by adding the path to pylint call in > build/android/PRESUBMIT.py Thanks. PTAL at patchset 28 which includes this fix.
https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:44: RunCmd(['gclient', 'runhooks'], flunk_on_failure=False) gclient runhooks should be called only once. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:69: warning_code=1) the warning code for this is 88, which is the default. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:72: def FindBugs(target): why does this take a target? Doesn't seem like it did that in bash. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:117: CheckWebViewLicenses() Let's make a "runhooks" step which calls gclient runhooks and put it here. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_run_bot.py (left): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:100: B('main-clang-builder', compile_step, slave_props={extra_gyp: 'clang=1'}), this property got dropped https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:120: {extra_gyp: 'component=shared_library'}), this property got dropped https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_run_bot.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:60: ' '.join(map(pipes.quote, property_args))), do we still need to pass factory_properties and build_properties? https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:64: host_opts = bot_config.host_opts remove variable. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_utils.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:26: print '>', ' '.join(map(pipes.quote, command)) use commandtostring() https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:42: print '<', ' '.join(map(pipes.quote, command)) use commandtostring() https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:50: if code != 0 and code != 88 and halt_on_failure: can you change this line to if code != warning_code and half_on_failure: https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:55: def ConvertJson(option, _, value, parser): nit, would be little cleaner to make this an inner function inside GetParser https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:74: def CommandToString(command): nit, move this to the top since it's used by functions below
Addressed/answered all your comments. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:44: RunCmd(['gclient', 'runhooks'], flunk_on_failure=False) On 2013/06/06 21:26:46, Isaac wrote: > gclient runhooks should be called only once. bash did this. Changed now with the new runhooks step. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:69: warning_code=1) On 2013/06/06 21:26:46, Isaac wrote: > the warning code for this is 88, which is the default. Is there any code which relies on the fact that default warning code is 88? If not, the latest change to RunCmd would make it generic isn't? https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:72: def FindBugs(target): On 2013/06/06 21:26:46, Isaac wrote: > why does this take a target? Doesn't seem like it did that in bash. the bash function depended on the env variable BUILDTYPE. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:117: CheckWebViewLicenses() On 2013/06/06 21:26:46, Isaac wrote: > Let's make a "runhooks" step which calls gclient runhooks and put it here. Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_run_bot.py (left): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:100: B('main-clang-builder', compile_step, slave_props={extra_gyp: 'clang=1'}), On 2013/06/06 21:26:46, Isaac wrote: > this property got dropped Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:120: {extra_gyp: 'component=shared_library'}), On 2013/06/06 21:26:46, Isaac wrote: > this property got dropped Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_run_bot.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:60: ' '.join(map(pipes.quote, property_args))), On 2013/06/06 21:26:46, Isaac wrote: > do we still need to pass factory_properties and build_properties? I dont think so. The code around my changes is changing with every rebase! Just passing slave_properties now. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_run_bot.py:64: host_opts = bot_config.host_opts On 2013/06/06 21:26:46, Isaac wrote: > remove variable. Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_utils.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:26: print '>', ' '.join(map(pipes.quote, command)) On 2013/06/06 21:26:46, Isaac wrote: > use commandtostring() Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:42: print '<', ' '.join(map(pipes.quote, command)) On 2013/06/06 21:26:46, Isaac wrote: > use commandtostring() Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:50: if code != 0 and code != 88 and halt_on_failure: On 2013/06/06 21:26:46, Isaac wrote: > can you change this line to > if code != warning_code and half_on_failure: Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:55: def ConvertJson(option, _, value, parser): On 2013/06/06 21:26:46, Isaac wrote: > nit, would be little cleaner to make this an inner function inside GetParser Done. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_utils.py:74: def CommandToString(command): On 2013/06/06 21:26:46, Isaac wrote: > nit, move this to the top since it's used by functions below Done.
lgtm with another round of changes. https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/107001/build/android/bui... build/android/buildbot/bb_host_steps.py:69: warning_code=1) On 2013/06/06 22:10:47, Siva Chandra wrote: > On 2013/06/06 21:26:46, Isaac wrote: > > the warning code for this is 88, which is the default. > > Is there any code which relies on the fact that default warning code is 88? If > not, the latest change to RunCmd would make it generic isn't? I don't know what you mean, but warning_code=1 here is incorrect. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:75: def FindBugs(target): We should avoid passing around a string here. Consider a namedtuple with 'Debug' and 'Release', or just a boolean is_release. Also buildtype would be a more correct terminology than target. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:79: RunCmd([SrcPath('build', 'android', 'findbugs_diff.py'), target]) we should not pass target arg if build type is debug. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:81: 'run_findbugs_plugin_tests.py'), nit linebreak https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:92: parser.add_option('--build-args', default='All', Is this option being used right now? If not, we should remove it and plumbing unless you think it will be used imminently. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:98: parser.add_option('--zipbuild', action='store_true', --zip-build https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:102: parser.add_option('--asan-tests-setup', action='store_true', --update-clang https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:116: target = options.factory_properties.get('target', 'Debug') could you rename this per commend above? Perhaps buildtype. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... File build/android/buildbot/bb_run_bot.py (right): https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_run_bot.py:60: CHROME_SRC, CommandToString(slave_properties)), don't convert to string here!!
There is an LGTM, but you do have few comments. I have now addressed them. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:75: def FindBugs(target): On 2013/06/06 23:32:02, Isaac wrote: > We should avoid passing around a string here. Consider a namedtuple with > 'Debug' and 'Release', or just a boolean is_release. Also buildtype would be a > more correct terminology than target. Done. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:79: RunCmd([SrcPath('build', 'android', 'findbugs_diff.py'), target]) On 2013/06/06 23:32:02, Isaac wrote: > we should not pass target arg if build type is debug. Done. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:81: 'run_findbugs_plugin_tests.py'), On 2013/06/06 23:32:02, Isaac wrote: > nit linebreak The line is now different. Where do you suggested I break it? https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:92: parser.add_option('--build-args', default='All', On 2013/06/06 23:32:02, Isaac wrote: > Is this option being used right now? If not, we should remove it and plumbing > unless you think it will be used imminently. We will require it downstream. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:98: parser.add_option('--zipbuild', action='store_true', On 2013/06/06 23:32:02, Isaac wrote: > --zip-build Done. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:102: parser.add_option('--asan-tests-setup', action='store_true', On 2013/06/06 23:32:02, Isaac wrote: > --update-clang Done. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:116: target = options.factory_properties.get('target', 'Debug') On 2013/06/06 23:32:02, Isaac wrote: > could you rename this per commend above? Perhaps buildtype. Done. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... File build/android/buildbot/bb_run_bot.py (right): https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_run_bot.py:60: CHROME_SRC, CommandToString(slave_properties)), On 2013/06/06 23:32:02, Isaac wrote: > don't convert to string here!! Done.
still lgtm, thanks. https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... File build/android/buildbot/bb_host_steps.py (right): https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:81: 'run_findbugs_plugin_tests.py'), On 2013/06/07 01:42:08, Siva Chandra wrote: > On 2013/06/06 23:32:02, Isaac wrote: > > nit linebreak > > The line is now different. Where do you suggested I break it? looks fine now. The comment was me thinking target could be moved to line 81 https://chromiumcodereview.appspot.com/15261003/diff/13005/build/android/buil... build/android/buildbot/bb_host_steps.py:92: parser.add_option('--build-args', default='All', On 2013/06/07 01:42:08, Siva Chandra wrote: > On 2013/06/06 23:32:02, Isaac wrote: > > Is this option being used right now? If not, we should remove it and plumbing > > unless you think it will be used imminently. > > We will require it downstream. OK.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/15261003/137008
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/15261003/151001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/15261003/151001
Message was sent while issue was closed.
Change committed as 204970 |
