Major updates to Contribution Guide (#1968)

* Major updates to Contribution Guide

* Grammar changes

* Fix missing/extra backticks

* Rewording, links, and tips added

1 files changed, 401 insertions, 127 deletions

diff --git a/lib/spack/docs/contribution_guide.rst b/lib/spack/docs/contribution_guide.rst
index 108b23a6d0..ebfab854d5 100644
--- a/lib/spack/docs/contribution_guide.rst
+++ b/lib/spack/docs/contribution_guide.rst
@@ -1,247 +1,521 @@
 .. _contribution-guide:
 
+==================
 Contribution Guide
-=====================
+==================
 
 This guide is intended for developers or administrators who want to
 contribute a new package, feature, or bugfix to Spack.
 It assumes that you have at least some familiarity with Git VCS and Github.
-The guide will show a few examples of contributing workflow and discuss
-the granularity of pull-requests (PRs).
+The guide will show a few examples of contributing workflows and discuss
+the granularity of pull-requests (PRs). It will also discuss the tests your
+PR must pass in order to be accepted into Spack.
 
 First, what is a PR? Quoting `Bitbucket's tutorials <https://www.atlassian.com/git/tutorials/making-a-pull-request/>`_:
 
-  Pull requests are a mechanism for a developer to notify team members that they have **completed a feature**.
-  The pull request is more than just a notification—it’s a dedicated forum for discussing the proposed feature
+  Pull requests are a mechanism for a developer to notify team members that
+  they have **completed a feature**. The pull request is more than just a
+  notification—it’s a dedicated forum for discussing the proposed feature.
 
-Important is completed feature, i.e. the changes one propose in a PR should
+Important is **completed feature**. The changes one proposes in a PR should
 correspond to one feature/bugfix/extension/etc. One can create PRs with
 changes relevant to different ideas, however reviewing such PRs becomes tedious
-and error prone. If possible, try to follow the rule **one-PR-one-package/feature.**
+and error prone. If possible, try to follow the **one-PR-one-package/feature** rule.
 
-Spack uses a rough approximation of the `Git Flow <http://nvie.com/posts/a-successful-git-branching-model/>`_ branching
-model. The develop branch contains the latest contributions, and master is
-always tagged and points to the latest stable release. Thereby when you send
-your request, make ``develop`` the destination branch on the
+Spack uses a rough approximation of the `Git Flow <http://nvie.com/posts/a-successful-git-branching-model/>`_
+branching model. The develop branch contains the latest contributions, and
+master is always tagged and points to the latest stable release. Therefore, when
+you send your request, make ``develop`` the destination branch on the
 `Spack repository <https://github.com/LLNL/spack>`_.
 
-Let's assume that the current (patched) state of your fork of Spack is only
-relevant to yourself. Now you come across a bug in a package or would like to
-extend a package and contribute this fix to Spack. It is important that
-whenever you change something that might be of importance upstream,
-create a pull-request (PR) as soon as possible. Do not wait for weeks/months to
-do this: a) you might forget why did you modified certain files; b) it could get
-difficult to isolate this change into a stand-alone clean PR.
+----------------------
+Continuous Integration
+----------------------
 
-Now let us discuss several approaches one may use to submit a PR while
-also keeping your local version of Spack patched.
+Spack uses `Travis CI <https://travis-ci.org/LLNL/spack>`_ for Continuous Integration
+testing. This means that every time you submit a pull request, a series of tests will
+be run to make sure you didn't accidentally introduce any bugs into Spack. Your PR
+will not be accepted until it passes all of these tests. While you can certainly wait
+for the results of these tests after submitting a PR, we recommend that you run them
+locally to speed up the review process.
 
+If you take a look in ``$SPACK_ROOT/.travis.yml``, you'll notice that we test
+against Python 2.6 and 2.7. We currently perform 3 types of tests:
 
-First approach (cherry-picking):
---------------------------------
+^^^^^^^^^^
+Unit Tests
+^^^^^^^^^^
 
