A workflow to contribute patches to Spack in a clean way

[Denis Davydov]

Hi all,

The content of this post might look simple for some, nonetheless i would like

to share a few tips on how one can use Git and contribute PR's to Spack while

maintaining a locally modified version. 

Disclaimer: I am not a Spack's developer, but a regular contributor to the project.

The intention is twofold:

1) share a workflow with the current and potential contributors.

2) start a discussion on this topic with a hope that it will get summarised into a "sticky" or a WiKi page.

The goal is to facilitate reviewing process of PRs by making them more concise and better organised.

A few important things about PRs from 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

This implies that files modified in a single PR should correspond to one feature/bugfix/extension/etc. One can create

PRs with changes relevant to different ideas/features/bugfixes/etc, but reviewing such PRs becomes tedious and error prone.

For example on Homebrew there is a rule one-PR-one-package.

Ok, so you use Spack and also patch/bend it in a few place to fit your needs.

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.

The first thing is that whenever you change something that might be of importance upstream,

create a pull-request (PR) A.S.A.P. 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 nice and clean PR.

First approach is as follows. You checkout your local hacked develop branch. Let's assume you call it develop_modified:

$git checkout develop_modified

Before we start, we need to assume that lines in files you will be modifying 

are the same in your own develop branch and upstream.

Now you edit files, make sure they work for you, create a commit

$git add <files_to_be_commited>
$git commit -m <descriptive note about changes>

As discussed above, if your intention is to contribute those changes, this have to be done ASAP for everyones benefit.

To do so we will create a new branch off the upstream develop and apply those changes. While still on your modified branch

get the hash of the last commit

$ git log -1

copy-paste the <hash> of the commit to the buffer. Now switch to upstream's develop, make sure it's updated, checkout the new branch and apply the patch and push to github:

$git checkout develop
$git pull
$git checkout -b <descriptive_branch_name>
$git cherry-pick <hash>
$git push <your_origin> -u

Here I assume that `develop` branch tracks upstream develop branch of Spack. This is not a requirement,

you could also do the same with remote branches. I personally find it more convenient to have a local branch which 

tracks upstream.

Now you can create a PR from web-interface of GitHub. The net result is as follows:

1. you patched your local version of Spack and can use it further

2. 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 it to the PR branch. This could get tedious and therefore there is another way:

In the Second approach we start from upstream develop: 

$ git checkout develop
$ git pull
$ git checkout -b <descriptive_branch_name>

Next you edit a few files and add a few commits by repeating

$git add <files_to_be_part_of_the_commit>
$git commit -m <descriptive_message_of_this_particular_commit>

Now one can push it to your fork and create a PR

$git push <your_origin> -u

Finally, since we want to have those changes in the modified version of spack we use, we need to merge this branch

$git checkout develop_modified
$git merge <descriptive_branch_name>

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.

How to clean-up a branch by rewriting history:

Say you are on a branch which has a lot of changes and can't be rebased on develop due to a long and convoluted history.

The way out is to merge upstream develop and reset to it. On the branch in question do:

$git merge develop
$git reset develop

At this point you will not have any commits and your branch will point to the same commit as develop,

BUT all the files which were changed previously will stay as such. You can review changes and look at diffs by

$git status
$git diff      ## or git difftool  for GUI

Now rewrite the history by adding files and committing

$git add <files_to_be_part_of_commit>
$git commit -m <descriptive_message>

You can also split changes within a file into separate commits, to that end do

$git add <file> -p

After all changed files are committed, you can push to your fork and create a PR

$git push <you_origin> -u

Regards,

Denis.

[Robert French]

I like this idea. One PR per feature or per package is granular enough to make yes/no decisions easier.

--
You received this message because you are subscribed to the Google Groups "Spack" group.
To unsubscribe from this group and stop receiving emails from it, send an email to spack+unsubscribe@googlegroups.com.
To post to this group, send email to sp...@googlegroups.com.
Visit this group at https://groups.google.com/group/spack.
For more options, visit https://groups.google.com/d/optout.

[Elizabeth F]

That is already how we do things.

[Denis Davydov]

On Sunday, August 28, 2016 at 10:38:59 PM UTC+2, Elizabeth F wrote:

That is already how we do things.

New contributors always come, so i think it's worth documenting best practices to ease the pain. I would  probably do this in CONTRIBUTING.md based on the OP. 

See https://github.com/dealii/dealii/blob/master/CONTRIBUTING.md for an example of what I am thinking about.

[Elizabeth F]

---------- Forwarded message ----------
From: Elizabeth F <eliz...@citibob.net>
Date: Mon, Aug 29, 2016 at 9:24 AM
Subject: Re: [spack] A workflow to contribute patches to Spack in a clean way
To: Denis Davydov <davy...@gmail.com>

Denis,

Why don't you add that to the Spack Developer Guide?  It's in .rst, not .md, but hopefully that won't be a serious obstacle.

http://software.llnl.gov/spack/developer_guide.html

https://github.com/LLNL/spack/blob/develop/lib/spack/docs/developer_guide.rst

-- Elizabeth

--

[Denis Davydov]

One could add it there, but for Developer guide:

This guide is intended for people who want to work on Spack itself. If you just want to develop packages, see the Packaging Guide.

whereas the content of the OP is also relevant to Packaging guide.

Maybe it should go to the same document but into a separate section like:

"Git contribution guide" ?

before "Packaging guide" section.

Or any other name which is more appropriate. Thoughts?

Regards,

Denis.

[Elizabeth F]

Denis,

You bring up a good point.  Looking at it now, it would seem that the Developer Guid and Packaging Guide should go next to each other, maybe under a single subheading.

I think the manual needs at some point a global review and possible reorganization.  In the meantime, maybe it's best to just write the section your propose, and put it where you think it goes best in the current structure.

-- Elizabeth

--

[Denis Davydov]

see https://github.com/LLNL/spack/pull/1664

To unsubscribe from this group and stop receiving emails from it, send an email to spack+un...@googlegroups.com.