[webkit-reviews] review granted: [Bug 35163] autoinstall: should unzip downloaded zip and gzip files prior to importing : [Attachment 49283] Proposed patch 4

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 35163: autoinstall: should unzip downloaded zip and gzip files prior to
importing
https://bugs.webkit.org/show_bug.cgi?id=35163

Attachment 49283: Proposed patch 4
https://bugs.webkit.org/attachment.cgi?id=49283&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just some minor nits to consider addressing before landing.
 


> diff --git a/WebKitTools/Scripts/webkitpy/thirdparty/autoinstall.py
b/WebKitTools/Scripts/webkitpy/thirdparty/autoinstall.py

You really re-wrote this. I don't think it should be in thirdparty anymore,
even though it originated that way. At this point,
we can't take upstream changes and it should match WK coding guidelines but
that can happen in another patch.



> +    def _extract_targz(self, path, scratch_dir):
> +	   # tarfile.extractall() extracts to a path without the
> +	   # trailing ".tar.gz".
> +	   #
> +	   # The substring ".tar.gz" has seven characters.
> +	   target_basename = os.path.basename(path[:-7])

How about
  target_basename = os.path.basename(path[:-len(".tar.gz")])
with no comment?


> +	   target_path = os.path.join(scratch_dir, target_basename)
> +
> +	   self._log_transfer("Starting gunzip/extract...", path, target_path)
> +
> +	   tfile = tarfile.open(path)

tfile is an odd abbreviation. How about tar_file or compressed_file, etc.?


> +    def _unzip(self, path, scratch_dir):
> +	   # zipfile.extractall() extracts to a path without the
> +	   # trailing ".zip".
> +	   #
> +	   # The substring ".zip" has four characters.
> +	   target_basename = os.path.basename(path[:-4])

Or
	target_basename = os.path.basename(path[:-len(".zip")])

with no comment (about the length of ".zip").


> +    def _download(self, url, scratch_dir):
> +	   """Download URL contents, and return the download path."""
> +	   self._log_transfer("Starting download...", url, scratch_dir)
> +	   url_path = urlparse.urlsplit(url)[2]
> +	   url_path = os.path.normpath(url_path)  # Removes trailing slash.

Unless there is some pep8 rule, stick with the one space between code and
comments.


> +
> +    def install(self, url, should_refresh=False, target_name=None,
> +		   url_subpath=None):
> +
> +	   target_path = os.path.join(self._target_dir, target_name)
> +	   if (not should_refresh) and self._is_downloaded(target_name, url):

no need for parens around "not should_refresh"