summaryrefslogtreecommitdiff
path: root/lib
diff options
context:
space:
mode:
authorAdam J. Stewart <ajstewart426@gmail.com>2016-10-14 11:16:13 -0500
committerTodd Gamblin <tgamblin@llnl.gov>2016-10-14 09:16:13 -0700
commit78d3c7e2a28069b9099f9d621e785f23b4e44359 (patch)
tree12330fca7f6608ff00e09016f5b089c12f6ec9c7 /lib
parent0d89e5e32b120fc36317240eacdb4fbb55e6425c (diff)
downloadspack-78d3c7e2a28069b9099f9d621e785f23b4e44359.tar.gz
spack-78d3c7e2a28069b9099f9d621e785f23b4e44359.tar.bz2
spack-78d3c7e2a28069b9099f9d621e785f23b4e44359.tar.xz
spack-78d3c7e2a28069b9099f9d621e785f23b4e44359.zip
Major updates to Contribution Guide (#1968)
* Major updates to Contribution Guide * Grammar changes * Fix missing/extra backticks * Rewording, links, and tips added
Diffstat (limited to 'lib')
-rw-r--r--lib/spack/docs/contribution_guide.rst528
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
+