PythonPackage: add import module smoke tests (#20023)

5 files changed, 126 insertions, 128 deletions

diff --git a/lib/spack/docs/build_systems/pythonpackage.rst b/lib/spack/docs/build_systems/pythonpackage.rst
index ef06725e01..de92595efa 100644
--- a/lib/spack/docs/build_systems/pythonpackage.rst
+++ b/lib/spack/docs/build_systems/pythonpackage.rst
@@ -90,7 +90,7 @@ Instead of using the ``PythonPackage`` base class, you should extend
 the ``Package`` base class and implement the following custom installation
 procedure:
 
-.. code-block::
+.. code-block:: python
 
    def install(self, spec, prefix):
        pip = which('pip')
@@ -255,7 +255,7 @@ Many packages are hosted on PyPI, but are developed on GitHub or another
 version control systems. The tarball can be downloaded from either
 location, but PyPI is preferred for the following reasons:
 
-#. PyPI contains the bare minimum of files to install the package.
+#. PyPI contains the bare minimum number of files needed to install the package.
 
    You may notice that the tarball you download from PyPI does not
    have the same checksum as the tarball you download from GitHub.
@@ -292,19 +292,6 @@ location, but PyPI is preferred for the following reasons:
    PyPI is nice because it makes it physically impossible to
    re-release the same version of a package with a different checksum.
 
-There are some reasons to prefer downloading from GitHub:
-
-#. The GitHub tarball may contain unit tests.
-
-   As previously mentioned, the PyPI tarball contains the bare minimum
-   of files to install the package. Unless explicitly specified by the
-   developers, it will not contain development files like unit tests.
-   If you desire to run the unit tests during installation, you should
-   use the GitHub tarball instead.
-
-If you really want to run these unit tests, no one will stop you from
-submitting a PR for a new package that downloads from GitHub.
-
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 Build system dependencies
 ^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -569,7 +556,8 @@ If the package uses ``setuptools``, check for the following clues:
 
   These are packages that are required to run the unit tests for the
   package. These dependencies can be specified using the
-  ``type='test'`` dependency type.
+  ``type='test'`` dependency type. However, the PyPI tarballs rarely
+  contain unit tests, so there is usually no reason to add these.
 
 In the root directory of the package, you may notice a
 ``requirements.txt`` file. It may look like this file contains a list
@@ -625,7 +613,8 @@ add run-time dependencies if they aren't needed, so you need to
 determine whether or not setuptools is needed. Grep the installation
 directory for any files containing a reference to ``setuptools`` or
 ``pkg_resources``. Both modules come from ``py-setuptools``.
-``pkg_resources`` is particularly common in scripts in ``prefix/bin``.
+``pkg_resources`` is particularly common in scripts found in
+``prefix/bin``.
 
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 Passing arguments to setup.py
@@ -699,48 +688,64 @@ a "package" is a directory containing files like:
    foo/baz.py
 
 
-whereas a "module" is a single Python file. Since ``find_packages``
-only returns packages, you'll have to determine the correct module
-names yourself. You can now add these packages and modules to the
-package like so:
+whereas a "module" is a single Python file.
+
+The ``PythonPackage`` base class automatically detects these module
+names for you. If, for whatever reason, the module names detected
+are wrong, you can provide the names yourself by overriding
+``import_modules`` like so:
 
 .. code-block:: python
 
    import_modules = ['six']
 
 
-When you run ``spack install --test=root py-six``, Spack will attempt
-to import the ``six`` module after installation.
+Sometimes the list of module names to import depends on how the
+package was built. For example, the ``py-pyyaml`` package has a
+``+libyaml`` variant that enables the build of a faster optimized
+version of the library. If the user chooses ``~libyaml``, only the
+``yaml`` library will be importable. If the user chooses ``+libyaml``,
+both the ``yaml`` and ``yaml.cyaml`` libraries will be available.
+This can be expressed like so:
+
+.. code-block:: python
+
+   @property
+   def import_modules(self):
+       modules = ['yaml']
+
+       if '+libyaml' in self.spec:
+           modules.append('yaml.cyaml')
 
-These tests most often catch missing dependencies and non-RPATHed
+       return modules
+
+
+These tests often catch missing dependencies and non-RPATHed
 libraries. Make sure not to add modules/packages containing the word
-"test", as these likely won't end up in installation directory.
+"test", as these likely won't end up in the installation directory,
+or may require test dependencies like pytest to be installed.
+
+These tests can be triggered by running ``spack install --test=root``
+or by running ``spack test run`` after the installation has finished.
 
 """"""""""
 Unit tests
 """"""""""
 
 The package you want to install may come with additional unit tests.
-By default, Spack runs:
-
-.. code-block:: console
-
-   $ python setup.py test
-
-
-if it detects that the ``setup.py`` file supports a ``test`` phase.
-You can add additional build-time or install-time tests by overriding
-``test`` or adding a custom install-time test function. For example,
-``py-numpy`` adds:
+You can add additional build-time or install-time tests by adding
+additional testing functions. For example, ``py-numpy`` adds:
 
 .. code-block:: python
 
-   install_time_test_callbacks = ['install_test', 'import_module_test']
-
+   @run_after('install')
+   @on_package_attributes(run_tests=True)
    def install_test(self):
-        with working_dir('..'):
-            python('-c', 'import numpy; numpy.test("full", verbose=2)')
+       with working_dir('spack-test', create=True):
+           python('-c', 'import numpy; numpy.test("full", verbose=2)')
+
 
+These tests can be triggered by running ``spack install --test=root``.
 
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 Setup file in a sub-directory
@@ -781,7 +786,7 @@ PythonPackage vs. packages that use Python
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 There are many packages that make use of Python, but packages that depend
-on Python are not necessarily ``PythonPackages``.
+on Python are not necessarily ``PythonPackage``'s.
 
 """""""""""""""""""""""
 Choosing a build system
@@ -878,8 +883,8 @@ and ``pip`` may be a perfectly valid alternative to using Spack. The
 main advantage of Spack over ``pip`` is its ability to compile
 non-Python dependencies. It can also build cythonized versions of a
 package or link to an optimized BLAS/LAPACK library like MKL,
-resulting in calculations that run orders of magnitude faster.
-Spack does not offer a significant advantage to other python-management
+resulting in calculations that run orders of magnitudes faster.
+Spack does not offer a significant advantage over other python-management
 systems for installing and using tools like flake8 and sphinx.
 But if you need packages with non-Python dependencies like
 numpy and scipy, Spack will be very valuable to you.
diff --git a/lib/spack/docs/build_systems/sippackage.rst b/lib/spack/docs/build_systems/sippackage.rst
index ddf9a26ab9..c636d856d2 100644
--- a/lib/spack/docs/build_systems/sippackage.rst
+++ b/lib/spack/docs/build_systems/sippackage.rst
@@ -93,10 +93,17 @@ in the site-packages directory:
    $ python
    >>> import setuptools
    >>> setuptools.find_packages()
-   ['QtPy5']
+   [
+       'PyQt5', 'PyQt5.QtCore', 'PyQt5.QtGui', 'PyQt5.QtHelp',
+       'PyQt5.QtMultimedia', 'PyQt5.QtMultimediaWidgets', 'PyQt5.QtNetwork',
+       'PyQt5.QtOpenGL', 'PyQt5.QtPrintSupport', 'PyQt5.QtQml',
+       'PyQt5.QtQuick', 'PyQt5.QtSvg', 'PyQt5.QtTest', 'PyQt5.QtWebChannel',
+       'PyQt5.QtWebSockets', 'PyQt5.QtWidgets', 'PyQt5.QtXml',
+       'PyQt5.QtXmlPatterns'
+    ]
 
 
-Large, complex packages like ``QtPy5`` will return a long list of
+Large, complex packages like ``py-pyqt5`` will return a long list of
 packages, while other packages may return an empty list. These packages
 only install a single ``foo.py`` file. In Python packaging lingo,
 a "package" is a directory containing files like:
@@ -108,21 +115,25 @@ a "package" is a directory containing files like:
    foo/baz.py
 
 
-whereas a "module" is a single Python file. Since ``find_packages``
-only returns packages, you'll have to determine the correct module
-names yourself. You can now add these packages and modules to the
-package like so:
+whereas a "module" is a single Python file.
+
+The ``SIPPackage`` base class automatically detects these module
+names for you. If, for whatever reason, the module names detected
+are wrong, you can provide the names yourself by overriding
+``import_modules`` like so:
 
 .. code-block:: python
 
    import_modules = ['PyQt5']
 
 
-When you run ``spack install --test=root py-pyqt5``, Spack will attempt
-to import the ``PyQt5`` module after installation.
+These tests often catch missing dependencies and non-RPATHed
+libraries. Make sure not to add modules/packages containing the word
+"test", as these likely won't end up in the installation directory,
+or may require test dependencies like pytest to be installed.
 
-These tests most often catch missing dependencies and non-RPATHed
-libraries.
+These tests can be triggered by running ``spack install --test=root``
+or by running ``spack test run`` after the installation has finished.
 
 ^^^^^^^^^^^^^^^^^^^^^^
 External documentation
diff --git a/lib/spack/spack/build_systems/python.py b/lib/spack/spack/build_systems/python.py
index 76159d88a1..87bc7e5135 100644
--- a/lib/spack/spack/build_systems/python.py
+++ b/lib/spack/spack/build_systems/python.py
@@ -10,8 +10,9 @@ from spack.directives import depends_on, extends
 from spack.package import PackageBase, run_after
 
 from llnl.util.filesystem import (working_dir, get_filetype, filter_file,
-                                  path_contains_subdirectory, same_path)
+                                  path_contains_subdirectory, same_path, find)
 from llnl.util.lang import match_predicate
+import llnl.util.tty as tty
 
 
 class PythonPackage(PackageBase):
@@ -74,25 +75,12 @@ class PythonPackage(PackageBase):
     # Default phases
     phases = ['build', 'install']
 
-    # Name of modules that the Python package provides
-    # This is used to test whether or not the installation succeeded
-    # These names generally come from running:
-    #
-    # >>> import setuptools
-    # >>> setuptools.find_packages()
-    #
-    # in the source tarball directory
-    import_modules = []
-
     # To be used in UI queries that require to know which
     # build-system class we are using
     build_system_class = 'PythonPackage'
 
-    #: Callback names for build-time test
-    build_time_test_callbacks = ['build_test']
-
     #: Callback names for install-time test
-    install_time_test_callbacks = ['import_module_test']
+    install_time_test_callbacks = ['test']
 
     extends('python')
 
@@ -100,6 +88,45 @@ class PythonPackage(PackageBase):
 
     py_namespace = None
 
+    @property
+    def import_modules(self):
+        """Names of modules that the Python package provides.
+
+        These are used to test whether or not the installation succeeded.
+        These names generally come from running:
+
+        .. code-block:: python
+
+           >> import setuptools
+           >> setuptools.find_packages()
+
+        in the source tarball directory. If the module names are incorrectly
+        detected, this property can be overridden by the package.
+
+        Returns:
+            list: list of strings of module names
+        """
+        modules = []
+
+        # Python libraries may be installed in lib or lib64
+        # See issues #18520 and #17126
+        for lib in ['lib', 'lib64']:
+            root = os.path.join(self.prefix, lib, 'python{0}'.format(
+                self.spec['python'].version.up_to(2)), 'site-packages')
+            # Some Python libraries are packages: collections of modules
+            # distributed in directories containing __init__.py files
+            for path in find(root, '__init__.py', recursive=True):
+                modules.append(path.replace(root + os.sep, '', 1).replace(
+                    os.sep + '__init__.py', '').replace('/', '.'))
+            # Some Python libraries are modules: individual *.py files
+            # found in the site-packages directory
+            for path in find(root, '*.py', recursive=False):
+                modules.append(path.replace(root + os.sep, '', 1).replace(
+                    '.py', '').replace('/', '.'))
+
+        tty.debug('Detected the following modules: {0}'.format(modules))
+        return modules
+
     def setup_file(self):
         """Returns the name of the setup file to use."""
         return 'setup.py'
@@ -118,27 +145,6 @@ class PythonPackage(PackageBase):
         with working_dir(self.build_directory):
             self.python('-s', setup, '--no-user-cfg', *args, **kwargs)
 
-    def _setup_command_available(self, command):
-        """Determines whether or not a setup.py command exists.
-
-        Args:
-            command (str): The command to look for
-
-        Returns:
-            bool: True if the command is found, else False
-        """
-        kwargs = {
-            'output': os.devnull,
-            'error':  os.devnull,
-            'fail_on_error': False
-        }
-
-        python = inspect.getmodule(self).python
-        setup = self.setup_file()
-
-        python('-s', setup, '--no-user-cfg', command, '--help', **kwargs)
-        return python.returncode == 0
-
     # The following phases and their descriptions come from:
     #   $ python setup.py --help-commands
 
@@ -359,33 +365,16 @@ class PythonPackage(PackageBase):
 
     # Testing
 
-    def build_test(self):
-        """Run unit tests after in-place build.
-
-        These tests are only run if the package actually has a 'test' command.
-        """
-        if self._setup_command_available('test'):
-            args = self.test_args(self.spec, self.prefix)
-
-            self.setup_py('test', *args)
-
-    def test_args(self, spec, prefix):
-        """Arguments to pass to test."""
-        return []
-
-    run_after('build')(PackageBase._run_default_build_time_test_callbacks)
-
-    def import_module_test(self):
-        """Attempts to import the module that was just installed.
-
-        This test is only run if the package overrides
-        :py:attr:`import_modules` with a list of module names."""
+    def test(self):
+        """Attempts to import modules of the installed package."""
 
         # Make sure we are importing the installed modules,
-        # not the ones in the current directory
-        with working_dir('spack-test', create=True):
-            for module in self.import_modules:
-                self.python('-c', 'import {0}'.format(module))
+        # not the ones in the source directory
+        for module in self.import_modules:
+            self.run_test(inspect.getmodule(self).python.path,
+                          ['-c', 'import {0}'.format(module)],
+                          purpose='checking import of {0}'.format(module),
+                          work_dir='spack-test')
 
     run_after('install')(PackageBase._run_default_install_time_test_callbacks)
 
diff --git a/lib/spack/spack/build_systems/sip.py b/lib/spack/spack/build_systems/sip.py
index f814ef1837..e4c283881d 100644
--- a/lib/spack/spack/build_systems/sip.py
+++ b/lib/spack/spack/build_systems/sip.py
@@ -4,11 +4,12 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import inspect
+import os
 
 from llnl.util.filesystem import working_dir, join_path
+from spack.build_systems.python import PythonPackage
 from spack.directives import depends_on, extends
 from spack.package import PackageBase, run_after
-import os
 
 
 class SIPPackage(PackageBase):
@@ -36,13 +37,15 @@ class SIPPackage(PackageBase):
     sip_module = 'sip'
 
     #: Callback names for install-time test
-    install_time_test_callbacks = ['import_module_test']
+    install_time_test_callbacks = ['test']
 
     extends('python')
 
     depends_on('qt')
     depends_on('py-sip')
 
+    import_modules = PythonPackage.import_modules
+
     def python(self, *args, **kwargs):
         """The python ``Executable``."""
         inspect.getmodule(self).python(*args, **kwargs)
@@ -98,17 +101,7 @@ class SIPPackage(PackageBase):
 
     # Testing
 
-    def import_module_test(self):
-        """Attempts to import the module that was just installed.
-
-        This test is only run if the package overrides
-        :py:attr:`import_modules` with a list of module names."""
-
-        # Make sure we are importing the installed modules,
-        # not the ones in the current directory
-        with working_dir('spack-test', create=True):
-            for module in self.import_modules:
-                self.python('-c', 'import {0}'.format(module))
+    test = PythonPackage.test
 
     run_after('install')(PackageBase._run_default_install_time_test_callbacks)
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index de394e2d45..531f4aadee 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1761,7 +1761,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
             work_dir (str or None): path to the smoke test directory
         """
         wdir = '.' if work_dir is None else work_dir
-        with fsys.working_dir(wdir):
+        with fsys.working_dir(wdir, create=True):
             try:
                 runner = which(exe)
                 if runner is None and skip_missing: