Title: [271211] trunk/Tools
Revision
271211
Author
[email protected]
Date
2021-01-06 12:31:23 -0800 (Wed, 06 Jan 2021)

Log Message

Assorted fixes for bisect-builds
https://bugs.webkit.org/show_bug.cgi?id=220158

Reviewed by Jonathan Bedard.

Switched to Python 3.
Many trivial Python style changes.
Simplified arguments, and cleaned up output.
Switched from run-safari to run-minibrowser on macOS, as run-safari currently
doesn't work on regular macOS installations.

* Scripts/bisect-builds:
(QueueDescriptor): New class that holds a description of a queue, or a "platform",
as coming from a string like mac-catalina or mac-catalina-x86_64-release. Used for
matching arguments to an existing archive directory on the server.
(trac_link): Added. The tool now prints a trac link for seeing where the regression
was introduced.
(bisect_builds): Fixed arithmetic mistakes. Got rid of an unnecessary while-true loop,
as the function was already recursive.
(host_platform_name): The script now defaults to current platform, so -p argument
is mostly to select simulator.
(main): Moved code from "if __name__ == '__main__'" block into main function for
consistency.

Modified Paths

Diff

Modified: trunk/Tools/ChangeLog (271210 => 271211)


--- trunk/Tools/ChangeLog	2021-01-06 20:10:42 UTC (rev 271210)
+++ trunk/Tools/ChangeLog	2021-01-06 20:31:23 UTC (rev 271211)
@@ -1,3 +1,29 @@
+2021-01-06  Alexey Proskuryakov  <[email protected]>
+
+        Assorted fixes for bisect-builds
+        https://bugs.webkit.org/show_bug.cgi?id=220158
+
+        Reviewed by Jonathan Bedard.
+
+        Switched to Python 3.
+        Many trivial Python style changes.
+        Simplified arguments, and cleaned up output.
+        Switched from run-safari to run-minibrowser on macOS, as run-safari currently
+        doesn't work on regular macOS installations.
+
+        * Scripts/bisect-builds: 
+        (QueueDescriptor): New class that holds a description of a queue, or a "platform",
+        as coming from a string like mac-catalina or mac-catalina-x86_64-release. Used for
+        matching arguments to an existing archive directory on the server.
+        (trac_link): Added. The tool now prints a trac link for seeing where the regression
+        was introduced.
+        (bisect_builds): Fixed arithmetic mistakes. Got rid of an unnecessary while-true loop,
+        as the function was already recursive.
+        (host_platform_name): The script now defaults to current platform, so -p argument
+        is mostly to select simulator.
+        (main): Moved code from "if __name__ == '__main__'" block into main function for
+        consistency.
+
 2021-01-06  Alex Christensen  <[email protected]>
 
         Add SPI to determine whether a regex is supported in WKContentRuleList

Modified: trunk/Tools/Scripts/bisect-builds (271210 => 271211)


--- trunk/Tools/Scripts/bisect-builds	2021-01-06 20:10:42 UTC (rev 271210)
+++ trunk/Tools/Scripts/bisect-builds	2021-01-06 20:31:23 UTC (rev 271211)
@@ -1,6 +1,6 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 
-# Copyright (C) 2017 Apple Inc. All rights reserved.
+# Copyright (C) 2017, 2020 Apple Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions
@@ -26,6 +26,12 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
 # THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+# A webkitpy import needs to go first for autoinstaller to work with subsequent imports.
+from webkitpy.common.memoized import memoized
+from webkitpy.common.system.systemhost import SystemHost
+from webkitpy.common.version_name_map import VersionNameMap
+from webkitpy.tool.grammar import pluralize
+
 import argparse
 import bisect
 import json
@@ -35,10 +41,9 @@
 import subprocess
 import sys
 import tempfile
-import urllib2
-import urlparse
-from webkitpy.common.memoized import memoized
+import urllib
 
+
 REST_API_URL = 'https://q1tzqfy48e.execute-api.us-west-2.amazonaws.com/v2_2/'
 REST_API_ARCHIVE_ENDPOINT = 'archives/'
 REST_API_MINIFIED_ARCHIVE_ENDPOINT = 'minified-archives/'
@@ -46,35 +51,94 @@
 REST_API_MINIFIED_PLATFORM_ENDPOINT = 'minified-platforms'
 
 