-First approach is as follows.
-You checkout your local develop branch, which for the purpose of this guide
-will be called ``develop_modified``:
+Unit tests ensure that core Spack features like fetching or spec resolution are
+working as expected. If your PR only adds new packages or modifies existing ones,
+there's very little chance that your changes could cause the unit tests to fail.
+However, if you make changes to Spack's core libraries, you should run the unit
+tests to make sure you didn't break anything.
+
+Since they test things like fetching from VCS repos, the unit tests require
+`git <https://git-scm.com/>`_, `mercurial <https://www.mercurial-scm.org/>`_,
+and `subversion <https://subversion.apache.org/>`_ to run. Make sure these are
+installed on your system and can be found in your ``PATH``. All of these can be
+installed with Spack or with your system package manager.
+
+To run *all* of the unit tests, use:
 
 .. code-block:: console
 
-  $ git checkout develop_modified
+   $ spack test
 
-Let us assume that lines in files you will be modifying
-are the same in `develop_modified` branch and upstream ``develop``.
-Next edit files, make sure they work for you and create a commit
+These tests may take several minutes to complete. If you know you are only
+modifying a single Spack feature, you can run a single unit test at a time:
 
 .. code-block:: console
 
-  $ git add <files_to_be_commited>
-  $ git commit -m <descriptive note about changes>
+   $ spack test architecture
 
-Normally we prefer that commits pertaining to a package ``<package-name>``` have
-a message ``<package-name>: descriptive message``. It is important to add
-descriptive message so that others, who might be looking at your changes later
-(in a year or maybe two), would understand the rationale behind.
+This allows you to develop iteratively: make a change, test that change, make
+another change, test that change, etc. To get a list of all available unit
+tests, run:
+
+.. command-output:: spack test --list
+
+Unit tests are crucial to making sure bugs aren't introduced into Spack. If you
+are modifying core Spack libraries or adding new functionality, please consider
+adding new unit tests or strengthening existing tests.
+
+.. note::
+
+   There is also a ``run-unit-tests`` script in ``share/spack/qa`` that runs the
+   unit tests. Afterwards, it reports back to Coverage with the percentage of Spack
+   that is covered by unit tests. This script is designed for Travis CI. If you
+   want to run the unit tests yourself, we suggest you use ``spack test``.
+
+^^^^^^^^^^^^
+Flake8 Tests
+^^^^^^^^^^^^
+
+Spack uses `Flake8 <http://flake8.pycqa.org/en/latest/>`_ to test for
+`PEP 8 <https://www.python.org/dev/peps/pep-0008/>`_ conformance. PEP 8 is
+a series of style guides for Python that provide suggestions for everything
+from variable naming to indentation. In order to limit the number of PRs that
+were mostly style changes, we decided to enforce PEP 8 conformance. Your PR
+needs to comply with PEP 8 in order to be accepted.
+
+Testing for PEP 8 compliance is easy. Simply add the quality assurance
+directory to your ``PATH`` and run the flake8 script:
+
+.. code-block:: console
+
+   $ export PATH+=":$SPACK_ROOT/share/spack/qa"
+   $ run-flake8-tests
 
+``run-flake8-tests`` has a couple advantages over running ``flake8`` by hand:
 
-Next we will create a branch off upstream's ``develop`` and copy this commit.
-Before doing this, while still on your modified branch, get the hash of the
-last commit
+#. It only tests files that you have modified since branching off of develop.
+
+#. It works regardless of what directory you are in.
+
+#. It automatically adds approved exemptions from the flake8 checks. For example,
+   URLs are often longer than 80 characters, so we exempt them from the line
+   length checks. We also exempt lines that start with "homepage", "url", "version",
+   "variant", "depends_on", and "extends" in the ``package.py`` files.
+
+More approved flake8 exemptions can be found
+`here <https://github.com/LLNL/spack/blob/develop/.flake8>`_.
+
+If all is well, you'll see something like this:
 
 .. code-block:: console
 
-  $ git log -1
+   $ run-flake8-tests
+   Dependencies found.
+   =======================================================
+   flake8: running flake8 code checks on spack.
 
-and copy-paste this ``<hash>`` to the buffer. Now switch to upstream's ``develop``,
-make sure it's updated, checkout the new branch, apply the patch and push to
-GitHub:
+   Modified files:
+
+     var/spack/repos/builtin/packages/hdf5/package.py
+     var/spack/repos/builtin/packages/hdf/package.py
+     var/spack/repos/builtin/packages/netcdf/package.py
+   =======================================================
+   Flake8 checks were clean.
+
+However, if you aren't compliant with PEP 8, flake8 will complain:
 
 .. code-block:: console
 
-  $ git checkout develop
-  $ git pull upstream develop
-  $ git checkout -b <descriptive_branch_name>
-  $ git cherry-pick <hash>
-  $ git push <your_origin> <descriptive_branch_name> -u
+   var/spack/repos/builtin/packages/netcdf/package.py:26: [F401] 'os' imported but unused
+   var/spack/repos/builtin/packages/netcdf/package.py:61: [E303] too many blank lines (2)
+   var/spack/repos/builtin/packages/netcdf/package.py:106: [E501] line too long (92 > 79 characters)
+   Flake8 found errors.
 
-Here we assume that local ``develop`` branch tracks upstream develop branch of
-Spack. This is not a requirement and you could also do the same with remote
-branches. Yet to some it is more convenient to have a local branch that
-tracks upstream.
+Most of the error messages are straightforward, but if you don't understand what
+they mean, just ask questions about them when you submit your PR. The line numbers
+will change if you add or delete lines, so simply run ``run-flake8-tests`` again
+to update them.
 
-Now you can create a PR from web-interface of GitHub. The net result is as
-follows:
+.. tip::
 
-#. You patched your local version of Spack and can use it further
-#. You "cherry-picked" these changes in a stand-alone branch and submitted it
-   as a PR upstream.
+   Try fixing flake8 errors in reverse order. This eliminates the need for
+   multiple runs of ``flake8`` just to re-compute line numbers and makes it
+   much easier to fix errors directly off of the Travis output.
 
+.. warning::
 
-Should you have several commits to contribute, you could follow the same
-procedure by getting hashes of all of them and cherry-picking to the PR branch.
-This could get tedious and therefore there is another way:
+   Flake8 requires setuptools in order to run. If you installed ``py-flake8``
+   with Spack, make sure to add ``py-setuptools`` to your ``PYTHONPATH``.
+   Otherwise, you will get an error message like:
+
+   .. code-block:: console
+
+      Traceback (most recent call last):
+        File: "/usr/bin/flake8", line 5, in <module>
+          from pkg_resources import load_entry_point
+      ImportError: No module named pkg_resources
 
+^^^^^^^^^^^^^^^^^^^
+Documentation Tests
+^^^^^^^^^^^^^^^^^^^
 
-Second approach:
-----------------
+Spack uses `Sphinx <http://www.sphinx-doc.org/en/stable/>`_ to build its
+documentation. In order to prevent things like broken links and missing imports,
+we added documentation tests that build the documentation and fail if there
+are any warning or error messages.
 
-In the second approach we start from upstream ``develop`` (again assuming
-that your local branch `develop` tracks upstream):
+Building the documentation requires several dependencies, all of which can be
+installed with Spack:
+
+* sphinx
+* graphviz
+* git
+* mercurial
+* subversion
+
+.. warning::
+
+   Sphinx has `several required dependencies <https://github.com/LLNL/spack/blob/develop/var/spack/repos/builtin/packages/py-sphinx/package.py>`_.
+   If you installed ``py-sphinx`` with Spack, make sure to add all of these
+   dependencies to your ``PYTHONPATH``. The easiest way to do this is to run
+   ``spack activate py-sphinx`` so that all of the dependencies are symlinked
+   to a central location. If you see an error message like:
+
+   .. code-block:: console
+
+      Traceback (most recent call last):
+        File: "/usr/bin/flake8", line 5, in <module>
+          from pkg_resources import load_entry_point
+      ImportError: No module named pkg_resources
+
+   that means Sphinx couldn't find setuptools in your ``PYTHONPATH``.
+
+Once all of the dependencies are installed, you can try building the documentation:
 
 .. code-block:: console
 
-  $ git checkout develop
-  $ git pull upstream develop
-  $ git checkout -b <descriptive_branch_name>
+   $ cd "$SPACK_ROOT/lib/spack/docs"
+   $ make clean
+   $ make
 
-Next edit a few files and create a few commits by
+If you see any warning or error messages, you will have to correct those before
+your PR is accepted.
+
+.. note::
+
+   There is also a ``run-doc-tests`` script in the Quality Assurance directory.
+   The only difference between running this script and running ``make`` by hand
+   is that the script will exit immediately if it encounters an error or warning.
+   This is necessary for Travis CI. If you made a lot of documentation tests, it
+   is much quicker to run ``make`` by hand so that you can see all of the warnings
+   at once.
+
+If you are editing the documentation, you should obviously be running the
+documentation tests. But even if you are simply adding a new package, your
+changes could cause the documentation tests to fail:
 
 .. code-block:: console
 
-  $ git add <files_to_be_part_of_the_commit>
-  $ git commit -m <descriptive_message_of_this_particular_commit>
+   package_list.rst:8745: WARNING: Block quote ends without a blank line; unexpected unindent.
+
+At first, this error message will mean nothing to you, since you didn't edit
+that file. Until you look at line 8745 of the file in question:
+
+.. code-block:: rst
+
+   Description:
+      NetCDF is a set of software libraries and self-describing, machine-
+     independent data formats that support the creation, access, and sharing
+     of array-oriented scientific data.
+
+Our documentation includes :ref:`a list of all Spack packages <package-list>`.
+If you add a new package, its docstring is added to this page. The problem in
+this case was that the docstring looked like:
 
-Now you can push it to your fork and create a PR
+.. code-block:: python
+
+   class Netcdf(Package):
+       """
+       NetCDF is a set of software libraries and self-describing,
+       machine-independent data formats that support the creation,
+       access, and sharing of array-oriented scientific data.
+       """
+
+Docstrings cannot start with a newline character, or else Sphinx will complain.
+Instead, they should look like:
+
+.. code-block:: python
+
+   class Netcdf(Package):
+       """NetCDF is a set of software libraries and self-describing,
+       machine-independent data formats that support the creation,
+       access, and sharing of array-oriented scientific data."""
+
+Documentation changes can result in much more obfuscated warning messages.
+If you don't understand what they mean, feel free to ask when you submit
+your PR.
+
+-------------
+Git Workflows
+-------------
+
+Spack is still in the beta stages of development. Most of our users run off of
+the develop branch, and fixes and new features are constantly being merged. So
+how do you keep up-to-date with upstream while maintaining your own local
+differences and contributing PRs to Spack?
+
+^^^^^^^^^
+Branching
+^^^^^^^^^
+
+The easiest way to contribute a pull request is to make all of your changes on
+new branches. Make sure your ``develop`` is up-to-date and create a new branch
+off of it:
 
 .. code-block:: console
 
-  $ git push <your_origin> <descriptive_branch_name> -u
+   $ git checkout develop
+   $ git pull upstream develop
+   $ git branch <descriptive_branch_name>
+   $ git checkout <descriptive_branch_name>
+
+Here we assume that the local ``develop`` branch tracks the upstream develop
+branch of Spack. This is not a requirement and you could also do the same with
+remote branches. But for some it is more convenient to have a local branch that
+tracks upstream.
 
-Most likely you would want to have those changes in your (modified) local
-version of Spack. To that end you need to merge this branch
+Normally we prefer that commits pertaining to a package ``<package-name>`` have
+a message ``<package-name>: descriptive message``. It is important to add
+descriptive message so that others, who might be looking at your changes later
+(in a year or maybe two), would understand the rationale behind them.
+
+Now, you can make your changes while keeping the ``develop`` branch pure.
+Edit a few files and commit them by running:
 
 .. code-block:: console
 
-  $ git checkout develop_modified
-  $ git merge <descriptive_branch_name>
+   $ git add <files_to_be_part_of_the_commit>
+   $ git commit --message <descriptive_message_of_this_particular_commit>
 
-The net result is similar to the first approach with a minor difference that
-you would also merge upstream develop into you modified version in the last
-step. Should this not be desirable, you have to follow the first approach.
+Next, push it to your remote fork and create a PR:
 
+.. code-block:: console
 
+   $ git push origin <descriptive_branch_name> --set-upstream
 
-How to clean-up a branch by rewriting history:
------------------------------------------------
+GitHub provides a `tutorial <https://help.github.com/articles/about-pull-requests/>`_
+on how to file a pull request. When you send the request, make ``develop`` the
+destination branch.
 
-Sometimes you may end up on a branch that has a lot of commits, merges of
-upstream branch and alike but it can't be rebased on ``develop`` due to a long
-and convoluted history. If the current commits history is more of an experimental
-nature and only the net result is important, you may rewrite the history.
-To that end you need to first merge upstream `develop` and reset you branch to
-it. So on the branch in question do:
+If you need this change immediately and don't have time to wait for your PR to
+be merged, you can always work on this branch. But if you have multiple PRs,
+another option is to maintain a Frankenstein branch that combines all of your
+other branches:
 
 .. code-block:: console
 
-   $ git merge develop
-   $ git reset develop
+   $ git co develop
+   $ git branch <your_modified_develop_branch>
+   $ git checkout <your_modified_develop_branch>
+   $ git merge <descriptive_branch_name>
 
-At this point you your branch will point to the same commit as develop and
-thereby the two are indistinguishable. However, all the files that were
-previously modified will stay as such. In other words, you do not loose the
-changes you made. Changes can be reviewed by looking at diffs
+This can be done with each new PR you submit. Just make sure to keep this local
+branch up-to-date with upstream ``develop`` too.
+
+^^^^^^^^^^^^^^
+Cherry-Picking
+^^^^^^^^^^^^^^
+
+What if you made some changes to your local modified develop branch and already
+committed them, but later decided to contribute them to Spack? You can use
+cherry-picking to create a new branch with only these commits.
+
+First, check out your local modified develop branch:
 
 .. code-block:: console
 
-   $ git status
-   $ git diff
+   $ git checkout <your_modified_develop_branch>
 
-One can also run GUI to visualize the current changes
+Now, get the hashes of the commits you want from the output of:
 
 .. code-block:: console
 
-   $ git difftool
+   $ git log
 
-Next step is to rewrite the history by adding files and creating commits
+Next, create a new branch off of upstream ``develop`` and copy the commits
+that you want in your PR:
 
 .. code-block:: console
 
-   $ git add <files_to_be_part_of_commit>
-   $ git commit -m <descriptive_message>
+   $ git checkout develop
+   $ git pull upstream develop
+   $ git branch <descriptive_branch_name>
+   $ git checkout <descriptive_branch_name>
+   $ git cherry-pick <hash>
+   $ git push origin <descriptive_branch_name> --set-upstream
 
+Now you can create a PR from the web-interface of GitHub. The net result is as
+follows:
 
-Shall you need to split changes within a file into separate commits, use
+#. You patched your local version of Spack and can use it further.
+#. You "cherry-picked" these changes in a stand-alone branch and submitted it
+   as a PR upstream.
+
+Should you have several commits to contribute, you could follow the same
+procedure by getting hashes of all of them and cherry-picking to the PR branch.
+
+.. note::
+
+   It is important that whenever you change something that might be of
+   importance upstream, create a pull request as soon as possible. Do not wait
+   for weeks/months to do this, because:
+
+   #. you might forget why you modified certain files
+   #. it could get difficult to isolate this change into a stand-alone clean PR.
+
+^^^^^^^^
+Rebasing
+^^^^^^^^
+
+Other developers are constantly making contributions to Spack, possibly on the
+same files that your PR changed. If their PR is merged before yours, it can
+create a merge conflict. This means that your PR can no longer be automatically
+merged without a chance of breaking your changes. In this case, you will be
+asked to rebase on top of the latest upstream ``develop``.
+
+First, make sure your develop branch is up-to-date:
 
 .. code-block:: console
 
-   $ git add <file> -p
+   $ git checkout develop
+   $ git pull upstream develop
 
-After all changed files are committed, you can push the branch to your fork
-and create a PR
+Now, we need to switch to the branch you submitted for your PR and rebase it
+on top of develop:
+
+.. code-block:: console
+
+   $ git checkout <descriptive_branch_name>
+   $ git rebase develop
+
+Git will likely ask you to resolve conflicts. Edit the file that it says can't
+be merged automatically and resolve the conflict. Then, run:
 
 .. code-block:: console
 
-   $ git push <you_origin> -u
+   $ git add <file_that_could_not_be_merged>
+   $ git rebase --continue
 
+You may have to repeat this process multiple times until all conflicts are resolved.
+Once this is done, simply force push your rebased branch to your remote fork:
 
+.. code-block:: console
 
-How to fix a bad rebase by "cherry-picking" commits:
-----------------------------------------------------
+   $ git push --force origin <descriptive_branch_name>
 
-Say you are working on a branch ``feature1``. It has several commits and is
-ready to be merged. However, there are a few minor merge conflicts and so
-you are asked to rebase onto ``develop`` upstream branch. Occasionally, it
-happens so that a contributor rebases not on top of the upstream branch, but
-on his/her local outdated copy of it. This would lead to an inclusion of the
-whole lot of duplicated history and of course can not be merged as-is.
+^^^^^^^^^^^^^^^^^^^^^^^^^
+Rebasing with cherry-pick
+^^^^^^^^^^^^^^^^^^^^^^^^^
 
-One way to get out of troubles is to ``cherry-pick`` important commits. To
-do that, first checkout a temporary back-up branch:
+You can also perform a rebase using ``cherry-pick``. First, create a temporary
+backup branch:
 
 .. code-block:: console
 
-  git checkout -b tmp
+   $ git checkout <descriptive_branch_name>
+   $ git branch tmp
 
-Now look at logs and save hashes of commits you would like to keep
+If anything goes wrong, you can always go back to your ``tmp`` branch.
+Now, look at the logs and save the hashes of any commits you would like to keep:
 
 .. code-block:: console
 
-  git log
+   $ git log
 
 Next, go back to the original branch and reset it to ``develop``.
 Before doing so, make sure that you local ``develop`` branch is up-to-date
-with the upstream.
+with upstream:
+
+.. code-block:: console
+
+   $ git checkout develop
+   $ git pull upstream develop
+   $ git checkout <descriptive_branch_name>
+   $ git reset --hard develop
+
+Now you can cherry-pick relevant commits:
+
+.. code-block:: console
+
+   $ git cherry-pick <hash1>
+   $ git cherry-pick <hash2>
+
+Push the modified branch to your fork:
 
 .. code-block:: console
 
-  git checkout feature1
-  git reset --hard develop
+   $ git push --force origin <descriptive_branch_name>
 
-Now you can cherry-pick relevant commits
+If everything looks good, delete the backup branch:
 
 .. code-block:: console
 
-  git cherry-pick  <hash1>
-  git cherry-pick  <hash2>
+   $ git branch --delete --force tmp
+
+^^^^^^^^^^^^^^^^^^
+Re-writing History
+^^^^^^^^^^^^^^^^^^
 
+Sometimes you may end up on a branch that has diverged so much from develop
+that it cannot easily be rebased. If the current commits history is more of
+an experimental nature and only the net result is important, you may rewrite
+the history.
 
-push the modified branch to your fork
+First, merge upstream ``develop`` and reset you branch to it. On the branch
+in question, run:
 
 .. code-block:: console
 
-  git push -f
+   $ git merge develop
+   $ git reset develop
 
-and if everything looks good, delete the back-up:
+At this point your branch will point to the same commit as develop and
+thereby the two are indistinguishable. However, all the files that were
+previously modified will stay as such. In other words, you do not lose the
+changes you made. Changes can be reviewed by looking at diffs:
 
 .. code-block:: console
 
-  git branch -D tmp
+   $ git status
+   $ git diff
+
+The next step is to rewrite the history by adding files and creating commits:
+
+.. code-block:: console
+
+   $ git add <files_to_be_part_of_commit>
+   $ git commit --message <descriptive_message>
+
+After all changed files are committed, you can push the branch to your fork
+and create a PR:
+
+.. code-block:: console
+
+   $ git push origin --set-upstream
+