[Webkit-unassigned] [Bug 124848] Add support for multiple sources for AutoInstaller

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #217818|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org>  2013-12-03 11:35:40 PST ---
(From update of attachment 217818)
View in context: https://bugs.webkit.org/attachment.cgi?id=217818&action=review

> Tools/Scripts/webkitpy/common/system/autoinstall.py:51
> +_mirror_regexs = re.compile('.*sourceforge.*'), re.compile('.*pypi.*')

Why don't we have a single array that contains both the regular expression and the env. variable name instead of zip'ing in _prepare_mirrors?

> Tools/Scripts/webkitpy/common/system/autoinstall.py:53
> +_sf_env_var = 'SOURCEFORGE_MIRRORS'

Please don't use abbreviations such as sf. Spell out source forge.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:269
> +    def _copy_not_packaged(self, path, scratch_dir):

s/not_packaged/unpackaged_files/

> Tools/Scripts/webkitpy/common/system/autoinstall.py:270
> +        """Copy the not packaged file to the scratch_dir in order to leave the local cache intact. """

We no longer add these Python-style one-line comments. That's reminiscent of Google-owned code.
Please give the method descriptive name such as copy_unpackaged_file_from_local_cache to avoid having to add these comments.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:306
> +    def _prepare_mirrors(self):

What does mean to say "prepare" mirrors?
It's better to give a more descriptive method name such as mirrors_from_env.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:320
> +    def _switch_to_mirror(self, url, mirrors, mirror_index):

Again, _switch_to_mirror is not the most descriptive method name here.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:325
> +        The environment variables must contain the values separated by colons.
> +        Pypi mirror examle: PYPI_MIRRORS=pypi.hustunique.com:e.pypi.python.org...
> +        Sourceforge mirror example: SOURCEFORGE_MIRRORS=aarnet.dl.sourceforge.net:citylan.dl.sourceforge.net:dfn.dl.sourceforge.net:freefr.dl.sourceforge.net...
> +        Mirror sources: http://www.pypi-mirrors.org/, http://sourceforge.net/apps/trac/sourceforge/wiki/Mirrors

This description belongs in _prepare_mirrors.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:359
> +                    url = self._switch_to_mirror(url, mirrors, mirror_index)
> +                    if url:
> +                        failures = 0
> +                        mirror_index += 1
> +                        continue

It's strange that we'll select the next mirror only if the current mirror matched.
How does that work?

> Tools/Scripts/webkitpy/common/system/autoinstall.py:383
> +    def _find_in_cache(self, filename):

It's better to call this _find_in_local_autoinstall_cache to be more explicit.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:387
> +        """The installer first looks at the local cache, and if the package is found there, it will be installed from there.
> +        The path of the cache is stored in the LOCAL_AUTOINSTALL_CACHE environment variable.
> +        The cache must contain the packages in their packed forms(As if they were just downloaded from the web)
> +        """

Doesn't seem like this is the description of this method.  It's better to add a comment in the relevant parts of _download as needed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.