+class QueueDescriptor(object):
+    def __init__(self, descriptor_string):
+        self.platform_name = None
+        self.version = None
+        self.architectures = set()
+        self.configuration = None
+
+        if descriptor_string.startswith('mac-'):
+            platform_name_end_index = descriptor_string.find('-')
+            version_start_index = platform_name_end_index + 1
+            version_end_index = descriptor_string.find('-', version_start_index)
+            self.platform_name = descriptor_string[:platform_name_end_index]
+            if version_end_index == -1:
+                self.version = descriptor_string[version_start_index:]
+                return
+            self.version = descriptor_string[version_start_index:version_end_index]
+            architectures_and_configuration = descriptor_string[version_end_index + 1:]
+        elif descriptor_string.startswith('ios-simulator-'):
+            platform_name_end_index = descriptor_string.find('-', descriptor_string.find('-') + 1)
+            version_start_index = platform_name_end_index + 1
+            version_end_index = descriptor_string.find('-', version_start_index)
+            self.platform_name = descriptor_string[:platform_name_end_index]
+            if version_end_index == -1:
+                self.version = descriptor_string[version_start_index:]
+                return
+            self.version = descriptor_string[version_start_index:version_end_index]
+            architectures_and_configuration = descriptor_string[version_end_index + 1:]
+        else:
+            platform_name_end_index = descriptor_string.find('-')
+            if platform_name_end_index == -1:
+                self.platform_name = descriptor_string
+                return
+            self.platform_name = descriptor_string[:platform_name_end_index]
+            architectures_and_configuration = descriptor_string[platform_name_end_index + 1:]
+        architectures_end_index = architectures_and_configuration.find('-')
+        if architectures_end_index == -1:
+            self.architectures = set(architectures_and_configuration.split(' '))
+            return
+        configuration_start_index = architectures_end_index + 1
+        self.architectures = set(architectures_and_configuration[:architectures_end_index].split(' '))
+        self.configuration = architectures_and_configuration[configuration_start_index:]
+
+    def pretty_string(self):
+        result = self.platform_name
+        if self.version:
+            result += '-' + self.version
+        result += ' (' + ' '.join(self.architectures)
+        result += ', ' + self.configuration + ')'
+        return result
+
+
+def trac_link(start_revision, end_revision):
+    if start_revision + 1 == end_revision:
+        return 'https://trac.webkit.org/r{}'.format(end_revision)
+    else:
+        return 'https://trac.webkit.org/log/trunk/?mode=follow_copy&rev={}&stop_rev={}'.format(end_revision, start_revision + 1)
+
+
 def bisect_builds(revision_list, start_index, end_index, options):
-    while True:
-        index_to_test = pick_next_build(revision_list, start_index, end_index)
-        if index_to_test == None:
-            print('No more builds to test...')
-            exit(1)
-        download_archive(options, revision_list[index_to_test])
-        extract_archive(options)
-        reproduces = test_archive(options, revision_list[index_to_test])
+    index_to_test = pick_next_build(revision_list, start_index, end_index)
+    if index_to_test is None:
+        print('\nWorks: r{}'.format(revision_list[start_index]))
+        print('Fails: r{}'.format(revision_list[end_index]))
+        print(trac_link(revision_list[start_index], revision_list[end_index]))
+        exit(0)
 
-        if reproduces:          # bisect left
-            index_to_test -= 1  # We can remove this from the end of the list of builds to test
-            bisect_builds(revision_list, start_index, index_to_test, options)
-        if not reproduces:      # bisect right
-            index_to_test += 1  # We can remove this from the start of the list of builds to test
-            bisect_builds(revision_list, index_to_test, end_index, options)
+    archive_count = end_index - start_index - 1
+    print('Bisecting between r{} and r{}, {} in the range.'.format(revision_list[start_index], revision_list[end_index], pluralize(archive_count, 'archive')))
+    reproduces = test_revision(options, revision_list[index_to_test])
 
+    if reproduces:
+        bisect_builds(revision_list, start_index, index_to_test, options)
+    else:
+        bisect_builds(revision_list, index_to_test, end_index, options)
 
+
+# download-built-product and built-product-archive implicitly use WebKitBuild directory for downloaded archive.
+# FIXME: Modifying the WebKitBuild directory makes no sense here, find a way to use a temporary directory for the archive.
 def download_archive(options, revision):
     api_url = get_api_archive_url(options)
     s3_url = get_s3_location_for_revision(api_url, revision)
-    print('Archive URL: {}'.format(s3_url))
+    print('Downloading r{}: {}'.format(revision, s3_url))
     command = ['python', '../CISupport/download-built-product', '--{}'.format(options.configuration), '--platform', options.platform, s3_url]
-    print('Downloading revision: {}'.format(revision))
     subprocess.check_call(command)
 
 
 def extract_archive(options):
