Bug 185045 – Enhanced Download Build Product Script with Additional Use Cases

• Component: Tools / Tests
• Status: NEW

[Amal Hussein]

Add support for a script which fetches binaries for a given platform/arch/config.

[Amal Hussein]

Created attachment 339013 [details]
Patch

[Amal Hussein]

I actually found an endpoint which returns all available build revisions ex: https://q1tzqfy48e.execute-api.us-west-2.amazonaws.com/v2/archives/mac-highsierra-x86_64-debug 

I'd like some feedback on whether I should use the archives endpoint or stick with the current one which is only the past 30 build revisions.

[youenn fablet]

Comment on attachment 339013 [details]
Patch

This goes in the right direction.
I wonder whether we could add some unit tests and what they would cover.
Please look at Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py and its unit tests to see whether the same approach would be usable/useful.

View in context: https://bugs.webkit.org/attachment.cgi?id=339013&action=review

> Tools/Scripts/download-build-product.py:29
> +import optparse

We should use argparse now if possible.
I guess that we can stick with optparse if argparse makes reusing platform/configuration options more difficult.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:1
> +# Copyright (C) 2014-2018 Apple Inc. All rights reserved.

2018

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:76
> +    def local_build_binaries_dir(self):

We usually do not use abbreviations, so directory might be better than dir.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:86
> +        elif self.port_name == "ios-simulator":

s/elif/if

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:88
> +        else:

No need for else

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:97
> +            if self.host.filesystem.exists(self.local_build_binaries_dir):

Maybe we should add a force option in case something bad happened and we need to overwrite the folder.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:137
> +        else:

No need for else.
Usually we would do the error case first and do the return after.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:156
> +            extract_exception = Exception('Cannot extract zipfile %s' % self.local_zip_path)

Can we do "import zipfile" and use that instead of platform specific code?

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:183
> +            shutil.rmtree(self.local_build_binaries_dir)

There is filesystem.rmtree I believe.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:186
> +            os.remove(self.local_zip_path)

There is filesystem.remove which might allow to remove the need to import os

[Ryosuke Niwa]

Comment on attachment 339013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339013&action=review

This clearly needs to share code with ./Tools/Scripts/bisect-builds. In fact, we have very similarly named script:
Tools/BuildSlaveSupport/download-built-product

I don't think we should land this script as is.

> Tools/Scripts/download-build-product.py:49
> +            optparse.make_option('--revision', type='str', default=None,
> +                                 help=('Pass in a build revision number from WebKit archives to download binaries. Revision=None will download the latest build.'))]),

Nit: wrong indentation. We always use 4 spaces, and never align to ( or any other punctuation.

[Amal Hussein]

Comment on attachment 339013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339013&action=review

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:97
>> +            if self.host.filesystem.exists(self.local_build_binaries_dir):
> 
> Maybe we should add a force option in case something bad happened and we need to overwrite the folder.

yes - thats a good idea - I'll add a force flag which we can use retry logic elsewhere

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:156
>> +            extract_exception = Exception('Cannot extract zipfile %s' % self.local_zip_path)
> 
> Can we do "import zipfile" and use that instead of platform specific code?

The reason why I'm using the platform-specific handling here is to avoid file permission issues I experienced when I did an extraction via zipfile.extract.  I had to use os.chmod to set permissions for the executable files, etc..

[Amal Hussein]

FYI Youenn --- I spoke with Ryosuke on IRC and we came up with a good comprise on merging the scripts. I'll refactor and push up changes. 

Thanks for your valuable feedback Ryosuke!

[Amal Hussein]

Created attachment 340416 [details]
Patch

[Amal Hussein]

Change Summary:

1) New Common BuildBinaryFetcher:
- Downloads and extracts minified archives from s3 to WebkitBuild/Release or WebkitBuild/Debug by default
- Downloads and extracts full archives from s3 via optional `full` property
- Downloads either full or minified archive without extracting via optional `no-extract` property
- Downloads and extracts to a specified build directory via optional `build-directory` property
- Constructs the s3 zip url from supplied options if no zip url is passed in
- Fetches the s3 zip directly if an s3 zip url is passed in via the optional `s3_zip_url` property
- Can return a list of sorted revisions for a given set build type
- Prompts a user if a build already exists in the intended extract path
- Skips prompting the user via an optional `delete-first` property

2) Biscet-build:
- gets the sorted revision list from the BuildBinaryFetcher class
- updated to remove all the logic for constructing the s3 zip URL
- removed separate call to extract the archive
- calls the enhanced called the download-build-product with given options and the `delete-first` flag to match existing behavior

3) build-product-archive:
- enhanced to support extracting to a specified build directory via optional `--build-directory` flag

[Alexey Proskuryakov]

Comment on attachment 340416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340416&action=review

