documentation: build-system phases + build-time tests (#2780)

* documentation: reworked packaging guide to add build-system phases

* documentation: improvements to AutotoolsPackage autodocs

* build_systems: updated autodocs

* run-tests: added a few information on how to run tests fixes #2606 fixes#2605

* documentation: fixed items brought up by @davydden

    * typos in docs
    * consistent use of 'build system' (i.e. removed 'build-system' from docs)
    * added a note on possible default implementations for build-time tests

* documentation: fixed items brought up by @citibeth

    * added note to explain the difference between build system and language used in a package
    * capitalized bullet items
    * added link to API docs

* documentation: fixed multiple cross-references after rebase

* documentation: fixed minor issues raised by @tgamblin

* documentation: added entry in table for the `PythonPackage` class

* docs: fixed issues brought up by @citybeth in the second review

6 files changed, 332 insertions, 114 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index b09c677e0b..f3927a0709 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -1999,41 +1999,122 @@ the Python extensions provided by them: once for ``+python`` and once
 for ``~python``.  Other than using a little extra disk space, that
 solution has no serious problems.
 
------------------------------------
-Implementing the ``install`` method
------------------------------------
+.. _installation_procedure:
 
-The last element of a package is its ``install()`` method.  This is
+---------------------------------------
+Implementing the installation procedure
+---------------------------------------
+
+The last element of a package is its **installation procedure**.  This is
 where the real work of installation happens, and it's the main part of
 the package you'll need to customize for each piece of software.
 
-.. code-block:: python
-   :linenos:
+Defining an installation procedure means overriding a set of methods or attributes
+that will be called at some point during the installation of the package.
+The package base class, usually specialized for a given build system, determines the
+actual set of entities available for overriding.
+The classes that are currently provided by Spack are:
+
+    +------------------------------------+----------------------------------+
+    |                                    |   **Base class purpose**         |
+    +====================================+==================================+
+    |          :py:class:`.Package`      | General base class not           |
+    |                                    | specialized for any build system |
+    +------------------------------------+----------------------------------+
+    |   :py:class:`.MakefilePackage`     | Specialized class for packages   |
+    |                                    | built invoking                   |
+    |                                    | hand-written Makefiles           |
+    +------------------------------------+----------------------------------+
+    |   :py:class:`.AutotoolsPackage`    | Specialized class for packages   |
+    |                                    | built using GNU Autotools        |
+    +------------------------------------+----------------------------------+
+    |  :py:class:`.CMakePackage`         | Specialized class for packages   |
+    |                                    | built using CMake                |
+    +------------------------------------+----------------------------------+
+    |  :py:class:`.RPackage`             | Specialized class for            |
+    |                                    | :py:class:`.R` extensions        |
+    +------------------------------------+----------------------------------+
+    |  :py:class:`.PythonPackage`        | Specialized class for            |
+    |                                    | :py:class:`.Python` extensions   |
+    +------------------------------------+----------------------------------+
 
-   def install(self, spec prefix):
-       configure('--prefix={0}'.format(prefix))
 
-       make()
-       make('install')
 
-``install`` takes a ``spec``: a description of how the package should
-be built, and a ``prefix``: the path to the directory where the
-software should be installed.
+.. note::
+    Choice of the appropriate base class for a package
+        In most cases packagers don't have to worry about the selection of the right base class
+        for a package, as ``spack create`` will make the appropriate choice on their behalf. In those
+        rare cases where manual intervention is needed we need to stress that a
+        package base class depends on the *build system* being used, not the language of the package.
+        For example, a Python extension installed with CMake would ``extends('python')`` and
+        subclass from :py:class:`.CMakePackage`.
+
+^^^^^^^^^^^^^^^^^^^^^
+Installation pipeline
+^^^^^^^^^^^^^^^^^^^^^
+
+When a user runs ``spack install``, Spack:
+
+1. Fetches an archive for the correct version of the software.
+2. Expands the archive.
+3. Sets the current working directory to the root directory of the expanded archive.
+
+Then, depending on the base class of the package under consideration, it will execute
+a certain number of **phases** that reflect the way a package of that type is usually built.
+The name and order in which the phases will be executed can be obtained either reading the API
+docs at :py:mod:`~.spack.build_systems`, or using the ``spack info`` command:
+
+.. code-block:: console
+    :emphasize-lines: 13,14
+
+    $ spack info m4
+    AutotoolsPackage:    m4
+    Homepage:            https://www.gnu.org/software/m4/m4.html
 
-Spack provides wrapper functions for ``configure`` and ``make`` so
-that you can call them in a similar way to how you'd call a shell
-command.  In reality, these are Python functions.  Spack provides
-these functions to make writing packages more natural. See the section
-on :ref:`shell wrappers <shell-wrappers>`.
+    Safe versions:
+        1.4.17    ftp://ftp.gnu.org/gnu/m4/m4-1.4.17.tar.gz
 
-Now that the metadata is out of the way, we can move on to the
-``install()`` method.  When a user runs ``spack install``, Spack
-fetches an archive for the correct version of the software, expands
-the archive, and sets the current working directory to the root
-directory of the expanded archive.  It then instantiates a package
-object and calls the ``install()`` method.
+    Variants:
+        Name       Default   Description
+
+        sigsegv    on        Build the libsigsegv dependency
+
+    Installation Phases:
+        autoreconf    configure    build    install
+
+    Build Dependencies:
+        libsigsegv
+
+    ...
+
+
+Typically, phases have default implementations that fit most of the common cases:
+
+.. literalinclude:: ../../../lib/spack/spack/build_systems/autotools.py
+    :pyobject: AutotoolsPackage.configure
+    :linenos:
+
+It is thus just sufficient for a packager to override a few
+build system specific helper methods or attributes to provide, for instance,
+configure arguments:
+
+.. literalinclude::  ../../../var/spack/repos/builtin/packages/m4/package.py
+    :pyobject: M4.configure_args
+    :linenos:
+
+.. note::
+    Each specific build system has a list of attributes that can be overridden to
+    fine-tune the installation of a package without overriding an entire phase. To
+    have more information on them the place to go is the API docs of the :py:mod:`~.spack.build_systems`
+    module.
+
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Overriding an entire phase
+^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-The ``install()`` signature looks like this:
+In extreme cases it may be necessary to override an entire phase. Regardless
+of the build system, the signature is the same. For example, the signature
+for the install phase is:
 
 .. code-block:: python
 
@@ -2041,8 +2122,6 @@ The ``install()`` signature looks like this:
        def install(self, spec, prefix):
            ...
 
-The parameters are as follows:
-
 ``self``
     For those not used to Python instance methods, this is the
     package itself.  In this case it's an instance of ``Foo``, which
@@ -2059,19 +2138,15 @@ The parameters are as follows:
     targets into.  It acts like a string, but it's actually its own
     special type, :py:class:`Prefix <spack.util.prefix.Prefix>`.
 
-``spec`` and ``prefix`` are passed to ``install`` for convenience.
-``spec`` is also available as an attribute on the package
-(``self.spec``), and ``prefix`` is actually an attribute of ``spec``
-(``spec.prefix``).
+The arguments ``spec`` and ``prefix`` are passed only for convenience, as they always
+correspond to ``self.spec`` and ``self.spec.prefix`` respectively.
 
-As mentioned in :ref:`install-environment`, you will usually not need
-to refer to dependencies explicitly in your package file, as the
-compiler wrappers take care of most of the heavy lifting here.  There
-will be times, though, when you need to refer to the install locations
-of dependencies, or when you need to do something different depending
-on the version, compiler, dependencies, etc. that your package is
-built with.  These parameters give you access to this type of
-information.
+As mentioned in :ref:`install-environment`, you will usually not need to refer
+to dependencies explicitly in your package file, as the compiler wrappers take care of most of
+the heavy lifting here.  There will be times, though, when you need to refer to
+the install locations of dependencies, or when you need to do something different
+depending on the version, compiler, dependencies, etc. that your package is
+built with.  These parameters give you access to this type of information.
 
 .. _install-environment:
 
@@ -2629,9 +2704,9 @@ build system.
 
 .. _sanity-checks:
 
--------------------------------
-Sanity checking an installation
--------------------------------
+------------------------
+Checking an installation
+------------------------
 
 By default, Spack assumes that a build has failed if nothing is
 written to the install prefix, and that it has succeeded if anything
@@ -2650,16 +2725,18 @@ Consider a simple autotools build like this:
 If you are using using standard autotools or CMake, ``configure`` and
 ``make`` will not write anything to the install prefix.  Only ``make
 install`` writes the files, and only once the build is already
-complete.  Not all builds are like this.  Many builds of scientific
-software modify the install prefix *before* ``make install``. Builds
-like this can falsely report that they were successfully installed if
-an error occurs before the install is complete but after files have
-been written to the ``prefix``.
+complete.
 
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 ``sanity_check_is_file`` and ``sanity_check_is_dir``
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+Unfortunately, many builds of scientific
+software modify the install prefix *before* ``make install``. Builds
+like this can falsely report that they were successfully installed if
+an error occurs before the install is complete but after files have
+been written to the ``prefix``.
+
 You can optionally specify *sanity checks* to deal with this problem.
 Add properties like this to your package:
 
@@ -2683,6 +2760,48 @@ the build will fail and the install prefix will be removed.  If they
 succeed, Spack considers the build successful and keeps the prefix in
 place.
 
+^^^^^^^^^^^^^^^^
+Build-time tests
+^^^^^^^^^^^^^^^^
+
+Sometimes packages finish to build "correctly" and issues with their run-time
+behavior are discovered only at a later stage, maybe after a full software stack
+relying on them has already been built. To avoid situations of that kind it's possible
+to write build-time tests that will be executed only if the option ``--run-tests``
+of ``spack install`` has been activated.
+
+The proper way to write these tests is relying on two decorators that come with
+any base class listed in :ref:`installation_procedure`.
+
+.. code-block:: python
+
+   @MakefilePackage.sanity_check('build')
+   @MakefilePackage.on_package_attributes(run_tests=True)
+   def check_build(self):
+        # Custom implementation goes here
+        pass
+
+The first decorator ``MakefilePackage.sanity_check('build')`` schedules this
+function to be invoked after the ``build`` phase has been executed, while the
+second one makes the invocation  conditional on the fact that ``self.run_tests == True``.
+It is also possible to schedule a function to be invoked *before* a given phase
+using the ``MakefilePackage.precondition`` decorator.
+
+.. note::
+
+    Default implementations for build-time tests
+
+        Packages that are built using specific build systems may already have a
+        default implementation for build-time tests. For instance :py:class:`~.AutotoolsPackage`
+        based packages will try to invoke ``make test`` and ``make check`` if
+        Spack is asked to run tests.
+        More information on each class is available in the the :py:mod:`~.spack.build_systems`
+        documentation.
+
+.. warning::
+
+    The API for adding tests is not yet considered stable and may change drastically in future releases.
+
 .. _file-manipulation:
 
 ---------------------------
diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py
index 78a4df5e11..37c780b360 100644
--- a/lib/spack/spack/build_systems/autotools.py
+++ b/lib/spack/spack/build_systems/autotools.py
@@ -36,31 +36,51 @@ from spack.package import PackageBase
 
 
 class AutotoolsPackage(PackageBase):
-    """Specialized class for packages that are built using GNU Autotools
+    """Specialized class for packages built using GNU Autotools.
 
     This class provides four phases that can be overridden:
 
-    * autoreconf
-    * configure
-    * build
-    * install
+        1. :py:meth:`~.AutotoolsPackage.autoreconf`
+        2. :py:meth:`~.AutotoolsPackage.configure`
+        3. :py:meth:`~.AutotoolsPackage.build`
+        4. :py:meth:`~.AutotoolsPackage.install`
 
     They all have sensible defaults and for many packages the only thing
-    necessary will be to override ``configure_args``
+    necessary will be to override the helper method :py:meth:`.configure_args`.
+    For a finer tuning you may also override:
+
+        +-----------------------------------------------+--------------------+
+        | **Method**                                    | **Purpose**        |
+        +===============================================+====================+
+        | :py:attr:`~.AutotoolsPackage.build_targets`   | Specify ``make``   |
+        |                                               | targets for the    |
+        |                                               | build phase        |
+        +-----------------------------------------------+--------------------+
+        | :py:attr:`~.AutotoolsPackage.install_targets` | Specify ``make``   |
+        |                                               | targets for the    |
+        |                                               | install phase      |
+        +-----------------------------------------------+--------------------+
+        | :py:meth:`~.AutotoolsPackage.check`           | Run  build time    |
+        |                                               | tests if required  |
+        +-----------------------------------------------+--------------------+
 
-    Additionally, you may specify make targets for build and install
-    phases by overriding ``build_targets`` and ``install_targets``
     """
+    #: Phases of a GNU Autotools package
     phases = ['autoreconf', 'configure', 'build', 'install']
-    # To be used in UI queries that require to know which
-    # build-system class we are using
+    #: This attribute is used in UI queries that need to know the build
+    #: system base class
     build_system_class = 'AutotoolsPackage'
+    #: Whether or not to update ``config.guess`` on old architectures
     patch_config_guess = True
 
+    #: Targets for ``make`` during the :py:meth:`~.AutotoolsPackage.build`
+    #: phase
     build_targets = []
+    #: Targets for ``make`` during the :py:meth:`~.AutotoolsPackage.install`
+    #: phase
     install_targets = ['install']
 
-    def do_patch_config_guess(self):
+    def _do_patch_config_guess(self):
         """Some packages ship with an older config.guess and need to have
         this updated when installed on a newer architecture."""
 
@@ -86,7 +106,7 @@ class AutotoolsPackage(PackageBase):
                 check_call([my_config_guess], stdout=PIPE, stderr=PIPE)
                 # The package's config.guess already runs OK, so just use it
                 return True
-            except:
+            except Exception:
                 pass
         else:
             return True
@@ -104,7 +124,7 @@ class AutotoolsPackage(PackageBase):
                 check_call([config_guess], stdout=PIPE, stderr=PIPE)
                 shutil.copyfile(config_guess, my_config_guess)
                 return True
-            except:
+            except Exception:
                 pass
 
         # Look for the system's config.guess
@@ -121,7 +141,7 @@ class AutotoolsPackage(PackageBase):
                 check_call([config_guess], stdout=PIPE, stderr=PIPE)
                 shutil.copyfile(config_guess, my_config_guess)
                 return True
-            except:
+            except Exception:
                 pass
 
         return False
@@ -131,11 +151,17 @@ class AutotoolsPackage(PackageBase):
         return self.stage.source_path
 
     def patch(self):
-        """Perform any required patches."""
+        """Patches config.guess if
+        :py:attr:``~.AutotoolsPackage.patch_config_guess`` is True
+
+        :raise RuntimeError: if something goes wrong when patching
+            ``config.guess``
+        """
 
         if self.patch_config_guess and self.spec.satisfies(
-                'arch=linux-rhel7-ppc64le'):
-            if not self.do_patch_config_guess():
+                'arch=linux-rhel7-ppc64le'
+        ):
+            if not self._do_patch_config_guess():
                 raise RuntimeError('Failed to find suitable config.guess')
 
     def autoreconf(self, spec, prefix):
@@ -144,22 +170,27 @@ class AutotoolsPackage(PackageBase):
 
     @PackageBase.sanity_check('autoreconf')
     def is_configure_or_die(self):
-        """Checks the presence of a ``configure`` file after the
-        autoreconf phase"""
+        """Checks the presence of a `configure` file after the
+        :py:meth:`.autoreconf` phase.
+
+        :raise RuntimeError: if the ``configure`` script does not exist.
+        """
         with working_dir(self.build_directory()):
             if not os.path.exists('configure'):
                 raise RuntimeError(
                     'configure script not found in {0}'.format(os.getcwd()))
 
     def configure_args(self):
-        """Method to be overridden. Should return an iterable containing
-        all the arguments that must be passed to configure, except ``--prefix``
+        """Produces a list containing all the arguments that must be passed to
+        configure, except ``--prefix`` which will be pre-pended to the list.
+
+        :return: list of arguments for configure
         """
         return []
 
     def configure(self, spec, prefix):
-        """Runs configure with the arguments specified in ``configure_args``
-        and an appropriately set prefix
+        """Runs configure with the arguments specified in :py:meth:`.configure_args`
+        and an appropriately set prefix.
         """
         options = ['--prefix={0}'.format(prefix)] + self.configure_args()
 
@@ -167,12 +198,16 @@ class AutotoolsPackage(PackageBase):
             inspect.getmodule(self).configure(*options)
 
     def build(self, spec, prefix):
-        """Make the build targets"""
+        """Makes the build targets specified by
+        :py:attr:``~.AutotoolsPackage.build_targets``
+        """
         with working_dir(self.build_directory()):
             inspect.getmodule(self).make(*self.build_targets)
 
     def install(self, spec, prefix):
-        """Make the install targets"""
+        """Makes the install targets specified by
+        :py:attr:``~.AutotoolsPackage.install_targets``
+        """
         with working_dir(self.build_directory()):
             inspect.getmodule(self).make(*self.install_targets)
 
@@ -181,8 +216,8 @@ class AutotoolsPackage(PackageBase):
     def _run_default_function(self):
         """This function is run after build if ``self.run_tests == True``
 
-        It will search for a method named ``check`` and run it. A sensible
-        default is provided in the base class.
+        It will search for a method named :py:meth:`.check` and run it. A
+        sensible default is provided in the base class.
         """
         try:
             fn = getattr(self, 'check')
@@ -192,8 +227,8 @@ class AutotoolsPackage(PackageBase):
             tty.msg('Skipping default sanity checks [method `check` not implemented]')  # NOQA: ignore=E501
 
     def check(self):
-        """Default test: search the Makefile for targets ``test`` and ``check``
-        and run them if found.
+        """Searches the Makefile for targets ``test`` and ``check``
+        and runs them if found.
         """
         with working_dir(self.build_directory()):
             self._if_make_target_execute('test')
diff --git a/lib/spack/spack/build_systems/cmake.py b/lib/spack/spack/build_systems/cmake.py
index 61d45784e8..a5e23e54f4 100644
--- a/lib/spack/spack/build_systems/cmake.py
+++ b/lib/spack/spack/build_systems/cmake.py
@@ -34,23 +34,39 @@ from spack.package import PackageBase
 
 
 class CMakePackage(PackageBase):
-    """Specialized class for packages that are built using CMake
+    """Specialized class for packages built using CMake
 
     This class provides three phases that can be overridden:
 
-    * cmake
-    * build
-    * install
+        1. :py:meth:`~.CMakePackage.cmake`
+        2. :py:meth:`~.CMakePackage.build`
+        3. :py:meth:`~.CMakePackage.install`
 
     They all have sensible defaults and for many packages the only thing
-    necessary will be to override ``cmake_args``
+    necessary will be to override :py:meth:`~.CMakePackage.cmake_args`.
+    For a finer tuning you may also override:
+
+        +-----------------------------------------------+--------------------+
+        | **Method**                                    | **Purpose**        |
+        +===============================================+====================+
+        | :py:meth:`~.CMakePackage.build_type`          | Specify the value  |
+        |                                               | for the            |
+        |                                               | CMAKE_BUILD_TYPE   |
+        |                                               | variable           |
+        +-----------------------------------------------+--------------------+
+        | :py:meth:`~.CMakePackage.root_cmakelists_dir` | Location of the    |
+        |                                               | root CMakeLists.txt|
+        +-----------------------------------------------+--------------------+
+        | :py:meth:`~.CMakePackage.build_directory`     | Directory where to |
+        |                                               | build the package  |
+        +-----------------------------------------------+--------------------+
+
 
-    Additionally, you may specify make targets for build and install
-    phases by overriding ``build_targets`` and ``install_targets``
     """
+    #: Phases of a CMake package
     phases = ['cmake', 'build', 'install']
-    # To be used in UI queries that require to know which
-    # build-system class we are using
+    #: This attribute is used in UI queries that need to know the build
+    #: system base class
     build_system_class = 'CMakePackage'
 
     build_targets = []
@@ -59,19 +75,25 @@ class CMakePackage(PackageBase):
     depends_on('cmake', type='build')
 
     def build_type(self):
-        """Override to provide the correct build_type in case a complex
-        logic is needed
+        """Returns the correct value for the ``CMAKE_BUILD_TYPE`` variable
+
+        :return: value for ``CMAKE_BUILD_TYPE``
         """
         return 'RelWithDebInfo'
 
     def root_cmakelists_dir(self):
-        """Directory where to find the root CMakeLists.txt"""
+        """Returns the location of the root CMakeLists.txt
+
+        :return: directory containing the root CMakeLists.txt
+        """
         return self.stage.source_path
 
     @property
     def std_cmake_args(self):
         """Standard cmake arguments provided as a property for
         convenience of package writers
+
+        :return: standard cmake arguments
         """
         # standard CMake arguments
         return CMakePackage._std_args(self)
@@ -97,20 +119,27 @@ class CMakePackage(PackageBase):
         return args
 
     def build_directory(self):
-        """Override to provide another place to build the package"""
+        """Returns the directory to use when building the package
+
+        :return: directory where to build the package
+        """
         return join_path(self.stage.source_path, 'spack-build')
 
     def cmake_args(self):
-        """Method to be overridden. Should return an iterable containing
-        all the arguments that must be passed to configure, except:
+        """Produces a list containing all the arguments that must be passed to
+        cmake, except:
+
+            * CMAKE_INSTALL_PREFIX
+            * CMAKE_BUILD_TYPE
+
+        which will be set automatically.
 
-        * CMAKE_INSTALL_PREFIX
-        * CMAKE_BUILD_TYPE
+        :return: list of arguments for cmake
         """
         return []
 
     def cmake(self, spec, prefix):
-        """Run cmake in the build directory"""
+        """Runs ``cmake`` in the build directory"""
         options = [self.root_cmakelists_dir()] + self.std_cmake_args + \
             self.cmake_args()
         with working_dir(self.build_directory(), create=True):
@@ -142,8 +171,8 @@ class CMakePackage(PackageBase):
             tty.msg('Skipping default build sanity checks [method `check` not implemented]')  # NOQA: ignore=E501
 
     def check(self):
-        """Default test: search the Makefile for the target ``test``
-        and run them if found.
+        """Searches the CMake-generated Makefile for the target ``test``
+        and runs it if found.
         """
         with working_dir(self.build_directory()):
             self._if_make_target_execute('test')
diff --git a/lib/spack/spack/build_systems/makefile.py b/lib/spack/spack/build_systems/makefile.py
index a56f316109..e8fa86264b 100644
--- a/lib/spack/spack/build_systems/makefile.py
+++ b/lib/spack/spack/build_systems/makefile.py
@@ -35,36 +35,67 @@ class MakefilePackage(PackageBase):
 
     This class provides three phases that can be overridden:
 
-    * edit
-    * build
-    * install
+        1. :py:meth:`~.MakefilePackage.edit`
+        2. :py:meth:`~.MakefilePackage.build`
+        3. :py:meth:`~.MakefilePackage.install`
 
-    It is necessary to override the 'edit' phase, while 'build' and 'install'
-    have sensible defaults.
+    It is usually necessary to override the :py:meth:`~.MakefilePackage.edit`
+    phase, while :py:meth:`~.MakefilePackage.build` and
+    :py:meth:`~.MakefilePackage.install` have sensible defaults.
+    For a finer tuning you may override:
+
+        +-----------------------------------------------+--------------------+
+        | **Method**                                    | **Purpose**        |
+        +===============================================+====================+
+        | :py:attr:`~.MakefilePackage.build_targets`    | Specify ``make``   |
+        |                                               | targets for the    |
+        |                                               | build phase        |
+        +-----------------------------------------------+--------------------+
+        | :py:attr:`~.MakefilePackage.install_targets`  | Specify ``make``   |
+        |                                               | targets for the    |
+        |                                               | install phase      |
+        +-----------------------------------------------+--------------------+
+        | :py:meth:`~.MakefilePackage.build_directory`  | Directory where the|
+        |                                               | Makefile is located|
+        +-----------------------------------------------+--------------------+
     """
+    #: Phases of a package that is built with an hand-written Makefile
     phases = ['edit', 'build', 'install']
-    # To be used in UI queries that require to know which
-    # build-system class we are using
+    #: This attribute is used in UI queries that need to know the build
+    #: system base class
     build_system_class = 'MakefilePackage'
 
+    #: Targets for ``make`` during the :py:meth:`~.MakefilePackage.build`
+    #: phase
     build_targets = []
+    #: Targets for ``make`` during the :py:meth:`~.MakefilePackage.install`
+    #: phase
     install_targets = ['install']
 
     def build_directory(self):
-        """Directory where the main Makefile is located"""
+        """Returns the directory containing the main Makefile
+
+        :return: build directory
+        """
         return self.stage.source_path
 
     def edit(self, spec, prefix):
-        """This phase cannot be defaulted for obvious reasons..."""
+        """Edits the Makefile before calling make. This phase cannot
+        be defaulted.
+        """
         tty.msg('Using default implementation: skipping edit phase.')
 
     def build(self, spec, prefix):
-        """Make the build targets"""
+        """Calls make, passing :py:attr:`~.MakefilePackage.build_targets`
+        as targets.
+        """
         with working_dir(self.build_directory()):
             inspect.getmodule(self).make(*self.build_targets)
 
     def install(self, spec, prefix):
-        """Make the install targets"""
+        """Calls make, passing :py:attr:`~.MakefilePackage.install_targets`
+        as targets.
+        """
         with working_dir(self.build_directory()):
             inspect.getmodule(self).make(*self.install_targets)
 
diff --git a/lib/spack/spack/build_systems/r.py b/lib/spack/spack/build_systems/r.py
index f642f2dfd8..a4f7359ec8 100644
--- a/lib/spack/spack/build_systems/r.py
+++ b/lib/spack/spack/build_systems/r.py
@@ -34,21 +34,21 @@ class RPackage(PackageBase):
 
     This class provides a single phase that can be overridden:
 
-    * install
+        1. :py:meth:`~.RPackage.install`
 
-    It has sensible defaults and for many packages the only thing
+    It has sensible defaults, and for many packages the only thing
     necessary will be to add dependencies
     """
     phases = ['install']
 
-    # To be used in UI queries that require to know which
-    # build-system class we are using
+    #: This attribute is used in UI queries that need to know the build
+    #: system base class
     build_system_class = 'RPackage'
 
     extends('r')
 
     def install(self, spec, prefix):
-        """Install the R package"""
+        """Installs an R package."""
         inspect.getmodule(self).R(
             'CMD', 'INSTALL',
             '--library={0}'.format(self.module.r_lib_dir),
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index f9bc1fafbc..24ff82fa38 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1706,9 +1706,13 @@ class PackageBase(object):
 
 
 class Package(PackageBase):
+    """General purpose class with a single ``install``
+    phase that needs to be coded by packagers.
+    """
+    #: The one and only phase
     phases = ['install']
-    # To be used in UI queries that require to know which
-    # build-system class we are using
+    #: This attribute is used in UI queries that require to know which
+    #: build-system class we are using
     build_system_class = 'Package'
     # This will be used as a registration decorator in user
     # packages, if need be