-    command = ['python', '../CISupport/built-product-archive', '--platform', options.platform, '--%s' % options.configuration, 'extract']
+    command = ['python', '../CISupport/built-product-archive', '--{}'.format(options.configuration), '--platform', options.platform, 'extract']
     subprocess.check_call(command)
 
 
@@ -96,29 +160,29 @@
 # ---- end bisect helpers ----
 
 
-def get_api_archive_url(options, LastEvaluatedKey=None):
+def get_api_archive_url(options, last_evaluated_key=None):
     if options.full:
-        base_url = urlparse.urljoin(REST_API_URL, REST_API_ARCHIVE_ENDPOINT)
+        base_url = urllib.parse.urljoin(REST_API_URL, REST_API_ARCHIVE_ENDPOINT)
     else:
-        base_url = urlparse.urljoin(REST_API_URL, REST_API_MINIFIED_ARCHIVE_ENDPOINT)
+        base_url = urllib.parse.urljoin(REST_API_URL, REST_API_MINIFIED_ARCHIVE_ENDPOINT)
 
-    api_url = urlparse.urljoin(base_url, '-'.join([options.platform, options.architecture, options.configuration]))
-    if LastEvaluatedKey:
-        querystring = urllib2.quote(json.dumps(LastEvaluatedKey))
+    api_url = urllib.parse.urljoin(base_url, urllib.parse.quote(options.queue))
+    if last_evaluated_key:
+        querystring = urllib.parse.quote(json.dumps(last_evaluated_key))
         api_url += '?ExclusiveStartKey=' + querystring
-        
+
     return api_url
 
 
 def get_indices_from_revisions(revision_list, start_revision, end_revision):
     if start_revision is None:
-        print('WARNING: No starting revision was given, defaulting to first available for this configuration')
+        print('WARNING: No starting revision was given, defaulting to first available for this configuration.')
         start_index = 0
     else:
         start_index = find_ge(revision_list, start_revision)
 
     if end_revision is None:
-        print('WARNING: No ending revision was given, defaulting to last avialable for this configuration')
+        print('WARNING: No ending revision was given, defaulting to last available for this configuration.')
         end_index = len(revision_list) - 1
     else:
         end_index = find_le(revision_list, end_revision)
@@ -129,45 +193,51 @@
 def get_sorted_revisions(revisions_dict):
     revisions = [int(item['revision']['N']) for item in revisions_dict['revisions']['Items']]
     return sorted(revisions)
-    
 
+
 def get_s3_location_for_revision(url, revision):
     url = ''.join([url, str(revision)])
-    r = urllib2.urlopen(url)
+    r = urllib.request.urlopen(url)
     data = ""
-    
+
     for archive in data['archive']:
         s3_url = archive['s3_url']
     return s3_url
 
 
+def host_platform_name():
+    platform = SystemHost().platform
+    version_name = VersionNameMap.strip_name_formatting(platform.os_version_name())
+    if version_name is None:
+        return platform.os_name
+    return platform.os_name + '-' + version_name
+
+
 def parse_args(args):
-    helptext = 'bisect-builds is designed to help pinpoint regressions to specific code changes. It does this by bisecting across archives produced by build.webkit.org. Full and "minified" archives are available. Minified archives are significantly smaller, as they have been stripped of dSYMs and other non-essential components.'
+    helptext = 'bisect-builds helps pinpoint regressions to specific code changes. It does this by bisecting across archives produced by build.webkit.org. Full and "minified" archives are available. Minified archives are significantly smaller, as they have been stripped of dSYMs and other non-essential components.'
     parser = argparse.ArgumentParser(description=helptext)
-    parser.add_argument('-c', '--configuration', default='release', help='The configuration to query [release | debug]')
-    parser.add_argument('-a', '--architecture', default='x86_64', help='The architecture to query [x86_64 | i386]')
-    parser.add_argument('-p', '--platform', default='None', required=True, help='The platform to query [mac-sierra | gtk | ios-simulator | win]')
-    parser.add_argument('-f', '--full', action='', default=False, help='Use full archives containing debug symbols. These are significantly larger files!')
-    parser.add_argument('-s', '--start', default=None, type=int, help='The starting revision to bisect.')
-    parser.add_argument('-e', '--end', default=None, type=int, help='The ending revision to bisect')
-    parser.add_argument('-l', '--list', action='', default=False, help='Display a list of platforms and revisions')
+    parser.add_argument('-c', '--configuration', default='release', help='the configuration to query [release | debug]')
+    parser.add_argument('-a', '--architecture', help='the architecture to query, e.g. x86_64, default is no preference')
+    parser.add_argument('-p', '--platform', default=host_platform_name(), help='the platform to query, e.g. mac-bigsur, gtk, ios-simulator-14, win, default is current host platform.')
+    parser.add_argument('-f', '--full', action='', default=False, help='use full archives containing debug symbols, which are significantly larger files')
+    parser.add_argument('-s', '--start', default=None, type=int, help='the starting revision to bisect')
+    parser.add_argument('-e', '--end', default=None, type=int, help='the ending revision to bisect')
+    parser.add_argument('--sanity-check', action='', default=False, help='verify both starting and ending revisions before bisecting')
+    parser.add_argument('-l', '--list', action='', default=False, help='display a list of platforms and revisions')
     return parser.parse_args(args)
 
 
 def pick_next_build(revision_list, start_index, end_index):