> Tools/BuildSlaveSupport/download-built-product:3
> -# Copyright (C) 2009 Apple Inc.  All rights reserved.
> +# Copyright (C) 2018 Apple Inc.  All rights reserved.

The copyright line is supposed to include all years in which published contributions were made, not just the latest. I'm not exactly sure why this says 2009 when the file was created in 2012, but perhaps that's because code was copy/pasted from an older file.

[Amal Hussein]

Created attachment 340445 [details]
Patch

[Amal Hussein]

(In reply to Alexey Proskuryakov from comment #9)
> Comment on attachment 340416 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340416&action=review
> 
> > Tools/BuildSlaveSupport/download-built-product:3
> > -# Copyright (C) 2009 Apple Inc.  All rights reserved.
> > +# Copyright (C) 2018 Apple Inc.  All rights reserved.
> 
> The copyright line is supposed to include all years in which published
> contributions were made, not just the latest. I'm not exactly sure why this
> says 2009 when the file was created in 2012, but perhaps that's because code
> was copy/pasted from an older file.

Good catch Alexey! Updated. 

Unrelated question: I'm not sure how to update the svn ignore. Do I need to check out the svn version of webkit? 

I've tried running `svn propset svn:ignore /DownloadedBinaries/ .` but it throws an svn: E155007: error with this msg my `/webkit' is not a working copy`.

[Alexey Proskuryakov]

> Unrelated question: I'm not sure how to update the svn ignore. Do I need to check out the svn version of webkit? 

Yes.

[EWS Watchlist]

Comment on attachment 340445 [details]
Patch

Attachment 340445 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7693395

New failing tests:
transitions/interrupted-transition-hardware.html

[EWS Watchlist]

Created attachment 340459 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Amal Hussein]

A flaky test was identified and rolled out right after the ios-sim build ran --  LayoutTests/transitions/interrupted-transition-hardware.html. So, the ios-sim failure is unrelated to my change.

See https://bugs.webkit.org/show_bug.cgi?id=185668 and https://trac.webkit.org/changeset/231823/webkit for details.

Does the ios-sim build need to be manually retriggered?

[Alexey Proskuryakov]

> Does the ios-sim build need to be manually retriggered?

No. We know that the only failure is unrelated, and there isn't currently a way to re-trigger, short of uploading a new patch.

[youenn fablet]

Comment on attachment 340445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340445&action=review

> Tools/BuildSlaveSupport/download-built-product:3
> +# Copyright (C) 2012-2018 Apple Inc.  All rights reserved.

You can also add your own Copyright in the list here.

> Tools/BuildSlaveSupport/download-built-product:57
> +    parser.add_argument('--delete-first', action='store_true', default=False, help='Override an exisiting build directory.')

Maybe --clean would be a better name.
s/exisiting/existing/

> Tools/BuildSlaveSupport/download-built-product:58
> +    parser.add_argument('--no-extract', action='store_true', default=False, help='Only download archives without extracting.')

Maybe should be inverted to --extract, default value being True then?

> Tools/BuildSlaveSupport/download-built-product:62
> +    build_binaries_fetcher = BuildBinariesFetcher(Host(), args.platform, args.architecture, args.configuration, args.full, args.revision, args.build_directory, args.delete_first, args.no_extract, args.url)

Why not passing args directly?
This will make it easier if we ever add a new option.

> Tools/Scripts/bisect-builds:194
> +    revision_list = build_binaries_fetcher.get_sorted_revisions()

Maybe get_sorted_revisions could be a class function of BuildBinariesFetcher and would take platform, architecture and configuration as parameters?

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:43
> +    def __init__(self, host, platform, architecture='x86_64', configuration='release', full=False, revision=None, build_directory=None, delete_first=False, no_extract=False, url=None):

Do we need architecture and configuration defaults?
It seems current call sites provide those values explicitly.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:53
> +        self.s3_zip_url = url

I am not sure all these values need to be public.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:60
> +            build_directory = ', '.join(self.build_directory.split('/')), self.platform + self.architecture + self.revision

Does this work in windows?
Should we use Filesystem.sep?
Should we add '-' separators between platform/architecture/revision? or maybe use s3_build_type for the final directory name.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:76
> +            s3_api_base_path = self.host.filesystem.join(REST_API_URL, REST_API_ENDPOINT)

Maybe rewrite it to "s3_api_base_path - ... join(REST_API_URL,REST_API_ENDPOINT if self.should_use_download_unminified_url else REST_API_MINIFIED_ENDPOINT)

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:85
> +        return self.host.filesystem.join(self.local_downloaded_binaries_directory, self.configuration + '.zip')

Should the configuration be lowercased/capitalized or does it not matter?

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:92
> +        _log.info('Fetching JSON from %s', url)

I would put this log at the beginning just before urlopen.
And then write:
return json.load(response)

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:96
> +        ans = raw_input('\n A build already exists at %s. Do you want to override it [y/n]: ' % self.local_extracted_directory)

s/ans/answer

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:103
> +            self._prompt_user_to_delete_first()

There is the confirm utility in Tools/Scripts/webkitpy/common/system/user.py that should help.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:108
> +                self.revision = os.path.basename(self.s3_zip_url).strip('.zip')

We should be using filesystem.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:115
> +

Empty line not needed probably.

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:117
> +                    _log.info('\n Aborting... to download build in another directory use the --build-directory flag')

s/... to/... To/

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:145
> +

line unneeded?

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:150
> +                raise Exception('22 No build revisions found at: %s' % self.s3_build_binaries_url)

s/22// ?

> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:157
> +

Ditto?

[Amal Hussein]

Comment on attachment 340445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340445&action=review

Youenn, thanks so much for the awesome feedback. Questions and comments below:

>> Tools/BuildSlaveSupport/download-built-product:62
>> +    build_binaries_fetcher = BuildBinariesFetcher(Host(), args.platform, args.architecture, args.configuration, args.full, args.revision, args.build_directory, args.delete_first, args.no_extract, args.url)
> 
> Why not passing args directly?
> This will make it easier if we ever add a new option.

Agreed - I passed args in an earlier iteration locally, but then changed it because of missing args in some use cases.  I was trying to avoid `if` statements inside `__init__`.  There may be a more elegant way to handle that.

>> Tools/Scripts/bisect-builds:194
>> +    revision_list = build_binaries_fetcher.get_sorted_revisions()
> 
> Maybe get_sorted_revisions could be a class function of BuildBinariesFetcher and would take platform, architecture and configuration as parameters?

good call - it's much better to avoid the initialization.

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:43
>> +    def __init__(self, host, platform, architecture='x86_64', configuration='release', full=False, revision=None, build_directory=None, delete_first=False, no_extract=False, url=None):
> 
> Do we need architecture and configuration defaults?
> It seems current call sites provide those values explicitly.

Good question - I left this in because I didn't want to rely on an external dependency to set the defaults.
This brings up a larger question on where validation should live. 
I considered moving the `supported_platforms` logic into its own common helper file, and methods for architecture and configuration can also live there and be imported here. 
Should this class have its own validation helpers which check that the  platform, architecture, and configuration are in the shared globals?

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:53
>> +        self.s3_zip_url = url
> 
> I am not sure all these values need to be public.

public api - platform, architecture,  revision, build_directory, s3_zip_url?

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:60
>> +            build_directory = ', '.join(self.build_directory.split('/')), self.platform + self.architecture + self.revision
> 
> Does this work in windows?
> Should we use Filesystem.sep?
> Should we add '-' separators between platform/architecture/revision? or maybe use s3_build_type for the final directory name.

yes, this works in windows - line 60 returns a tuple of all the directories, and that gets passed to WebKitFinder in line 62. 

I initially used the s3_build_type in a previous version of the patch but got feedback about the nested structure. 

Which option is preferable - both utilize s3_build_type:

A) DownloadedBinaries/ios-simulator-11-x86_64-release/231081
B) DownloadedBinaries/ios-simulator-11-x86_64-release-231081

>> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:85
>> +        return self.host.filesystem.join(self.local_downloaded_binaries_directory, self.configuration + '.zip')
> 
> Should the configuration be lowercased/capitalized or does it not matter?

I explicitly capitalize it when it does matter - see line 66. 

Its probably best sanitize all the string args to be lower case for consistency with logging and checking if a directory exists on Linux or mac. I will make that update.

[youenn fablet]

> >> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:43
> >> +    def __init__(self, host, platform, architecture='x86_64', configuration='release', full=False, revision=None, build_directory=None, delete_first=False, no_extract=False, url=None):
> > 
> > Do we need architecture and configuration defaults?
> > It seems current call sites provide those values explicitly.
> 
> Good question - I left this in because I didn't want to rely on an external
> dependency to set the defaults.
> This brings up a larger question on where validation should live. 
> I considered moving the `supported_platforms` logic into its own common
> helper file, and methods for architecture and configuration can also live
> there and be imported here. 
> Should this class have its own validation helpers which check that the 
> platform, architecture, and configuration are in the shared globals?

Sounds ok to me, as long as the validation code is shared by all users.

> >> Tools/Scripts/webkitpy/common/build_binaries_fetcher.py:53
> >> +        self.s3_zip_url = url
> > 
> > I am not sure all these values need to be public.
> 
> public api - platform, architecture,  revision, build_directory, s3_zip_url?

Usually, we go to private by default and public if needed.
So maybe these values could be made private if they are not used elsewhere for now.

[Alex Christensen]

Comment on attachment 340445 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.