-
Notifications
You must be signed in to change notification settings - Fork 27
update doc about GitHub PR cycle #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
# Github workflow and contribution guide | ||
|
||
[TOC] | ||
|
||
|
||
## Overview | ||
## Introduction | ||
|
||
This page should allow users that are new to Git to get started with the FCC | ||
software, and describes the workflow for accessing and contributing FCC | ||
|
@@ -14,81 +14,148 @@ For a general introduction to git, have a look at these tutorials: | |
- [Interactive tutorial](https://pcottle.github.io/learnGitBranching/) | ||
- [The git book](https://git-scm.com/book/en/v2) | ||
|
||
## First time setup of git | ||
### First time setup of git | ||
|
||
Set global information about you, particularly your name and email. This information will be attached to each commit and it will be visible to other contributors. To do it, please use the following commands | ||
```bash | ||
git config --global user.name "Name Family-name" | ||
git config --global user.email "[email protected]" | ||
``` | ||
|
||
Please refer to [this tutorial](https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup) and the [GitHub help](https://help.github.com/articles/set-up-git/). | ||
|
||
## Generate and set up ssh keys for github | ||
### Generate and set up ssh keys for github | ||
|
||
When working on lxplus we recommend to clone github repositories via SSH, especially if you want to contribute code. For this to work, you need to generate ssh keys for authentication. See the corresponding github [help-page](https://help.github.com/articles/generating-an-ssh-key/). | ||
|
||
:::{admonition} Generate and set up ssh keys for github | ||
:class: callout | ||
|
||
If you only want to use the software it may be easier to use https. In that case you don't need to generate the keys but have to replace `git@github:` with `https://github.com/` in all the instructions. Note that you'll not be able to push to your repository when you are on lxplus. You can also start using https for now and later re-add your repository with ssh authentication, see the [trouble shooting section](#trouble-shooting) | ||
If you only want to use the software it may be easier to use https. In that case you don't need to generate the keys but have to replace `git@github:` with `https://github.com/` in all the instructions. Note that you'll not be able to push to your repository. You can also start using https for now and later re-add your repository with ssh authentication, see the [trouble shooting section](#trouble-shooting) | ||
::: | ||
|
||
|
||
## Improving your git experience | ||
### Improving your git experience | ||
|
||
It may be useful to install [Git integration tools](https://github.com/git/git/tree/master/contrib/completion) for your shell that allow tab-completion of most git commands and also can show you in your prompt on which branch you currently are, what changes you have pending, etc. | ||
|
||
## Development workflow | ||
Other tools with a graphical interface are available, e.g., `git-gui`, `git-cola`, `gitkraken` (which present the main Git commands as buttons, shows differences between commits, etc.) and `tig` (a terminal-based tool that graphically displays the Git commit history). | ||
atolosadelgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## How to contribute | ||
|
||
This section covers the main commands to contribute to an open project via Pull Requests. See [TLDR section](TL;DR) for a summary. | ||
|
||
You will be using (at least) 3 versions of the FCCSW repository: | ||
### Understanding repositories | ||
|
||
1. The official [FCCSW on github](https://github.com/HEP-FCC/FCCSW) | ||
2. Your fork of FCCSW on github (see [github help](https://help.github.com/articles/fork-a-repo/) on what that means) | ||
3. Your local repository copy in your work area (e.g. on AFS) | ||
You will be using (at least) 3 versions of a repository, two on GitHub and a local copy in your machine: | ||
|
||
The repositories 1 and 2 are added as remote to the repository 3: | ||
1. The official on GitHub, e.g. [FCCSW on github](https://github.com/HEP-FCC/FCCSW) | ||
2. Your fork on GitHub (see [github help](https://help.github.com/articles/fork-a-repo/) on what that means) | ||
3. Your local repository copy in your work area | ||
|
||
The repositories 1 and 2 are added as remote to the repository 3. Let's start by creating a local copy of a dummy repository called `key4hep_repo`, from the `key4hep` organization | ||
|
||
```bash | ||
git clone [email protected]:[YOUR_GITHUB_USER]/key4hep_repo.git | ||
``` | ||
|
||
That command links the local copy to the remote repository. The default name of this remote repository is `origin`. Let's add the officialrepository as remote too, but with a different name. The usual convention is to name the official repository `upstream`: | ||
atolosadelgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
```bash | ||
git clone [email protected]:[YOUR_GITHUB_USER]/FCCSW.git # create a local copy (3) of your fork (2) | ||
cd FCCSW | ||
git remote add hep-fcc git@github.com:HEP-FCC/FCCSW.git # add official repo (1) as additional remote | ||
git remote add upstream https://github.com/key4hep/key4hep_repo.git | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say the opposite here, I typically want to clone the repository from Key4hep and then add my remote. This makes it so that pulling will pull by default from the Key4hep one instead of your fork, that may never receive any updates. |
||
``` | ||
|
||
### Keeping your local repository up to date | ||
### Development workflow | ||
|
||
- fetch all changes from the official repository (1) | ||
This section shows how to work with a non-global package, how to develop a change and upstream it to the official repository. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What non-global means is not very clear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. true! Thanks for spotting it. I will replace the whole sentence by the following, is it better? |
||
|
||
```bash | ||
git fetch hep-fcc | ||
``` | ||
A recommend zero step is to open an issue in the corresponding repository, e.g. [in FCCSW repository](https://github.com/HEP-FCC/FCCSW/issues). | ||
atolosadelgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- rebase your development area to the master branch from the official repository (1), **please read [this](https://www.atlassian.com/git/tutorials/rewriting-history) to avoid loss of work** | ||
#### Source the software stack | ||
|
||
```bash | ||
git rebase -i hep-fcc/master | ||
``` | ||
We need to source the software stack before starting the actual development, e.g., | ||
```bash | ||
# nightlies | ||
source /cvmfs/sw-nightlies.hsf.org/key4hep/setup.sh | ||
# release | ||
source /cvmfs/sw.hsf.org/key4hep/setup.sh | ||
``` | ||
|
||
in this process you can also fix any commits that need touching up, **be aware that deleting commits in the list will result also in the deletion of the corresponding changes** (more info in the [GitHub help](https://help.github.com/articles/about-git-rebase/) and the [Atlassian tutorial](https://www.atlassian.com/git/tutorials/rewriting-history)) | ||
The sourcing of stacks accepts arguments: | ||
* -r <year-month-day>, for a specific date | ||
* -d, for packages compiled with debug symbols | ||
atolosadelgado marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- push your local changes to your fork (2), see [below](#contributing-code) how to create a local branch | ||
#### Branching | ||
|
||
``` | ||
git push origin [NAME_OF_LOCAL_BRANCH] | ||
``` | ||
The development should happen in a dedicated branch of your personal repository (2), forked from the official repository (1). To download a local copy of your personal fork, run the following command: | ||
```bash | ||
git clone [email protected]:[YOUR_GITHUB_USER]/key4hep_repo.git | ||
``` | ||
|
||
### Contributing code | ||
Move to the new directory, | ||
```bash | ||
cd key4hep_repo | ||
``` | ||
|
||
- if you are fixing a bug, first create an issue in the github [issue tracker](https://github.com/HEP-FCC/FCCSW/issues) | ||
- develop your feature in your local copy (3) on a local branch of your choice, to create a branch do: | ||
Create a new branch for the feature you are developing. This branch is created now on your local copy of the repository. We will have to update the GitHub repository later. | ||
```bash | ||
git checkout -b new_feature_branch | ||
``` | ||
|
||
``` | ||
git branch -b [NAME_OF_LOCAL_BRANCH] | ||
``` | ||
Now we are ready to start developing the actual changes. | ||
|
||
#### Creating changes | ||
|
||
Let's replace locally the official repository in the stack by our local copy. To do so, configure (first line), build and install (second line) your local copy of the repository, and then redefine the bash environmental variables to point to your local copy (third line). This is achieved by the following code: | ||
```bash | ||
cmake -B build -S . -D CMAKE_INSTALL_PREFIX=install | ||
cmake --build build -- -j 4 install | ||
k4_local_repo | ||
``` | ||
|
||
#### Create commits and run tests | ||
|
||
Modify files as needed for your development. Ideally, each meaninful change should correspond to a commit. Before adding a new commit, it is recommend to run some kind of test (at least that it compiles and run without errors; running ctest would be highly desirable), otherwise there is a risk of leaving the repository in a so-called `broken state`. The following lines show how to mark for commiting, so called `stage`, the files `file1`, `file2` and `README` (first line), and then creating a commit (second line). The commit message should make the change in the files understandable to other developers. | ||
```bash | ||
git add file1 README | ||
git commit -m "new awesome feature" | ||
``` | ||
Refer to [this tutorial](https://www.atlassian.com/git/tutorials/saving-changes) further details about commiting in Git. | ||
|
||
:::{admonition} | ||
If the feature is completely new in the repository, it is advisable to add a new ctest. Since there is not a standard in Key4hep, it is easier to ask the maintainer of the corresponding repository how to do it. See the following [link](https://github.com/HEP-FCC/FCCSW/blob/master/doc/AddingTestsToFCCSW.md) for further details | ||
::: | ||
|
||
Once all your changes are ready, please run the ctests. To do so, go to the build directory and run `ctest` as follows. | ||
```bash | ||
cd build | ||
ctest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe mention There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ctest -j N does not honor the dependency among tests, so for a new person running this, can be misleading (there should not be dependencies among tests in first place, but we know how it works...) |
||
``` | ||
|
||
:::{admonition} | ||
If you want to integrate changes in the official repository (1) happening while you are developing, [the next section](#keeping-your-local-repository-up-to-date)) show how to update your local repository, in such a manner that your changes (git commits) are reallocated on top of the newest changes from the official repository (so-called `rebasing`). | ||
::: | ||
|
||
Then, we push the changes of our local repository in the development branch `new_feature_branch` to the personal fork in GitHub (named `origin`) by doing the following | ||
```bash | ||
git push origin new_feature_branch | ||
``` | ||
|
||
### Opening a Pull Request (PR) | ||
|
||
Once we have pushed the changes into a new branch of our fork, we are ready to open a Pull Request. If we now go to the GitHub page of the repository, `https://github.com/[YOUR_GITHUB_USER]/key4hep_repo`, a banner asking if we want to open a Pull request should appear. If not, look for a button called `Contribute`, and click on `Open pull request`. Please check the [following section](Pull-Request-description) for a list of tasks to be fullfilled before opening a PR. | ||
|
||
|
||
### After merging the Pull Request | ||
|
||
After the PR is merged you will have to update (_rebase_) the main branch in the forked repository, **not the feature branch**. The following commands will update the local copy of the main branch with the `upstream/main` branch, and then push the changes to the forked repository | ||
|
||
- refer to [this tutorial](https://www.atlassian.com/git/tutorials/saving-changes) to see how to commit changes | ||
- occasionally, get new code from the official repository (1) as explained above and merge it in this branch | ||
- test: | ||
- that the code compiles and all tests succeed (`make && make test`) | ||
- that your code runs (even better: [add an automatic test](https://github.com/HEP-FCC/FCCSW/blob/master/doc/AddingTestsToFCCSW.md)) | ||
- that it produces the expected results | ||
- push your local branch to your fork (2) (see [above](#keeping-your-local-repository-up-to-date)) | ||
- create a pull request from your fork (2) to the offical repository's (1) master branch (see github [help-page](https://help.github.com/articles/creating-a-pull-request/)) | ||
- also see the [recommendations for pull requests](#pull-requests) | ||
``` | ||
git fetch upstream -p --all | ||
git checkout main | ||
git rebase upstream/main | ||
git push | ||
``` | ||
|
||
## Recommendations | ||
|
||
|
@@ -119,6 +186,89 @@ Please always follow the recommendations below. | |
|
||
Here, you may write a few more lines if needed | ||
``` | ||
### Pull Request description | ||
|
||
When opening a new Pull Request, please fill the following information in text box. | ||
|
||
1. Please fill a short description what your PR is changing, to be added in the release notes of the repository. This text has the following syntax | ||
``` | ||
BEGINRELEASENOTES | ||
- New/Extends XXX | ||
- Fixes issue XXX | ||
|
||
ENDRELEASENOTES | ||
``` | ||
|
||
2. In the description, give a short bullet-point list of what was done. If your PR is fixing an open Issue in GitHub, please | ||
- include the link to it. | ||
- Make sure you added a test that shows they are actually fixed | ||
- In the description mention that you fixed it by referring to the issue: "fixes #<issue-id>" (this will automatically close the issue, see also [GitHub help](https://help.github.com/articles/closing-issues-via-commit-messages/)) | ||
|
||
|
||
:::{admonition} | ||
There are number of requisites that will speed up the revision of the Pull request | ||
* Give a meaningful title that appropriately describes what you did (e.g. Add new calorimeter clustering) | ||
* Pull requests of work in progress (to make people aware that you are working on a feature) create a PR starting with "[WIP]" | ||
* Sufficiently documented code (inline, doxygenm, readmes...) | ||
* Tests cover the modified code | ||
* The branch is up to date (see [next section](Keeping-your-local-repository-up-to-date) about how to do it) | ||
* The pull request only contains minimal necesary changes | ||
|
||
In case of modifying detector-related code, please check the following as well: | ||
* Overlap test, [link](https://hep-fcc.github.io/fcc-tutorials/main/full-detector-simulations/Geometry/Geometry.html#overlap-checking) | ||
* Simulation test, run with a particle gun of your choice, e.g. [link](https://hep-fcc.github.io/fcc-tutorials/main/full-detector-simulations/Geometry/Geometry.html#modify-an-existing-xml-file) | ||
::: | ||
|
||
### Keeping your local repository up to date | ||
|
||
It may happen that your fork or development branch becomes outdated with respect to the main branch in the official (`upstream`) repository (1). To update your development branch of your forked repository, please follow these steps: | ||
|
||
1. Fetch all changes from the official repository (1) | ||
```bash | ||
git fetch upstream | ||
``` | ||
|
||
2. Rebase your development branch onto the latest main branch from the upstream repository | ||
Some repositories have renamed the `master` branch to `main`. Use autocompletion or check on GitHub to confirm. | ||
|
||
```bash | ||
git rebase -i upstream/master | ||
``` | ||
|
||
During the rebase process, you may encounter conflicts if you have modified the same file that has already been changed in the main repository. **Do not panic!** Read carefully the terminal messages and follow their instructions. | ||
|
||
#### Handling Merge Conflicts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also be done from Github, close to the bottom of the PR page and can be very quick to do if the conflicts are simple - one can also click some options to take either the version in HEAD from the conflict or the introduced one directly (also possible from the command line, but maybe less known). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Never used myself. I have added the following to the end of the section, do you think it is ok? :::{admonition} |
||
For each conflict: | ||
1. Open the corresponding file. | ||
2. Look for the conflicting code marked with `>>>`, `===`, and `<<<` symbols. | ||
3. Edit the file to resolve the conflict (removing the conflict markers may be enough, but ensure the correct changes are kept). | ||
4. Save the changes. | ||
5. Inform Git that the file is ready by running: | ||
```bash | ||
git add <file_name> | ||
``` | ||
6. Continue the rebase process with: | ||
```bash | ||
git rebase --continue | ||
``` | ||
7. If necessary, repeat these steps until the rebase is complete. | ||
|
||
If something goes wrong and you need to cancel the rebase, you can run: | ||
```bash | ||
git rebase --abort | ||
``` | ||
|
||
During this process, you can also fix any commits that need modification. **Be aware that deleting commits from the list will also delete the corresponding changes.** More information can be found in the [GitHub help](https://help.github.com/articles/about-git-rebase/) and the [Atlassian tutorial](https://www.atlassian.com/git/tutorials/rewriting-history). | ||
|
||
For further details on how to avoid loss of work, **please read [this guide](https://www.atlassian.com/git/tutorials/rewriting-history)**. | ||
|
||
3. Push your local changes to your fork (2) | ||
|
||
```bash | ||
git push --set-upstream origin [NAME_OF_LOCAL_BRANCH] | ||
``` | ||
|
||
where the option `--set-upstream` sets up tracking so that future `git pull` and `git push` commands will automatically reference `origin/[NAME_OF_LOCAL_BRANCH]` | ||
|
||
### Cleaning history | ||
|
||
|
@@ -138,14 +288,7 @@ Please always follow the recommendations below. | |
- git will guide you through the steps, where you can delete entire commits (and the corresponding changes), merge commits and change commit messages | ||
- more information can be found in [this tutorial](https://www.atlassian.com/git/tutorials/rewriting-history#git-rebase-i) | ||
|
||
### Pull requests | ||
|
||
- Give a meaningful title that appropriately describes what you did (e.g. Add new calorimeter clustering) | ||
- Pull requests of work in progress (to make people aware that you are working on a feature) create a PR starting with "[WIP]" | ||
- In the description, give a short bullet-point list of what was done | ||
- If your pull request fixes issues tracked in the [issue tracker](https://github.com/HEP-FCC/FCCSW/issues): | ||
- Make sure you added a test that shows they are actually fixed | ||
- In the description mention that you fixed it by referring to the issue: "fixes #<issue-id>" (this will automatically close the issue, see also [GitHub help](https://help.github.com/articles/closing-issues-via-commit-messages/)) | ||
|
||
## Trouble-shooting | ||
|
||
|
@@ -184,6 +327,45 @@ Now you can push to that remote with: | |
git push [the remote name] [the branch you want to push] | ||
``` | ||
|
||
## TL;DR | ||
|
||
```bash | ||
# Clone your fork of the key4hep repository locally | ||
git clone [email protected]:atolosadelgado/key4hep_repo | ||
cd key4hep_repo | ||
|
||
# Create a new branch for the feature you are developing | ||
git checkout -b new_feature_branch | ||
|
||
# Modify files | ||
git add file1 file2... Readme | ||
git commit -m "new feature" | ||
|
||
# Run ctest before pushing (ideally before committing) | ||
|
||
# Push the new branch to your fork | ||
git push origin new_feature_branch | ||
|
||
# Ready to open a PR on GitHub | ||
``` | ||
|
||
If you need to rebase your development branch with the latest changes from | ||
key4hep central repository (so-called `upstream`): | ||
|
||
```bash | ||
# Add the upstream repository | ||
git remote add upstream https://github.com/key4hep/key4hep_repo.git | ||
|
||
# Fetch the latest changes from upstream | ||
git fetch upstream | ||
|
||
# Rebase your development branch onto the latest main branch (resolve conflicts if needed) | ||
git rebase upstream/main | ||
|
||
# Push updated branch (use --force-with-lease to avoid overwriting unintended changes) | ||
git push --force-with-lease | ||
``` | ||
|
||
--- | ||
|
||
## Need help? | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sentence "Note that you'll not be able to push to your repository" doesn't make much sense now in the middle of this. I think originally what was meant to go here is that if you have your keys locally you can't push from a different place without the keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! how would you change the text then?