-    revisions_remaining = (end_index - start_index) + 1
-    print('Found {} revisions in this range to test...'.format(revisions_remaining))
-
-    if start_index >= end_index:
-        print('No archives available between {} and {}'.format(revision_list[end_index], revision_list[start_index]))
+    if start_index + 1 >= end_index:
+        print('No archives available between r{} and r{}.'.format(revision_list[start_index], revision_list[end_index]))
         return None
 
-    middleIndex = (start_index + end_index) / 2
-    return int(math.ceil(middleIndex))
+    middle_index = (start_index + end_index) / 2
+    return int(math.ceil(middle_index))
 
 
 def prompt_did_reproduce():
-    var = raw_input('\nDid the error reproduce? [y/n]: ')
+    var = input('\nDid the error reproduce? [y/n]: ')
     var = var.lower()
     if 'y' in var:
         return True
@@ -175,106 +245,158 @@
         return False
     else:
         prompt_did_reproduce()
-    
 
+
 def set_webkit_output_dir(temp_dir):
-    print('Setting environment variable WEBKIT_OUTPUTDIR to {}'.format(temp_dir))
+    print('Archives will be extracted to {}'.format(temp_dir))
     os.environ['WEBKIT_OUTPUTDIR'] = temp_dir
 
 
-def test_archive(options, revision):
-    print('Testing revision {}...'.format(revision))
-    command = []
-    if 'mac' in options.platform:
-        command = ['./run-safari']
-    elif 'ios' in options.platform:
-        command = ['./run-safari', '--simulator']
+def test_revision(options, revision):
+    download_archive(options, revision)
+    extract_archive(options)
+    if options.platform.startswith('ios-simulator'):
+        command = ['./run-safari', '--iphone-simulator', '--{}'.format(options.configuration)]
     else:
-        print('Default test behavior for this platform is not implemented...'.format(options.platform))
+        command = ['./run-minibrowser', '--{}'.format(options.configuration)]
 
     if command:
