Bug 124848 – Add support for multiple sources for AutoInstaller

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Berta József]

The autoinstaller in the webkitpy currently fails if the download source of a package is unavailable. This patch adds support for multiple sources to the script. The sources are provided in three environment variables. If it exists, the script will look at a local cache. If not, it will try to download the package from the original url. If it fails, it gets a mirror from the corresponding environment variable.(One for sourceforge and one for pypi.python.org)

[Berta József]

Created attachment 217818 [details]
Support for multiple sources in the autoinstaller of webkitpy.

[Ryosuke Niwa]

Comment on attachment 217818 [details]
Support for multiple sources in the autoinstaller of webkitpy.

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.

[Berta József]

Comment on attachment 217818 [details]
Support for multiple sources in the autoinstaller of webkitpy.

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?

I just think it's clearer to have these separated here, and put them together after the env. variables are processed. It might reduce readability if the array would be made here with the variable names, and later replaced by the contents.

>> Tools/Scripts/webkitpy/common/system/autoinstall.py:359
>> +                        continue
> 
> It's strange that we'll select the next mirror only if the current mirror matched.
> How does that work?

The url will match sourceforge or pypi, if the original url did, and if the mirrors are from the mirror sources above. So I decided to distinguish the source by the url. If the current url doesn't matched, it's not from pypi or sourceforge.
The other way could be to add an indicator to each source in the webkitpy/thirdparty/__init__.py, but it's not necessary in this case.

[Berta József]

Created attachment 218775 [details]
Correcting patch.

[Ryosuke Niwa]

Comment on attachment 218775 [details]
Correcting patch.

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

> Tools/Scripts/webkitpy/common/system/autoinstall.py:54
> +_mirror_regexs = re.compile('.*sourceforge.*'), re.compile('.*pypi.*')
> +_pypi_env_var = 'PYPI_MIRRORS'
> +_sourceforge_env_var = 'SOURCEFORGE_MIRRORS'
> +_cache_env_var = 'LOCAL_AUTOINSTALL_CACHE'

I think we normally use ALL CAPS for constants.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:332
> +            if regex.match(parsed_url[1]):
> +                if mirror_index < len(addresses):

Combine these two ifs.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:355
> +                    url = self._replace_domain_with_next_mirror(url, mirrors, mirror_index)
> +                    if url:

Why don't we just pop the next item from mirrors instead of keeping mirror_index separately?

> Tools/Scripts/webkitpy/common/system/autoinstall.py:385
> +        #The cache must contain the packages in their packed forms(As if they were just downloaded from the web)
> +
> +        #The path of the cache is stored in the environment variable named in the _cache_env_var variable.

Nit: You should put a space between # and The.
Nit: You also need a space between forms and (, and "As" should not be capitalized and ) should be followed by a single period.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:398
> +        #The installer first looks at the local cache, and if the package is found there, it will be installed from there.

I don't think this comment is necessary.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:407
> +        # If this part of the code is reached, there is no copy of the package in the local cache, so it will be copied there.

Ditto.

[Berta József]

Created attachment 218945 [details]
Replaced the mirror lists with iteratos.

[Berta József]

(In reply to comment #6)
> Created an attachment (id=218945) [details]
> Replaced the mirror lists with iteratos.

Could you please take a look at this?

[Ryosuke Niwa]

Comment on attachment 218945 [details]
Replaced the mirror lists with iteratos.

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

> Tools/ChangeLog:1
> +

Nit: remove this empty line.

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

get prefix is used for functions with an out arguments. See our style guideline.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:306
> +        """If the original download url fails, it is replaced by a mirror provided in the environment variables

This is what the caller of this function does.  I don't think we need this comment here.  Please remove it.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:310
> +        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 is an excessive amount of comments/examples for a very simple example.

It's much better to convey this information via a more descriptive function name such as
parse_colon_separated_mirrors_from_env

> Tools/Scripts/webkitpy/common/system/autoinstall.py:327
> +        parsed_url = list(urlparse.urlparse(url))

Why are we converting the parsed result into a list?  That's reducing the readability of the code below.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:353
> +                else:

Since the previous if statement ends with continue, we shouldn't have else clause. See our style guideline.

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

I don't think we if should use the prefix "find" given that this function doesn't search through the file system.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:384
> +        # The cache must contain the packages in their packed forms, as if they were just downloaded from the web.
> +
> +        # The path of the cache is stored in the environment variable named in the _CACHE_ENV_VAR variable.

These two comments are more confusing than helpful as far as I read the code. Please remove them.

> Tools/Scripts/webkitpy/common/system/autoinstall.py:385
> +        if _CACHE_ENV_VAR in os.environ:

We prefer an early exit. e.g.
if _CACHE_ENV_VAR not in os.environ:
    return False
...
if not path:
    return False
return ...

> Tools/Scripts/webkitpy/common/system/autoinstall.py:388
> +            path = glob(os.path.join(os.environ[_CACHE_ENV_VAR], filename) + '*')
> +            if path:
> +                return path[0]

Why are we returning the first path?

> Tools/Scripts/webkitpy/common/system/autoinstall.py:390
> +        else:
> +            return False

These two lines are complete no-op as is.

[Berta József]

Comment on attachment 218945 [details]
Replaced the mirror lists with iteratos.

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

>> Tools/Scripts/webkitpy/common/system/autoinstall.py:310
>> +        Mirror sources: http://www.pypi-mirrors.org/, http://sourceforge.net/apps/trac/sourceforge/wiki/Mirrors
> 
> This is an excessive amount of comments/examples for a very simple example.
> 
> It's much better to convey this information via a more descriptive function name such as
> parse_colon_separated_mirrors_from_env

Ok, I removed most of it, but the rest is necessary, i think

>> Tools/Scripts/webkitpy/common/system/autoinstall.py:327
>> +        parsed_url = list(urlparse.urlparse(url))
> 
> Why are we converting the parsed result into a list?  That's reducing the readability of the code below.

The urlparse() returns a tuple, which can't be modified.

>> Tools/Scripts/webkitpy/common/system/autoinstall.py:388
>> +                return path[0]
> 
> Why are we returning the first path?

Glob returns a list, in this case containing only one string. We only need the string, not a list.

[Berta József]

Created attachment 220629 [details]
Modifying patch.

[Csaba Osztrogonác]

(In reply to comment #10)
> Created an attachment (id=220629) [details]
> Modifying patch.

The patch looks good to me, it seems everything 
is fixed now. But I let rniwa to set the final r+ .

[WebKit Commit Bot]

Comment on attachment 220629 [details]
Modifying patch.

Clearing flags on attachment: 220629

Committed r163570: <http://trac.webkit.org/changeset/163570>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.