search

SND-LHC / sndsw

SND@LHC experiment framework based on FairShip and FairRoot
6 stars 17 forks source link

Ana lib fix and readme #205

Closed siilieva closed 8 months ago

siilieva commented 8 months ago

Removing non-existing nestedfunctions c++ struct - resulted in a warning during the build. Add dependency to analysis_tools when building the analysis_cuts lib to prevent build issues on ubuntu. No issues were encountered on lxplus slc7/Alma9 machines even without the explicit dependency declaration. This proposed fix works on lxplus and ubuntu.

Update the README to include git pull, rebase and push instructions - a second review at these won't hurt.

olantwin commented 8 months ago

At a higher level, we should maybe consider how much detail for git is appropriate for the README.

I think I'd lean towards explaining what to do, and relegating how to do it to other locations as much as possible (e.g. sndsw wiki, twiki, software tutorial, Github and Git documentation.

E.g.

To contribute code, please make pull requests. In order to make the review as easy and quick as possible, please make sure that your branch has been rebased against master. For detailed instructions, please see bla. For a short introduction to git and recommended git config options, please see here.

siilieva commented 8 months ago

At a higher level, we should maybe consider how much detail for git is appropriate for the README.

I think I'd lean towards explaining what to do, and relegating how to do it to other locations as much as possible (e.g. sndsw wiki, twiki, software tutorial, Github and Git documentation.

E.g.

To contribute code, please make pull requests. In order to make the review as easy and quick as possible, please make sure that your branch has been rebased against master. For detailed instructions, please see bla. For a short introduction to git and recommended git config options, please see here.

Once a Twiki/github etc page like that is set up it would be easy to cut down this README. For now, I would go with this solution to minimize risk of disturbance in the master commit history.

olantwin commented 8 months ago

One comment maybe: generally for discussing changes it would be good to make individual commits. They can then later all be squashed into one in a final rebase before merging.

I had trouble following all the small changes between the different versions of this commit.

siilieva commented 8 months ago

One comment maybe: generally for discussing changes it would be good to make individual commits. They can then later all be squashed into one in a final rebase before merging.

I had trouble following all the small changes between the different versions of this commit.

true, noted! Thanks for your feedback! Btw we deserve a treat for this effort!

olantwin commented 8 months ago

One comment maybe: generally for discussing changes it would be good to make individual commits. They can then later all be squashed into one in a final rebase before merging. I had trouble following all the small changes between the different versions of this commit.

true, noted! Thanks for your feedback! Btw we deserve a treat for this effort!

Sure, treats next week at the CM?