-        subprocess.call(command)
+        subprocess.call(command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
     return prompt_did_reproduce()
 
 
 def get_platforms(endpoint):
-    platform_url = urlparse.urljoin(REST_API_URL, endpoint)
-    r = urllib2.urlopen(platform_url)
+    platform_url = urllib.parse.urljoin(REST_API_URL, endpoint)
+    r = urllib.request.urlopen(platform_url)
     data = ""
     platforms = []
     for platform in data.get('Items'):
         platforms.append(str(platform['identifier']['S']))
-    
+
     return platforms
-     
+
+
 @memoized
 def minified_platforms():
     return get_platforms(REST_API_MINIFIED_PLATFORM_ENDPOINT)
- 
+
+
 @memoized
 def unminified_platforms():
     return get_platforms(REST_API_PLATFORM_ENDPOINT)
- 
-def is_supported_platform(options):
-    platform = '-'.join([options.platform, options.architecture, options.configuration])    
+
+
+def queue_for(options):
     if options.full:
-        return platform in unminified_platforms()
-    return platform in minified_platforms()
+        platform_list = unminified_platforms()
+    else:
+        platform_list = minified_platforms()
 
+    descriptor_from_options = QueueDescriptor(options.platform)
 
+    if not descriptor_from_options.architectures:
+        if options.architecture:
+            descriptor_from_options.architectures = set(options.architecture)
+    elif options.architecture is not None and descriptor_from_options.architectures != {options.architecture}:
+        return None
+
+    if not descriptor_from_options.configuration:
+        if options.configuration:
+            descriptor_from_options.configuration = options.configuration
+    elif options.configuration is not None and descriptor_from_options.configuration != options.configuration:
+        return None
+
+    for platform_name in platform_list:
+        available_platform = QueueDescriptor(platform_name)
+        if descriptor_from_options.platform_name != available_platform.platform_name:
+            continue
+        if descriptor_from_options.version and descriptor_from_options.version != available_platform.version:
+            continue
+        if not descriptor_from_options.architectures.issubset(available_platform.architectures):
+            continue
+        if descriptor_from_options.configuration and descriptor_from_options.configuration != available_platform.configuration:
+            continue
+        return platform_name
+    return None
+
+
+def print_platforms(platforms):
+    platform_strings = ['    {}'.format(QueueDescriptor(queue_name).pretty_string()) for queue_name in platforms]
+    print('\n'.join(sorted(platform_strings)))
+
+
 def validate_options(options):
-    if not is_supported_platform(options):
-        print('Unsupported platform combination: [{}], exiting...'.format('-'.join([options.platform, options.architecture, options.configuration])))
+    options.queue = queue_for(options)  # Resolve and cache for future use.
+    if options.queue is None:
+        print('Unsupported platform combination, exiting.')
         if options.full:
-            print('Available Unminified platforms: {}'.format(unminified_platforms()))
+            print('Available unminified platforms:')
+            print_platforms(unminified_platforms())
         else:
-            print('Available Minified platforms: {}'.format(minified_platforms()))
-            print('INFO: pass --full to try against full archives')
+            print('Available minified platforms:')
+            print_platforms(minified_platforms())
         exit(1)
 
+
 def print_list_and_exit(revision_list, options):
-        print('Supported minified platforms: {}'.format(minified_platforms()))
-        print('Supported unminified platforms: {}'.format(unminified_platforms()))
-        print('{} revisions available for supplied platform: {}-{}-{}:'.format(len(revision_list), options.platform, options.architecture, options.configuration))
-        print(revision_list)
-        exit(0)
+    print('Supported minified platforms:')
+    print_platforms(minified_platforms())
+    print('Supported unminified platforms:')
+    print_platforms(unminified_platforms())
+    print('{} revisions available for {}:'.format(len(revision_list), options.queue))
+    print(revision_list)
+    exit(0)
 
-def fetch_revision_list(options, LastEvaluatedKey=None):
-    url = "" LastEvaluatedKey)
-    r = urllib2.urlopen(url)
+
+def fetch_revision_list(options, last_evaluated_key=None):
+    url = "" last_evaluated_key)
+    r = urllib.request.urlopen(url)
     data = ""
     revision_list = get_sorted_revisions(data)
 
     if 'LastEvaluatedKey' in data['revisions']:
-        LastEvaluatedKey = data['revisions']['LastEvaluatedKey']
-        revision_list += fetch_revision_list(options, LastEvaluatedKey)
-        
+        last_evaluated_key = data['revisions']['LastEvaluatedKey']
+        revision_list += fetch_revision_list(options, last_evaluated_key)
+
     return revision_list
-    
-def main(options):
+
+
+def main():
+    options = parse_args(sys.argv[1:])
+    script_path = os.path.abspath(__file__)
+    script_directory = os.path.dirname(script_path)
+    os.chdir(script_directory)
+    webkit_output_dir = tempfile.mkdtemp()
+
     validate_options(options)
     revision_list = fetch_revision_list(options)
-    
+
     if options.list:
         print_list_and_exit(revision_list, options)
 
+    if not revision_list:
+        print('No archives found for {}.'.format(options.queue))
+        exit(1)
     start_index, end_index = get_indices_from_revisions(revision_list, options.start, options.end)
-    print('Bisecting between {} and {}'.format(revision_list[start_index], revision_list[end_index]))
-    
-    # from here forward, use indices instead of revisions
-    bisect_builds(revision_list, start_index, end_index, options)
 
+    set_webkit_output_dir(webkit_output_dir)
 
-if __name__ == '__main__':
-    options = parse_args(sys.argv[1:])
-    script_path = os.path.abspath(__file__)
-    script_directory = os.path.dirname(script_path)
-    os.chdir(script_directory)
-    webkit_output_dir = tempfile.mkdtemp()
-    set_webkit_output_dir(webkit_output_dir)
+    # From here forward, use indices instead of revisions.
     try:
-        main(options)    
+        if options.sanity_check:
+            if test_revision(options, revision_list[start_index]):
+                print('Issue reproduced with the first revision in the range, cannot bisect.')
+                exit(1)
+            if not test_revision(options, revision_list[end_index]):
+                print('Issue did not reproduce with the last revision in the range, cannot bisect.')
+                exit(1)
+
+        bisect_builds(revision_list, start_index, end_index, options)
     except KeyboardInterrupt:
-        exit("Aborting.")
+        exit(1)
     finally:
         shutil.rmtree(webkit_output_dir, ignore_errors=True)
+
+
+if __name__ == '__main__':
+    main()
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to