Skip to content
Snippets Groups Projects
Commit 78d3c7e2 authored by Adam J. Stewart's avatar Adam J. Stewart Committed by Todd Gamblin
Browse files

Major updates to Contribution Guide (#1968) 

* Major updates to Contribution Guide

* Grammar changes

* Fix missing/extra backticks

* Rewording, links, and tips added
parent 0d89e5e3
Branches
Tags
No related merge requests found
Hide whitespace changes
Inline Side-by-side
README.md
+ 4
− 10
View file @ 78d3c7e2
...@@ -63,17 +63,11 @@ Contributing to Spack is relatively easy. Just send us a ...@@ -63,17 +63,11 @@ Contributing to Spack is relatively easy. Just send us a
When you send your request, make ``develop`` the destination branch on the When you send your request, make ``develop`` the destination branch on the
[Spack repository](https://github.com/LLNL/spack). [Spack repository](https://github.com/LLNL/spack).
Before you send a PR, your code should pass the following checks: Your PR must pass Spack's unit tests and documentation tests, and must be
[PEP 8](https://www.python.org/dev/peps/pep-0008/) compliant.
* Your contribution will need to pass the `spack test` command.
Run this before submitting your PR.
* Also run the `share/spack/qa/run-flake8-tests` script to check for PEP8 compliance.
To encourage contributions and readability by a broad audience,
Spack uses the [PEP8](https://www.python.org/dev/peps/pep-0008/) coding
standard with [a few exceptions](https://github.com/LLNL/spack/blob/develop/.flake8).
We enforce these guidelines with [Travis CI](https://travis-ci.org/LLNL/spack). We enforce these guidelines with [Travis CI](https://travis-ci.org/LLNL/spack).
To run these tests locally, and for helpful tips on git, see our
[Contribution Guide](http://spack.readthedocs.io/en/latest/contribution_guide.html).
Spack uses a rough approximation of the [Git Spack uses a rough approximation of the [Git
Flow](http://nvie.com/posts/a-successful-git-branching-model/) Flow](http://nvie.com/posts/a-successful-git-branching-model/)
......
lib/spack/docs/contribution_guide.rst
+ 401
− 127
View file @ 78d3c7e2
.. _contribution-guide: .. _contribution-guide:
==================
Contribution Guide Contribution Guide
===================== ==================
This guide is intended for developers or administrators who want to This guide is intended for developers or administrators who want to
contribute a new package, feature, or bugfix to Spack. contribute a new package, feature, or bugfix to Spack.
It assumes that you have at least some familiarity with Git VCS and Github. 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 guide will show a few examples of contributing workflows and discuss
the granularity of pull-requests (PRs). 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/>`_: 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**. Pull requests are a mechanism for a developer to notify team members that
The pull request is more than just a notification—it’s a dedicated forum for discussing the proposed feature 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 correspond to one feature/bugfix/extension/etc. One can create PRs with
changes relevant to different ideas, however reviewing such PRs becomes tedious 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 Spack uses a rough approximation of the `Git Flow <http://nvie.com/posts/a-successful-git-branching-model/>`_
model. The develop branch contains the latest contributions, and master is branching model. The develop branch contains the latest contributions, and
always tagged and points to the latest stable release. Thereby when you send master is always tagged and points to the latest stable release. Therefore, when
your request, make ``develop`` the destination branch on the you send your request, make ``develop`` the destination branch on the
`Spack repository <https://github.com/LLNL/spack>`_. `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 Continuous Integration
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.
Now let us discuss several approaches one may use to submit a PR while Spack uses `Travis CI <https://travis-ci.org/LLNL/spack>`_ for Continuous Integration
also keeping your local version of Spack patched. 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. Unit tests ensure that core Spack features like fetching or spec resolution are
You checkout your local develop branch, which for the purpose of this guide working as expected. If your PR only adds new packages or modifies existing ones,
will be called ``develop_modified``: 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 .. code-block:: console
$ git checkout develop_modified $ spack test
Let us assume that lines in files you will be modifying These tests may take several minutes to complete. If you know you are only
are the same in `develop_modified` branch and upstream ``develop``. modifying a single Spack feature, you can run a single unit test at a time:
Next edit files, make sure they work for you and create a commit
.. code-block:: console .. code-block:: console
$ git add <files_to_be_commited> $ spack test architecture
$ git commit -m <descriptive note about changes>
Normally we prefer that commits pertaining to a package ``<package-name>``` have This allows you to develop iteratively: make a change, test that change, make
a message ``<package-name>: descriptive message``. It is important to add another change, test that change, etc. To get a list of all available unit
descriptive message so that others, who might be looking at your changes later tests, run:
(in a year or maybe two), would understand the rationale behind.
.. 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. #. It only tests files that you have modified since branching off of develop.
Before doing this, while still on your modified branch, get the hash of the
last commit #. 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 .. 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``, Modified files:
make sure it's updated, checkout the new branch, apply the patch and push to
GitHub: 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 .. code-block:: console
$ git checkout develop var/spack/repos/builtin/packages/netcdf/package.py:26: [F401] 'os' imported but unused
$ git pull upstream develop var/spack/repos/builtin/packages/netcdf/package.py:61: [E303] too many blank lines (2)
$ git checkout -b <descriptive_branch_name> var/spack/repos/builtin/packages/netcdf/package.py:106: [E501] line too long (92 > 79 characters)
$ git cherry-pick <hash> Flake8 found errors.
$ git push <your_origin> <descriptive_branch_name> -u
Here we assume that local ``develop`` branch tracks upstream develop branch of Most of the error messages are straightforward, but if you don't understand what
Spack. This is not a requirement and you could also do the same with remote they mean, just ask questions about them when you submit your PR. The line numbers
branches. Yet to some it is more convenient to have a local branch that will change if you add or delete lines, so simply run ``run-flake8-tests`` again
tracks upstream. to update them.
Now you can create a PR from web-interface of GitHub. The net result is as .. tip::
follows:
#. You patched your local version of Spack and can use it further Try fixing flake8 errors in reverse order. This eliminates the need for
#. You "cherry-picked" these changes in a stand-alone branch and submitted it multiple runs of ``flake8`` just to re-compute line numbers and makes it
as a PR upstream. much easier to fix errors directly off of the Travis output.
.. warning::
Should you have several commits to contribute, you could follow the same Flake8 requires setuptools in order to run. If you installed ``py-flake8``
procedure by getting hashes of all of them and cherry-picking to the PR branch. with Spack, make sure to add ``py-setuptools`` to your ``PYTHONPATH``.
This could get tedious and therefore there is another way: 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 Building the documentation requires several dependencies, all of which can be
that your local branch `develop` tracks upstream): 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 .. code-block:: console
$ git checkout develop $ cd "$SPACK_ROOT/lib/spack/docs"
$ git pull upstream develop $ make clean
$ git checkout -b <descriptive_branch_name> $ 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 .. code-block:: console
$ git add <files_to_be_part_of_the_commit> package_list.rst:8745: WARNING: Block quote ends without a blank line; unexpected unindent.
$ git commit -m <descriptive_message_of_this_particular_commit>
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 .. 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 Normally we prefer that commits pertaining to a package ``<package-name>`` have
version of Spack. To that end you need to merge this branch 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 .. code-block:: console
$ git checkout develop_modified $ git add <files_to_be_part_of_the_commit>
$ git merge <descriptive_branch_name> $ git commit --message <descriptive_message_of_this_particular_commit>
The net result is similar to the first approach with a minor difference that Next, push it to your remote fork and create a PR:
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.
.. 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 If you need this change immediately and don't have time to wait for your PR to
upstream branch and alike but it can't be rebased on ``develop`` due to a long be merged, you can always work on this branch. But if you have multiple PRs,
and convoluted history. If the current commits history is more of an experimental another option is to maintain a Frankenstein branch that combines all of your
nature and only the net result is important, you may rewrite the history. other branches:
To that end you need to first merge upstream `develop` and reset you branch to
it. So on the branch in question do:
.. code-block:: console .. code-block:: console
$ git merge develop $ git co develop
$ git reset 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 This can be done with each new PR you submit. Just make sure to keep this local
thereby the two are indistinguishable. However, all the files that were branch up-to-date with upstream ``develop`` too.
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 ^^^^^^^^^^^^^^
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 .. code-block:: console
$ git status $ git checkout <your_modified_develop_branch>
$ git diff
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 .. 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 .. code-block:: console
$ git add <files_to_be_part_of_commit> $ git checkout develop
$ git commit -m <descriptive_message> $ 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 .. 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 Now, we need to switch to the branch you submitted for your PR and rebase it
and create a PR 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 .. 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 Rebasing with cherry-pick
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.
One way to get out of troubles is to ``cherry-pick`` important commits. To You can also perform a rebase using ``cherry-pick``. First, create a temporary
do that, first checkout a temporary back-up branch: backup branch:
.. code-block:: console .. 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 .. code-block:: console
git log $ git log
Next, go back to the original branch and reset it to ``develop``. 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 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 .. code-block:: console
git checkout feature1 $ git push --force origin <descriptive_branch_name>
git reset --hard develop
Now you can cherry-pick relevant commits If everything looks good, delete the backup branch:
.. code-block:: console .. code-block:: console
git cherry-pick <hash1> $ git branch --delete --force tmp
git cherry-pick <hash2>
^^^^^^^^^^^^^^^^^^
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 .. 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 .. 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
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment