Merge branch 'chiaratolentino/fix-pip-source-regex' into 'master'

plugins/sources/pip.py: Accomodate characters '-','.','_' for packages

See merge request BuildStream/buildstream!914

9 files changed, 203 insertions, 32 deletions

diff --git a/buildstream/plugins/sources/pip.py b/buildstream/plugins/sources/pip.py
index 6ba3a0407..2ef401620 100644
--- a/buildstream/plugins/sources/pip.py
+++ b/buildstream/plugins/sources/pip.py
@@ -96,7 +96,7 @@ _PYTHON_VERSIONS = [
 # Names of source distribution archives must be of the form
 # '%{package-name}-%{version}.%{extension}'.
 _SDIST_RE = re.compile(
-    r'^([a-zA-Z0-9]+?)-(.+).(?:tar|tar.bz2|tar.gz|tar.xz|tar.Z|zip)$',
+    r'^([\w.-]+?)-((?:[\d.]+){2,})\.(?:tar|tar.bz2|tar.gz|tar.xz|tar.Z|zip)$',
     re.IGNORECASE)
 
 
@@ -225,12 +225,27 @@ class PipSource(Source):
     def _parse_sdist_names(self, basedir):
         reqs = []
         for f in os.listdir(basedir):
-            pkg_match = _SDIST_RE.match(f)
-            if pkg_match:
-                reqs.append(pkg_match.groups())
+            pkg = _match_package_name(f)
+            if pkg is not None:
+                reqs.append(pkg)
 
         return sorted(reqs)
 
 
+# Extract the package name and version of a source distribution
+#
+# Args:
+#    filename (str): Filename of the source distribution
+#
+# Returns:
+#    (tuple): A tuple of (package_name, version)
+#
+def _match_package_name(filename):
+    pkg_match = _SDIST_RE.match(filename)
+    if pkg_match is None:
+        return None
+    return pkg_match.groups()
+
+
 def setup():
     return PipSource
diff --git a/tests/integration/pip_source.py b/tests/integration/pip_source.py
index fc5b56a7c..84cf5dda9 100644
--- a/tests/integration/pip_source.py
+++ b/tests/integration/pip_source.py
@@ -4,6 +4,7 @@ import pytest
 from buildstream import _yaml
 
 from tests.testutils import cli_integration as cli
+from tests.testutils.python_repo import setup_pypi_repo
 from tests.testutils.integration import assert_contains
 
 
@@ -17,12 +18,21 @@ DATA_DIR = os.path.join(
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_pip_source_import(cli, tmpdir, datafiles):
+def test_pip_source_import(cli, tmpdir, datafiles, setup_pypi_repo):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     checkout = os.path.join(cli.directory, 'checkout')
     element_path = os.path.join(project, 'elements')
     element_name = 'pip/hello.bst'
 
+    # check that exotically named packages are imported correctly
+    myreqs_packages = ['hellolib']
+    packages = ['app2', 'app.3', 'app-4', 'app_5', 'app.no.6', 'app-no-7', 'app_no_8']
+
+    # create mock pypi repository
+    pypi_repo = os.path.join(project, 'files', 'pypi-repo')
+    os.makedirs(pypi_repo, exist_ok=True)
+    setup_pypi_repo(myreqs_packages + packages, pypi_repo)
+
     element = {
         'kind': 'import',
         'sources': [
@@ -32,9 +42,9 @@ def test_pip_source_import(cli, tmpdir, datafiles):
             },
             {
                 'kind': 'pip',
-                'url': 'file://{}'.format(os.path.realpath(os.path.join(project, 'files', 'pypi-repo'))),
+                'url': 'file://{}'.format(os.path.realpath(pypi_repo)),
                 'requirements-files': ['myreqs.txt'],
-                'packages': ['app2']
+                'packages': packages
             }
         ]
     }
@@ -51,16 +61,31 @@ def test_pip_source_import(cli, tmpdir, datafiles):
     assert result.exit_code == 0
 
     assert_contains(checkout, ['/.bst_pip_downloads',
-                               '/.bst_pip_downloads/HelloLib-0.1.tar.gz',
-                               '/.bst_pip_downloads/App2-0.1.tar.gz'])
+                               '/.bst_pip_downloads/hellolib-0.1.tar.gz',
+                               '/.bst_pip_downloads/app2-0.1.tar.gz',
+                               '/.bst_pip_downloads/app.3-0.1.tar.gz',
+                               '/.bst_pip_downloads/app-4-0.1.tar.gz',
+                               '/.bst_pip_downloads/app_5-0.1.tar.gz',
+                               '/.bst_pip_downloads/app.no.6-0.1.tar.gz',
+                               '/.bst_pip_downloads/app-no-7-0.1.tar.gz',
+                               '/.bst_pip_downloads/app_no_8-0.1.tar.gz'])
 
 
 @pytest.mark.datafiles(DATA_DIR)
-def test_pip_source_build(cli, tmpdir, datafiles):
+def test_pip_source_build(cli, tmpdir, datafiles, setup_pypi_repo):
     project = os.path.join(datafiles.dirname, datafiles.basename)
     element_path = os.path.join(project, 'elements')
     element_name = 'pip/hello.bst'
 
+    # check that exotically named packages are imported correctly
+    myreqs_packages = ['hellolib']
+    packages = ['app2', 'app.3', 'app-4', 'app_5', 'app.no.6', 'app-no-7', 'app_no_8']
+
+    # create mock pypi repository
+    pypi_repo = os.path.join(project, 'files', 'pypi-repo')
+    os.makedirs(pypi_repo, exist_ok=True)
+    setup_pypi_repo(myreqs_packages + packages, pypi_repo)
+
     element = {
         'kind': 'manual',
         'depends': ['base.bst'],
@@ -71,16 +96,15 @@ def test_pip_source_build(cli, tmpdir, datafiles):
             },
             {
                 'kind': 'pip',
-                'url': 'file://{}'.format(os.path.realpath(os.path.join(project, 'files', 'pypi-repo'))),
+                'url': 'file://{}'.format(os.path.realpath(pypi_repo)),
                 'requirements-files': ['myreqs.txt'],
-                'packages': ['app2']
+                'packages': packages
             }
         ],
         'config': {
             'install-commands': [
                 'pip3 install --no-index --prefix %{install-root}/usr .bst_pip_downloads/*.tar.gz',
-                'chmod +x app1.py',
-                'install app1.py  %{install-root}/usr/bin/'
+                'install app1.py %{install-root}/usr/bin/'
             ]
         }
     }
@@ -95,5 +119,4 @@ def test_pip_source_build(cli, tmpdir, datafiles):
 
     result = cli.run(project=project, args=['shell', element_name, '/usr/bin/app1.py'])
     assert result.exit_code == 0
-    assert result.output == """Hello App1!
-"""
+    assert result.output == "Hello App1! This is hellolib\n"
diff --git a/tests/integration/project/files/pypi-repo/app2/App2-0.1.tar.gz b/tests/integration/project/files/pypi-repo/app2/App2-0.1.tar.gz
deleted file mode 100644
index 86cb43cfe..000000000
--- a/tests/integration/project/files/pypi-repo/app2/App2-0.1.tar.gz
+++ /dev/null
diff --git a/tests/integration/project/files/pypi-repo/app2/index.html b/tests/integration/project/files/pypi-repo/app2/index.html
deleted file mode 100644
index 5bc72e47c..000000000
--- a/tests/integration/project/files/pypi-repo/app2/index.html
+++ /dev/null
@@ -1,8 +0,0 @@
-<html>
-  <head>
-    <title>Links for app1</title>
-  </head>
-  <body>
-    <a href='App2-0.1.tar.gz'>App2-0.1.tar.gz</a><br />
-  </body>
-</html>
diff --git a/tests/integration/project/files/pypi-repo/hellolib/HelloLib-0.1.tar.gz b/tests/integration/project/files/pypi-repo/hellolib/HelloLib-0.1.tar.gz
deleted file mode 100644
index 3b0884c66..000000000
--- a/tests/integration/project/files/pypi-repo/hellolib/HelloLib-0.1.tar.gz
+++ /dev/null
diff --git a/tests/integration/project/files/pypi-repo/hellolib/index.html b/tests/integration/project/files/pypi-repo/hellolib/index.html
deleted file mode 100644
index eb9935c7f..000000000
--- a/tests/integration/project/files/pypi-repo/hellolib/index.html
+++ /dev/null
@@ -1,8 +0,0 @@
-<html>
-  <head>
-    <title>Links for app1</title>
-  </head>
-  <body>
-    <a href='HelloLib-0.1.tar.gz'>HelloLib-0.1.tar.gz</a><br />
-  </body>
-</html>
diff --git a/tests/sources/pip.py b/tests/sources/pip.py
index 8b4c213dc..6e1a347d9 100644
--- a/tests/sources/pip.py
+++ b/tests/sources/pip.py
@@ -3,6 +3,7 @@ import pytest
 
 from buildstream._exceptions import ErrorDomain
 from buildstream import _yaml
+from buildstream.plugins.sources.pip import _match_package_name
 from tests.testutils import cli
 
 DATA_DIR = os.path.join(
@@ -45,3 +46,22 @@ def test_no_packages(cli, tmpdir, datafiles):
         'show', 'target.bst'
     ])
     result.assert_main_error(ErrorDomain.SOURCE, None)
+
+
+# Test that pip source parses tar ball names correctly for the ref
+@pytest.mark.parametrize(
+    'tarball, expected_name, expected_version',
+    [
+        ('dotted.package-0.9.8.tar.gz', 'dotted.package', '0.9.8'),
+        ('hyphenated-package-2.6.0.tar.gz', 'hyphenated-package', '2.6.0'),
+        ('underscore_pkg-3.1.0.tar.gz', 'underscore_pkg', '3.1.0'),
+        ('numbers2and5-1.0.1.tar.gz', 'numbers2and5', '1.0.1'),
+        ('multiple.dots.package-5.6.7.tar.gz', 'multiple.dots.package', '5.6.7'),
+        ('multiple-hyphens-package-1.2.3.tar.gz', 'multiple-hyphens-package', '1.2.3'),
+        ('multiple_underscore_pkg-3.4.5.tar.gz', 'multiple_underscore_pkg', '3.4.5'),
+        ('shortversion-1.0.tar.gz', 'shortversion', '1.0'),
+        ('longversion-1.2.3.4.tar.gz', 'longversion', '1.2.3.4')
+    ])
+def test_match_package_name(tarball, expected_name, expected_version):
+    name, version = _match_package_name(tarball)
+    assert (expected_name, expected_version) == (name, version)
diff --git a/tests/testutils/__init__.py b/tests/testutils/__init__.py
index 4a79c3be2..eb7211ea8 100644
--- a/tests/testutils/__init__.py
+++ b/tests/testutils/__init__.py
@@ -29,3 +29,4 @@ from .artifactshare import create_artifact_share
 from .element_generators import create_element_size, update_element_size
 from .junction import generate_junction
 from .runner_integration import wait_for_cache_granularity
+from .python_repo import setup_pypi_repo
diff --git a/tests/testutils/python_repo.py b/tests/testutils/python_repo.py
new file mode 100644
index 000000000..9cda5e910
--- /dev/null
+++ b/tests/testutils/python_repo.py
@@ -0,0 +1,128 @@
+from setuptools.sandbox import run_setup
+import os
+import pytest
+import re
+import shutil
+
+
+SETUP_TEMPLATE = '''\
+from setuptools import setup
+
+setup(
+    name='{name}',
+    version='{version}',
+    description='{name}',
+    packages=['{pkgdirname}'],
+    entry_points={{
+        'console_scripts': [
+            '{pkgdirname}={pkgdirname}:main'
+        ]
+    }}
+)
+'''
+
+# All packages generated via generate_pip_package will have the functions below
+INIT_TEMPLATE = '''\
+def main():
+    print('This is {name}')
+
+def hello(actor='world'):
+    print('Hello {{}}! This is {name}'.format(actor))
+'''
+
+HTML_TEMPLATE = '''\
+<html>
+  <head>
+    <title>Links for {name}</title>
+  </head>
+  <body>
+    <a href='{name}-{version}.tar.gz'>{name}-{version}.tar.gz</a><br />
+  </body>
+</html>
+'''
+
+
+# Creates a simple python source distribution and copies this into a specified
+# directory which is to serve as a mock python repository
+#
+# Args:
+#    tmpdir (str): Directory in which the source files will be created
+#    pypi (str): Directory serving as a mock python repository
+#    name (str): The name of the package to be created
+#    version (str): The version of the package to be created
+#
+# Returns:
+#    None
+#
+def generate_pip_package(tmpdir, pypi, name, version='0.1'):
+    # check if package already exists in pypi
+    pypi_package = os.path.join(pypi, re.sub('[^0-9a-zA-Z]+', '-', name))
+    if os.path.exists(pypi_package):
+        return
+
+    # create the package source files in tmpdir resulting in a directory
+    # tree resembling the following structure:
+    #
+    # tmpdir
+    # |-- setup.py
+    # `-- package
+    #     `-- __init__.py
+    #
+    setup_file = os.path.join(tmpdir, 'setup.py')
+    pkgdirname = re.sub('[^0-9a-zA-Z]+', '', name)
+    with open(setup_file, 'w') as f:
+        f.write(
+            SETUP_TEMPLATE.format(
+                name=name,
+                version=version,
+                pkgdirname=pkgdirname
+            )
+        )
+    os.chmod(setup_file, 0o755)
+
+    package = os.path.join(tmpdir, pkgdirname)
+    os.makedirs(package)
+
+    main_file = os.path.join(package, '__init__.py')
+    with open(main_file, 'w') as f:
+        f.write(INIT_TEMPLATE.format(name=name))
+    os.chmod(main_file, 0o644)
+
+    run_setup(setup_file, ['sdist'])
+
+    # create directory for this package in pypi resulting in a directory
+    # tree resembling the following structure:
+    #
+    # pypi
+    # `-- pypi_package
+    #     |-- index.html
+    #     `-- foo-0.1.tar.gz
+    #
+    os.makedirs(pypi_package)
+
+    # add an index html page
+    index_html = os.path.join(pypi_package, 'index.html')
+    with open(index_html, 'w') as f:
+        f.write(HTML_TEMPLATE.format(name=name, version=version))
+
+    # copy generated tarfile to pypi package
+    dist_dir = os.path.join(tmpdir, 'dist')
+    for tar in os.listdir(dist_dir):
+        tarpath = os.path.join(dist_dir, tar)
+        shutil.copy(tarpath, pypi_package)
+
+
+@pytest.fixture
+def setup_pypi_repo(tmpdir):
+    def create_pkgdir(package):
+        pkgdirname = re.sub('[^0-9a-zA-Z]+', '', package)
+        pkgdir = os.path.join(str(tmpdir), pkgdirname)
+        os.makedirs(pkgdir)
+        return pkgdir
+
+    def add_packages(packages, pypi_repo):
+        for package in packages:
+            pkgdir = create_pkgdir(package)
+            generate_pip_package(pkgdir, pypi_repo, package)
+
+    return add_packages