[Webkit-unassigned] [Bug 185045] Add Support for a Download Build Product Script

[bugzilla-daemon at webkit.org]

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

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

-- 
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/20180427/fe70c57f/attachment.html>