[Webkit-unassigned] [Bug 185045] Enhanced Download Build Product Script with Additional Use Cases

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=185045

--- Comment #17 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 340445
  --> https://bugs.webkit.org/attachment.cgi?id=340445
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180516/bbf17473/attachment-0001.html>