update doc about GitHub PR cycle #162

atolosadelgado · 2025-03-03T19:16:28Z

BEGINRELEASENOTES

Update description about how to open a Pull Request in GitHub for Key4hep repositories

ENDRELEASENOTES

Any comment is more than welcome :)

Also, I do not like the references to working in lxplus, is it really a recommendation to work on lxplus UI machines?

jmcarcell · 2025-03-04T14:26:18Z

developing-fcc-software/FccSoftwareGit.md


 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)


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.

thanks! how would you change the text then?

developing-fcc-software/FccSoftwareGit.md

jmcarcell · 2025-03-04T14:30:19Z

developing-fcc-software/FccSoftwareGit.md

 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


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.

jmcarcell · 2025-03-04T14:30:51Z

developing-fcc-software/FccSoftwareGit.md


-   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.


What non-global means is not very clear.

true! Thanks for spotting it. I will replace the whole sentence by the following, is it better?
This section explains how to use a locally installed package instead of the one provided in the stack, make changes to it, and contribute those changes to the official repository.

developing-fcc-software/FccSoftwareGit.md

jmcarcell · 2025-03-04T14:34:15Z

developing-fcc-software/FccSoftwareGit.md

+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


Maybe mention ctest -j N runs N parallel, depending on the repo it can take forever with one job (k4geo...)

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...)

jmcarcell · 2025-03-04T14:36:55Z

developing-fcc-software/FccSoftwareGit.md

+
+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


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).

Thanks! Never used myself. I have added the following to the end of the section, do you think it is ok?

:::{admonition}
GitHub web interface allows to update (rebase) and solve simple merge conflicts as well, see link
:::

atolosadelgado

Thanks @jmcarcell for the review! I left unresolved some questions. I will commit the rest of the comments

atolosadelgado · 2025-03-04T14:59:34Z

developing-fcc-software/FccSoftwareGit.md


 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)


thanks! how would you change the text then?

atolosadelgado · 2025-03-04T15:04:17Z

developing-fcc-software/FccSoftwareGit.md


-   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.


true! Thanks for spotting it. I will replace the whole sentence by the following, is it better?
This section explains how to use a locally installed package instead of the one provided in the stack, make changes to it, and contribute those changes to the official repository.

atolosadelgado · 2025-03-04T15:07:15Z

developing-fcc-software/FccSoftwareGit.md

+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


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...)

atolosadelgado · 2025-03-04T15:11:17Z

developing-fcc-software/FccSoftwareGit.md

+
+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


Thanks! Never used myself. I have added the following to the end of the section, do you think it is ok?

:::{admonition}
GitHub web interface allows to update (rebase) and solve simple merge conflicts as well, see link
:::

update doc about GihHub PR cycle

4e67a26

atolosadelgado requested review from BrieucF, kjvbrt and jmcarcell March 4, 2025 07:23

jmcarcell reviewed Mar 4, 2025

View reviewed changes

atolosadelgado commented Mar 4, 2025

View reviewed changes

add Juanmi's comments

e9afe80

kjvbrt changed the title ~~update doc about GihHub PR cycle~~ update doc about GitHub PR cycle Mar 7, 2025


		- 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.


		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

update doc about GitHub PR cycle #162

Are you sure you want to change the base?

update doc about GitHub PR cycle #162

Uh oh!

Conversation

atolosadelgado commented Mar 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atolosadelgado left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!