Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128613005:
Archive Linux perf builds for manual bisect (Closed)
|
Created:
4 years, 5 months ago by miimnk Modified:
4 years, 4 months ago Reviewers:
dimu, ghost stip (do not use), dtu, RobertoCN, martiniss, sullivan, dimu1, prasadv1, stgao, Michael Moss CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org, sullivan Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionArchive Linux perf builds for manual bisect
changes to chromium perf buildbot to archive linux perf
builds without symbols and test files to chrome-test-builds
gs bucket
https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-PDo/edit
BUG=604452
R=dimu, stgao, dtu, prasadv
Committed: https://chromium.googlesource.com/chromium/tools/build/+/fd87b7ef932d03a3e462f1034e68502340146f5a
Patch Set 1 #Patch Set 2 : Archive Linux perf builds for manual bisect [2] #
Total comments: 10
Patch Set 3 : removed unzip_strip_zip, code style change #Patch Set 4 : removed unzip_strip_zip, code style change #Patch Set 5 : removed unzip_strip_zip, code style change #Patch Set 6 : removed unzip_strip_zip, code style change #
Total comments: 24
Patch Set 7 : Code style change #
Total comments: 6
Patch Set 8 : Archive Linux perf builds for manual bisect #Patch Set 9 : Archive Linux perf builds for manual bisect #Patch Set 10 : Archive Linux perf builds for manual bisect #Patch Set 11 : Archive Linux perf builds for manual bisect #Patch Set 12 : Archive Linux perf builds for manual bisect #
Total comments: 13
Patch Set 13 : Archive Linux perf builds for manual bisect #
Total comments: 28
Patch Set 14 : Archive Linux perf builds for manual bisect #Patch Set 15 : Archive Linux perf builds for manual bisect #Patch Set 16 : Archive Linux perf builds for manual bisect #Patch Set 17 : Archive Linux perf builds for bisect #Patch Set 18 : Archive Linux perf builds for manual bisect #Patch Set 19 : Merge remote-tracking branch 'refs/remotes/origin/master' into archive #Patch Set 20 : Merge remote-tracking branch 'refs/remotes/origin/master' into archive #
Total comments: 10
Patch Set 21 : Archive Linux perf builds for manual bisect #
Total comments: 1
Patch Set 22 : Merge remote-tracking branch 'refs/remotes/origin/master' into archive #Patch Set 23 : added a missing comma #Messages
Total messages: 105 (78 generated)
dimu@chromium.org changed reviewers: + dimu@chromium.org
https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/archive/api.py:96: exclude_files=None, includePerfTestFiles=True, update_properties = None, store_by_hash = True, **kwargs): make use of **kwargs, no need to explicitly declare these params as they are only perf builder-specific? nit: 1) 80 maximum letters 2) no space around '=' https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/archive/api.py:123: if not includePerfTestFiles: merge this block with the previous if block in line 119 https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/archive/api.py:127: # If update_properties is passed in and store_by_hash is False, we store it with commit position number instead of a hash nit: 80 words https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/api.py:410: # For archiving 'chromium.perf', the builder also archives a version without perf test files. nit: 80 words https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/api.py:423: update_properties = update_step.presentation.properties, nit: remove the space around '=' https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/api.py:785: def _build_gs_archive_url(self, mastername, master_config, buildername, no_symbol_save = False): rename to saveSymbol or stripSymbol https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/api.py:801: master_config.get('strip_build_gs_bucket'), rename the bucket key https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right): https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:17: 'strip_build_gs_bucket' : 'chrome-test-builds', Need a better name for the bucket: bisect_builds_gs_bucket https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/zip_build.py File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/zip_build... scripts/slave/zip_build.py:26: STRIP_LIST_LINUX = ['chrome', 'nacl_helper'] Add the list as an argument, and pass this as a argument from the caller of zip_build.py? https://codereview.chromium.org/2128613005/diff/20001/scripts/slave/zip_build... scripts/slave/zip_build.py:174: def unzip_strip_zip(zip_file, zip_file_list, strip_list): Is it possible to strip, then zip? To avoid the zip, unzip, strip, zip step?
https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... scripts/common/chromium_utils.py:676: os.system('strip %s' %(os.path.join(dest_dir, basename))) use RunCommand() https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... scripts/common/chromium_utils.py:680: os.system('strip %s' %(os.path.join(archive_dir, basename))) use RunCommand() https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:105: nit: remove empty line https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: FILES = [ Add some comments abt what the files for.
https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... scripts/common/chromium_utils.py:674: CopyFileToDir(src_path, dest_dir, basename, link_ok=True) As it is a link here, will we strip the original binary file? https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:122: inclusions = ",".join(perf_test_files.FILES) style nit: single quote instead of double quote. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: FILES = [ Maybe use a more self-explained name for what these files are for? https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:414: 'package build', This seems to add a new step, should we make a new name for it to avoid conflict with line #430 below? https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:420: # TODO(machenbach): Make asan a configuration switch. What's this for? https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... scripts/slave/zip_build.py:175: strip_files = ['chrome', 'nacl_helper'] Not sure where this file list should be, but this seems not the best place for it. It should be close to the logic which determine which files should be stripped. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... scripts/slave/zip_build.py:267: self.exclusive_include = options.exclusive_include What's this for? The naming seems a bit confusing to me.
https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... scripts/common/chromium_utils.py:674: CopyFileToDir(src_path, dest_dir, basename, link_ok=True) On 2016/07/11 19:04:27, stgao wrote: > As it is a link here, will we strip the original binary file? I checked and the original binary file in build directory is not stripped. https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... scripts/common/chromium_utils.py:676: os.system('strip %s' %(os.path.join(dest_dir, basename))) On 2016/07/11 17:47:09, dimu1 wrote: > use RunCommand() Done. https://codereview.chromium.org/2128613005/diff/100001/scripts/common/chromiu... scripts/common/chromium_utils.py:680: os.system('strip %s' %(os.path.join(archive_dir, basename))) On 2016/07/11 17:47:09, dimu1 wrote: > use RunCommand() Done. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:105: On 2016/07/11 17:47:09, dimu1 wrote: > nit: remove empty line Done. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:122: inclusions = ",".join(perf_test_files.FILES) On 2016/07/11 19:04:27, stgao wrote: > style nit: single quote instead of double quote. Done. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: FILES = [ On 2016/07/11 19:04:27, stgao wrote: > Maybe use a more self-explained name for what these files are for? Would CHROME_REQUIRED_FILES be ok? https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: FILES = [ On 2016/07/11 17:47:09, dimu1 wrote: > Add some comments abt what the files for. Done. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:414: 'package build', On 2016/07/11 19:04:27, stgao wrote: > This seems to add a new step, should we make a new name for it to avoid conflict > with line #430 below? How about if we name it to 'package build bisect'. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:420: # TODO(machenbach): Make asan a configuration switch. On 2016/07/11 19:04:27, stgao wrote: > What's this for? I essentially copied and pasted the self.m.archive.zip_and_upload_build from the below and changed gs archive url and additionally included a few required parameters. It seems that 'package_dsym_files' is only related to 'mac', so I will remove this since we are currently only archiving for Linux. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... scripts/slave/zip_build.py:175: strip_files = ['chrome', 'nacl_helper'] On 2016/07/11 19:04:27, stgao wrote: > Not sure where this file list should be, but this seems not the best place for > it. It should be close to the logic which determine which files should be > stripped. There are only two binary files with a significant size ('chrome', 'nacl_helper'). Should I still implement a logic to strip all binary files above a certain file size? https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... scripts/slave/zip_build.py:267: self.exclusive_include = options.exclusive_include On 2016/07/11 19:04:27, stgao wrote: > What's this for? The naming seems a bit confusing to me. Currently, it is not possible to only include items in the 'include' list. It goes through a pregenerated regex_whitelist to match other files too. So, I added a logic to skip the regex_whitelist part. Other names I can think of are 'exclude_unincluded', and 'ignore_regex'. This is different from 'exclude_unmatched' because exclude_unmatched still has to go through including files in whitelist.
It seems no test is added yet. https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... scripts/slave/zip_build.py:175: strip_files = ['chrome', 'nacl_helper'] On 2016/07/11 22:08:48, miimnk wrote: > On 2016/07/11 19:04:27, stgao wrote: > > Not sure where this file list should be, but this seems not the best place for > > it. It should be close to the logic which determine which files should be > > stripped. > > There are only two binary files with a significant size ('chrome', > 'nacl_helper'). Should I still implement a logic to strip all binary files above > a certain file size? Stripping big files are good for now. My comment is more of that the file list should not be added here, because this script is only for archiving. It should not determine which files to be stripped with a passed in True/False flag. Is there any alternative location that the file list could be in? Maybe archive/perf_test_files.py above? https://codereview.chromium.org/2128613005/diff/120001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/120001/scripts/common/chromiu... scripts/common/chromium_utils.py:676: cmd = ['strip', os.path.join(dest_dir, basename)] Will the strip also run on Mac and Windows? If so, is that expected? https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: # The below is the list of files from Chrome perf Linux builds nit: missing a file header. Comments are usually in a docstring. https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:414: 'package build bisect', "for bisect" instead?
The CQ bit was checked by miimnk@google.com
The CQ bit was unchecked by miimnk@google.com
The CQ bit was checked by miimnk@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by miimnk@google.com
I added a test json file generated from recipe_simulation_test.py https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/100001/scripts/slave/zip_buil... scripts/slave/zip_build.py:175: strip_files = ['chrome', 'nacl_helper'] On 2016/07/12 20:40:40, stgao wrote: > On 2016/07/11 22:08:48, miimnk wrote: > > On 2016/07/11 19:04:27, stgao wrote: > > > Not sure where this file list should be, but this seems not the best place > for > > > it. It should be close to the logic which determine which files should be > > > stripped. > > > > There are only two binary files with a significant size ('chrome', > > 'nacl_helper'). Should I still implement a logic to strip all binary files > above > > a certain file size? > > Stripping big files are good for now. > > My comment is more of that the file list should not be added here, because this > script is only for archiving. It should not determine which files to be stripped > with a passed in True/False flag. > > Is there any alternative location that the file list could be in? > > Maybe archive/perf_test_files.py above? I stored the strip list in archive/perf_test_files.py and passed to zip_build.py as a comma separated string of the files that needs to be stripped. Now, it passes in the file list instead of a True/False flag. https://codereview.chromium.org/2128613005/diff/120001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/120001/scripts/common/chromiu... scripts/common/chromium_utils.py:676: cmd = ['strip', os.path.join(dest_dir, basename)] On 2016/07/12 20:40:40, stgao wrote: > Will the strip also run on Mac and Windows? If so, is that expected? No. Strip will not run on Mac and Windows because strip flag will only be passed for Linux Builder. https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: # The below is the list of files from Chrome perf Linux builds On 2016/07/12 20:40:40, stgao wrote: > nit: missing a file header. > > Comments are usually in a docstring. Done. https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/120001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:414: 'package build bisect', On 2016/07/12 20:40:40, stgao wrote: > "for bisect" instead? Done.
sullivan@chromium.org changed reviewers: + sullivan@chromium.org
Sorry for the drive-by, but it looks like this didn't get a full design review before going into code review. It looks like the design doc is here? https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-P... Please link design docs in bugs and code review descriptions. Design docs should be on chromium.org and chromium-commentable when possible, and on google.com when they contain potentially sensitive information. Please send to chrome-speed-infra@google.com for review. I left some comments on the design doc.
The CQ bit was checked by miimnk@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
robertocn@chromium.org changed reviewers: + robertocn@chromium.org
https://codereview.chromium.org/2128613005/diff/220001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/common/chromiu... scripts/common/chromium_utils.py:603: raise_error=True, remove_archive_directory=True, strip_files = None): nit: Doesn't it make more sense to default to an empty list? https://codereview.chromium.org/2128613005/diff/220001/scripts/common/chromiu... scripts/common/chromium_utils.py:676: cmd = ['strip', os.path.join(dest_dir, basename)] 'strip' is a gnu command, right? what happens if this is called from windows? https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:411: # without perf test files. Maybe link to the bug for more context? https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:17: 'bisect_build_gs_bucket' : 'chrome-test-builds/official-by-commit', it's not immediately clear the relationship between bisect_builds and chrome-test-builds. Maybe comment to make it clear this is for manual bisect? Also, this looks more like a partial path than a bucket. https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/zip_buil... scripts/slave/zip_build.py:361: unversioned_base_name, strip_files=options.strip_files) Is this line over 80 chars, or is it the codereview site wrapping it incorrectly? https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/zip_buil... scripts/slave/zip_build.py:443: 'be stripped in the zip.') stripped of symbols
Hi Roberto! Thank you so much for the review. I fixed the issues you mentioned. Please let me know if any more things should be changed. https://codereview.chromium.org/2128613005/diff/220001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/common/chromiu... scripts/common/chromium_utils.py:603: raise_error=True, remove_archive_directory=True, strip_files = None): On 2016/08/03 20:49:49, RobertoCN wrote: > nit: Doesn't it make more sense to default to an empty list? Changed https://codereview.chromium.org/2128613005/diff/220001/scripts/common/chromiu... scripts/common/chromium_utils.py:676: cmd = ['strip', os.path.join(dest_dir, basename)] On 2016/08/03 20:49:49, RobertoCN wrote: > 'strip' is a gnu command, right? what happens if this is called from windows? Yes. It seems that it would give error for windows. I changed it so the command won't run for windows. Updated the comments above. https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/08/03 20:49:49, RobertoCN wrote: > nit: 2016 Fixed https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:411: # without perf test files. On 2016/08/03 20:49:49, RobertoCN wrote: > Maybe link to the bug for more context? Fixed https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:17: 'bisect_build_gs_bucket' : 'chrome-test-builds/official-by-commit', On 2016/08/03 20:49:49, RobertoCN wrote: > it's not immediately clear the relationship between bisect_builds and > chrome-test-builds. Maybe comment to make it clear this is for manual bisect? > Also, this looks more like a partial path than a bucket. Commented that it is for manual bisect. Added a new attribute 'bisect_build_gs_extra' for the second part https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/220001/scripts/slave/zip_buil... scripts/slave/zip_build.py:361: unversioned_base_name, strip_files=options.strip_files) On 2016/08/03 20:49:49, RobertoCN wrote: > Is this line over 80 chars, or is it the codereview site wrapping it > incorrectly? Fixed
https://codereview.chromium.org/2128613005/diff/240001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/common/chromiu... scripts/common/chromium_utils.py:603: raise_error=True, remove_archive_directory=True, strip_files = []): There's a tricky Python caveat with lists (or any mutable object) as default arguments: https://google.github.io/styleguide/pyguide.html?showone=Default_Argument_Val... Instead, do: def MakeZip(..., strip_files=None): if not strip_files: strip_files = [] https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:120: if not kwargs.pop("includePerfTestFiles", True): style nit: argument/variable names are always lowercase_with_underscores. style nit: I think we generally use single quotes for string literals It looks like kwargs is being used in this function for arguments to be forwarded onto self.m.python(). Since you're using include_perf_test_files in this function, you should declare it in the function definition. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. Name could be misleading -- these files aren't used for perf tests. Maybe manual_bisect_files? https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:9: CHROME_REQUIRED_FILES = [ nit: Alphabetize this list. Also, is there any way to get the ground truth as to which files are required? This seems like it may break at unexpected times, and then the perf sheriff will be responsible. I notice there's a chrome.isolate in chromium src -- maybe that could be useful? Also, can you comment with where the list came from / how someone can reproduce the list? https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:414: buildername == 'Linux Builder'): This condition could be brittle -- prefer not to do things based on the name of the builder. Maybe add an option to the master_config that enables this behavior? https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:419: mastername, master_config, buildername, strip_symbol = True), style nit: no space around = style nit: this line is very long. it may be more readable to assign the result of self._build_gs_archive_url) to a variable beforehand. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:784: strip_symbol = False): style nit: no space around = https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:798: if strip_symbol: This function has completely separate code paths when this argument is True vs when it's False. Maybe it doesn't belong in this function at all? On the other hand, maybe this function provides an abstraction layer that shouldn't be violated? Up to you. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:17: # Bucket for stroring builds for manula bisect spelling nit: storing, manual https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:18: 'bisect_build_gs_bucket' : 'chrome-test-builds', nit: no space before : https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:174: strip_files = [f.strip() for f in csv.reader([strip_files]).next()] might be more readable to do strip_files.split(',') instead of csv.reader([strip_files]).next() ? Up to you https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:276: 'Ignore regex matches: %s' % (self.ignore_regex)]) style nit: unnecessary parentheses https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:409: option_parser.add_option('--ignore_regex', action='store_true', Command-line args should be separated by - instead of _ https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:444: 'be stripped of symbols in the zip.') I'd prefer if you convert the argument to a list in this function, rather than in MakeUnversionedArchive. It's conceptually part of the argument parsing.
Hi Dave. Thank you so much for reviewing the code. I fixed the issues that you pointed out. Please let me know if any other things need to be fixed. Best, Min https://codereview.chromium.org/2128613005/diff/240001/scripts/common/chromiu... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/common/chromiu... scripts/common/chromium_utils.py:603: raise_error=True, remove_archive_directory=True, strip_files = []): On 2016/08/05 21:06:18, dtu wrote: > There's a tricky Python caveat with lists (or any mutable object) as default > arguments: > https://google.github.io/styleguide/pyguide.html?showone=Default_Argument_Val... > > Instead, do: > > def MakeZip(..., strip_files=None): > if not strip_files: > strip_files = [] Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:120: if not kwargs.pop("includePerfTestFiles", True): On 2016/08/05 21:06:18, dtu wrote: > style nit: argument/variable names are always lowercase_with_underscores. > > style nit: I think we generally use single quotes for string literals > > It looks like kwargs is being used in this function for arguments to be > forwarded onto self.m.python(). Since you're using include_perf_test_files in > this function, you should declare it in the function definition. Fixed the variable name with lowercase_with_underscores. Passed include_perf_test_files, update_properties, store_by_hash as parameters instead of passing through **kwargs. Did you mean to only pass include_perf_test_files as a parameter? https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/perf_test_files.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/08/05 21:06:18, dtu wrote: > Name could be misleading -- these files aren't used for perf tests. Maybe > manual_bisect_files? Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/perf_test_files.py:9: CHROME_REQUIRED_FILES = [ On 2016/08/05 21:06:19, dtu wrote: > nit: Alphabetize this list. > > Also, is there any way to get the ground truth as to which files are required? > This seems like it may break at unexpected times, and then the perf sheriff will > be responsible. I notice there's a chrome.isolate in chromium src -- maybe that > could be useful? > > Also, can you comment with where the list came from / how someone can reproduce > the list? i) Alphabetized the list. ii) Yes, it is possible that the archive might not be able to execute Chrome unexpectedly in the future if archive files change. But the file list seems to be able to work for all the previous ~80000 builds. Also, I passed the file list to chromium_utils.MakeZip with raise_error parameter as False. Thus, even if some files get missing later, it will not break the buildbot. The file list was gotten from the local chrome executable path in Linux. (This can be retrieved by typing 'chrome://version' in chrome and following the executable path. I added this into the comments. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:414: buildername == 'Linux Builder'): On 2016/08/05 21:06:19, dtu wrote: > This condition could be brittle -- prefer not to do things based on the name of > the builder. Maybe add an option to the master_config that enables this > behavior? Done. Added an option in master_config. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:419: mastername, master_config, buildername, strip_symbol = True), On 2016/08/05 21:06:19, dtu wrote: > style nit: no space around = > > style nit: this line is very long. it may be more readable to assign the result > of self._build_gs_archive_url) to a variable beforehand. Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:784: strip_symbol = False): On 2016/08/05 21:06:19, dtu wrote: > style nit: no space around = Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:798: if strip_symbol: On 2016/08/05 21:06:19, dtu wrote: > This function has completely separate code paths when this argument is True vs > when it's False. Maybe it doesn't belong in this function at all? On the other > hand, maybe this function provides an abstraction layer that shouldn't be > violated? Up to you. I see that the part of the code is indeed unrelated to the function. I made a new function with that code. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:17: # Bucket for stroring builds for manula bisect On 2016/08/05 21:06:19, dtu wrote: > spelling nit: storing, manual Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:18: 'bisect_build_gs_bucket' : 'chrome-test-builds', On 2016/08/05 21:06:19, dtu wrote: > nit: no space before : Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... File scripts/slave/zip_build.py (right): https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:174: strip_files = [f.strip() for f in csv.reader([strip_files]).next()] On 2016/08/05 21:06:19, dtu wrote: > might be more readable to do strip_files.split(',') instead of > csv.reader([strip_files]).next() > ? Up to you Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:276: 'Ignore regex matches: %s' % (self.ignore_regex)]) On 2016/08/05 21:06:19, dtu wrote: > style nit: unnecessary parentheses Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:409: option_parser.add_option('--ignore_regex', action='store_true', On 2016/08/05 21:06:19, dtu wrote: > Command-line args should be separated by - instead of _ Done. https://codereview.chromium.org/2128613005/diff/240001/scripts/slave/zip_buil... scripts/slave/zip_build.py:444: 'be stripped of symbols in the zip.') On 2016/08/05 21:06:19, dtu wrote: > I'd prefer if you convert the argument to a list in this function, rather than > in MakeUnversionedArchive. It's conceptually part of the argument parsing. Done.
Description was changed from ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket BUG=604452 R=dimu, stgao, dtu, ananthak ========== to ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket BUG=604452 R=dimu, stgao, dtu, prasadv ==========
miimnk@google.com changed reviewers: + prasadv@google.com - ananthak@google.com
The CQ bit was checked by dtu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/308f5776001ab110)
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dimu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dimu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dimu@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-infra-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dtu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/309486ae171c3e10)
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket BUG=604452 R=dimu, stgao, dtu, prasadv ========== to ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-P... BUG=604452 R=dimu, stgao, dtu, prasadv ==========
Description was changed from ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-P... BUG=604452 R=dimu, stgao, dtu, prasadv ========== to ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-P... BUG=604452 R=dimu, stgao, dtu, prasadv ==========
mmoss@chromium.org changed reviewers: + mmoss@chromium.org
lgtm
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by miimnk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dtu@chromium.org, mmoss@chromium.org Link to the patchset: https://codereview.chromium.org/2128613005/#ps380001 (title: "Merge remote-tracking branch 'refs/remotes/origin/master' into archive")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3095de062d5cb110)
The CQ bit was unchecked by miimnk@google.com
The CQ bit was checked by miimnk@google.com
The CQ bit was unchecked by miimnk@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by miimnk@google.com
stip@chromium.org changed reviewers: + martiniss@chromium.org, stip@chromium.org
Sorry for delaying this patch further :/. I have some concerns with how the configuration is modified deep down in the code, it would really be best to move this to a top-level config that perf builders can turn on and off as needed. I've added martiniss@ who can advise on the best way to do this. https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:8: from . import manual_bisect_files is that needed? can you not just do import manual_bisect_files? https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:95: exclude_files=None, include_perf_test_files = True, nit: no spaces around = https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:95: exclude_files=None, include_perf_test_files = True, second nit: I'd prefer to flip this flag: "exclude_perf_test_files". That better shows that the normal operation is to include everything, and only in certain circumstances do we exclude. https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:125: if not include_perf_test_files: if exclude_perf_test_files: ... https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/manual_bisect_files.py (right): https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/manual_bisect_files.py:5: """Declares required files to run manual bisect script on chrome Linux nit: """One sentence docstring ending with a period. A blank line following a larger, multi-line description. After the description, the closing three quotes are on a line of their own. """ https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:437: if (mastername.startswith('chromium.perf') and I'm not comfortable with having an if statement gated on the mastername this deep in the code. We should set a flag for archive_build() enabling this behavior (perhaps something like "enable_manual_bisect"), and turn on this behavior in the recipe itself. I believe this is done with a recipe config. martiniss@ can help out here for the right way to do it.
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you so much for your review Mike. I fixed the issues you mentioned in your review. https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:8: from . import manual_bisect_files On 2016/08/15 20:12:15, stip wrote: > is that needed? can you not just do import manual_bisect_files? Done. https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:95: exclude_files=None, include_perf_test_files = True, On 2016/08/15 20:12:15, stip wrote: > nit: no spaces around = Done. https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/manual_bisect_files.py (right): https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/manual_bisect_files.py:5: """Declares required files to run manual bisect script on chrome Linux On 2016/08/15 20:12:16, stip wrote: > nit: > > """One sentence docstring ending with a period. > > A blank line following a larger, multi-line description. > After the description, the closing three quotes are on > a line of their own. > """ Done. https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_tests/api.py (right): https://codereview.chromium.org/2128613005/diff/380001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_tests/api.py:437: if (mastername.startswith('chromium.perf') and On 2016/08/15 20:12:16, stip wrote: > I'm not comfortable with having an if statement gated on the mastername this > deep in the code. We should set a flag for archive_build() enabling this > behavior (perhaps something like "enable_manual_bisect"), and turn on this > behavior in the recipe itself. I believe this is done with a recipe config. > martiniss@ can help out here for the right way to do it. If you see recipe_modules/chromium_tests/chromium_perf.py, it contains the SPEC for running the perf recipes. I previously added a 'bisect_builders' attribute to the SPEC. We don't need to check the master name because we only need to check if the SPEC for the master contains 'bisect_builders'. I removed the "(mastername.startswith('chromium.perf'))" condition as it works without it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
great! lgtm w/ nit https://codereview.chromium.org/2128613005/diff/400001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/archive/api.py (right): https://codereview.chromium.org/2128613005/diff/400001/scripts/slave/recipe_m... scripts/slave/recipe_modules/archive/api.py:8: import manual_bisect_files nit: sort line 8 before line 7
The CQ bit was checked by miimnk@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by miimnk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dtu@chromium.org, mmoss@chromium.org, stip@chromium.org Link to the patchset: https://codereview.chromium.org/2128613005/#ps420001 (title: "Merge remote-tracking branch 'refs/remotes/origin/master' into archive")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-P... BUG=604452 R=dimu, stgao, dtu, prasadv ========== to ========== Archive Linux perf builds for manual bisect changes to chromium perf buildbot to archive linux perf builds without symbols and test files to chrome-test-builds gs bucket https://docs.google.com/document/d/1yzpQ7JCC7HzZ2mG5f3Z4ozKPXqrihsEiCpoFW7j-P... BUG=604452 R=dimu, stgao, dtu, prasadv Committed: https://chromium.googlesource.com/chromium/tools/build/+/fd87b7ef932d03a3e462... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001) as https://chromium.googlesource.com/chromium/tools/build/+/fd87b7ef932d03a3e462... |
