diff --git a/.arcconfig b/.arcconfig --- a/.arcconfig +++ b/.arcconfig @@ -1,3 +1,7 @@ { - "phabricator.uri" : "https://phab.hepforge.org" + "phabricator.uri" : "https://phab.hepforge.org", + "repository.callsign" : "evtgen", + "arc.land.onto" : [ "master" ], + "arc.land.onto-remote" : "origin", + "arc.land.strategy" : "squash" } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,63 @@ +# Guidelines for contributing bug reports or code to EvtGen + +In order to submit bug reports or to submit new/modified code for review, you +will need an account on [HepForge](https://www.hepforge.org/register). + +Once your account is created, you should contact the +[EvtGen developers](mailto:evtgen@projects.hepforge.org) to request that your +account be added to the EvtGen project. + +## Submitting bug reports + +In the HepForge Phabricator instance, bug reports are handled by the +[Maniphest](https://phab.hepforge.org/maniphest/) application. +Please follow the instructions in [Working with Maniphest Tasks](dev-docs/Tasks.md), +making sure to use the *Bug reports* column on the +[EvtGen Workboard](https://phab.hepforge.org/project/board/135/). + +## C++ standard and coding conventions + +New EvtGen code can assume the use of the C++ 17 standard. + +We will be introducing certain static analysis checks (using `clang-tidy`). +These will be documented here as they are introduced. +Almost certainly they will include modernisation checks such as preferring use of: +* `nullptr` +* uniform initialisation +* override +* etc. + +As such, any new code should try to conform to these already. + +## Code formatting, naming conventions, etc. + +EvtGen uses a `clang-format` style file for code formatting. +Please run `clang-format` on all files that you have modified/created. + +EvtGen is licenced under the GNU GPL v3 (or later) and as such the appropriate +copyright header should appear at the top of each source file. + +The naming conventions in the code have not been very strictly applied in the +past and so there is currently some variation throughout the source code. +We are attempting to gradually unify the naming of all classes, functions, +variables, etc. and update the guidelines here accordingly. +The current guidelines are: +* All class/struct/namespace/enumeration names should be UpperCamelCase and + should start with the Evt prefix +* All enumeration states should also be UpperCamelCase +* All function and variable names should be lowerCamelCase +* All member variables should be prefixed with `m_` + +As specified in [T105](https://phab.hepforge.org/T105), the use of inclusive +language is preferred and encouraged in the EvtGen project. +New code is encouraged to adopt gender-neutral terminology as much as possible. +Existing gendered terminology will gradually be migrated to more neutral terms. + +## Additional checks before submitting code for review + +* Have you documented your changes in the History.md file, referencing the + appropriate Maniphest task and/or Differential revision? + +## Submitting code for review + +Follow the documented procedures for [code review](dev-docs/CodeReview.md). diff --git a/History.md b/History.md --- a/History.md +++ b/History.md @@ -11,6 +11,12 @@ === ## R02-02-0X +16 Nov 2022 Tom Latham +* T123: Provide documention of how to contribute bug reports, feature requests, new/modified code + +8 Sep 2022 Fernando Abudinen, John Back, Michal Kreps, Tom Latham, Alex Ward +* T108: Implement JSON test framework for all decay models + 9 June 2022 Michal Kreps * D84: Improve efficiency of RareLbToLll decay model for final states with e+e- pair. diff --git a/dev-docs/CodeReview.md b/dev-docs/CodeReview.md new file mode 100644 --- /dev/null +++ b/dev-docs/CodeReview.md @@ -0,0 +1,177 @@ +# Code review + +HepForge uses Phabricator, which has a slightly different philosophy than +GitHub/GitLab with regard to code review. +It envisages that you will still commit to a feature branch in your local +repository but rather than pushing that branch to the central repository +and opening a pull/merge request where the review occurs, you instead +upload a diff to the Differential tool and conduct the review within that +tool. +Only once the review is completed there you (or someone with the +appropriate permissions) can "land" your changes onto the target branch +(usually master) in the central repository. +By default this will squash all the commits in your local feature branch +and "land" them as a single commit, but this behaviour can be modified (see +below). +In order to make this process easier, the +[Arcanist](https://secure.phabricator.com/book/phabricator/article/arcanist/) +command line tool is provided. +However, while documentation for this tool is improving it is still rather +sparse or entirely missing for some features, so I try here to provide some +guidance based on my own experimentation. + +## Installing Arcanist + +The only prerequisite that I know of is the PHP command line interface, for +which there is a minimum version requirement, which is currently 5.5.0 + +Otherwise you simply do: +```sh +git clone https://github.com/phacility/arcanist.git +``` +then make sure that `arcanist/bin` is added to your `PATH` or set an alias +using the full path to the `arc` executable. + +## Procedure for reviewees + +For the most straightforward workflow you can do the following: +* Firstly, if you have not already done so, create a new task in Maniphest + to describe what you're trying to do (fix a bug, implement a new feature, + etc.) by following the instructions in + [Working with Maniphest Tasks](Tasks.md) +* Create a new local branch based on the branch to which you want to eventually + apply the changes, usually `master` +* Make your changes and commit as normal but **do not push the branch to + HepForge**! +* Once you are ready to create a Differential revision, you can run the + following commands from within your local git repository: + * `arc which ` + + This should tell you: + * which remote repository it is going to interact with + * which commits will be sent to Differential + * whether a new revision will be created or if an existing one will be + updated + + If these are all as expected then you can proceed with standard options + for the rest of the commands. + Otherwise, you will have to provide some extra options to the commands to + override the behaviour - run `arc help command_name` to get further info + (or see below for some of them). + + * `arc cover` + + This can tell you possible reviewers for the changes you've made. + + * `arc diff ` + + This will create or update a Differential revision from your local + changes. + Probably you can use this without specifying options but you might want + to use some of the following: + * `--create` forces creation of a new revision + * `--update ` forces update of a specific revision + * `--draft` create or update the revision in draft mode, which allows you + to look over your changes in the web interface before requesting review + * `--reviewers ` specify the reviewers + * `--cc ` specify other users to be cc'ed + + You will be prompted to edit an automatically generated summary + message. + One useful edit would be to mention the associated Maniphest task so + that the task and revision are automatically linked. + +* Once the Differential revision is created, take a look at it in your web + browser and, if necessary, modify its settings so that it is: + * Visible To: Public + * Editable By: EvtGen + * Tags: EvtGen + +* Once you are happy with the revision, you can select "Request Review" from + the drop-down menu at the very bottom of the page, optionally adding a + comment to the reviewers, and pressing "Publish revision" + +* The review then proceeds via the web interface, with comments and replies + and so on (see also the section below on 'Procedure for reviewers'). + If changes are requested then you can edit your code accordingly, commit, + and then call `arc diff` again (probably also best to check that things + will proceed as you expect by running `arc which` first) to update the + revision. + +* Once the revision is accepted by the reviewers it can be landed. + If you have the appropriate permissions then you can do so, following the + instructions below. + Otherwise, one of the project admins will do so for you. + +## Landing an accepted revision + +If you already have the latest state of the revision in your local repository +(usually the case if you are the author of the revision) you can proceed +immediately to run `arc land`. +Otherwise, you first need to obtain the latest state by running `arc patch`. +Both commands are described below: + +* `arc patch D12345` + + This will create a new branch in your local repository that applies the + changes in the revision `D12345`, where you should substitute the number of + the desired revision. + Prior to running the command make sure you have the target branch of the + revision (usually master) checked out and up to date. + +* `arc land [ref]` + + This will publish the changes in the accepted revision onto a branch in the + central repository. + Sometimes this can be run with no options, although using `--preview` first + is **always** a good idea. + Sometimes it will be necessary to specify the tip commit or to use the + `--revision` option, decribed below, in order to specify the commits to be + landed. + Sometimes it will be necessary to specify the remote and branch onto which + to land, using the `--onto` and `--onto-remote` options described below. + Here is a brief summary of the most useful options (run `arc help land` for + more information): + * `--preview` only prints the commits that will be published but does not + make any local or remote modifications + * `--hold` prints the commits that will be published and prepares the local + modifications to be pushed but does not actually push anything + * `--onto ` specifies the branch on the remote repository onto + which the changes should be published (usually `master`) + * `--onto-remote ` specifies the remote repository onto which + the changes should be published (usually `origin`) + * `--revision ` Land a specific revision, rather than + determining revisions automatically from the commits that are landing + * `--strategy ` defines whether to squash local commits when + publishing (the default behaviour) or whether to retain all local commits + and create a merge commit + + When editing the commit message, bear in mind if you want to resolve and + close the associated Task, in which case you can add `closes T123`. + +## Procedure for reviewers + +The review should consist of the obvious part, reading the code changes, and +also running some automated checks. +Depending on the outcome of each of these parts, you will then need to post +appropriate comments on the Differential revision and then mark the revision +either as being accepted or as requiring changes. +It may be necessary to iterate several times before accepting. + +To run the automatic checks you should: +* Get a copy of the code in a new branch in your local repository using `arc + patch`, described above +* Push that new branch into the CERN GitLab mirror repository +* The CI should then run - check the outcome of the pipeline +* Report any problems in the CI when commenting on the revision, along with any + comments you have on the code itself + +Other general points to check: + +* The changes should be described in History.md, preferrably providing a link + to the Maniphest task and/or Differential revision +* Applying clang-format (version 10.0.0) should result in no changes (should be + automatically checked in the CERN GitLab CI - otherwise check manually) +* Names of classes, functions, variables, etc. should follow the existing conventions +* Any new files should have the EvtGen copyright notice at the top + diff --git a/dev-docs/MakeRelease.md b/dev-docs/MakeRelease.md new file mode 100644 --- /dev/null +++ b/dev-docs/MakeRelease.md @@ -0,0 +1,68 @@ +# Making a release + +When the master branch is ready to release there are several checks to perform +and, depending on the outcome of those checks, some resulting actions, and +finally there are further actions required to make the tag and tar file. + + +## Pre-release checks + +The following items need to checked and, if found to need rectifying, the +appropriate changes should be made and committed to the master branch before +tagging it: + +* Make sure that the History.md file contains appropriate entries for all + changes since the previous tag +* Make sure that the History.md file contains the lines to indicate the new tag +* Make sure that the version number in the top-level CMakeLists.txt file + matches that for the new tag +* Make sure that the version number in the setupEvtGen.sh script matches that + for the new tag +* Double check that the version numbers in History.md, the top-level + CMakeLists.txt, and setupEvtGen.sh all match + + +## Tagging the repository + +To create the new tag, issue the command: +```sh +git tag -a RXX-YY-ZZ +``` +where XX, YY, and ZZ should be replaced by the appropriate major, minor, and +patch version numbers, e.g. `R02-01-01`. +You will also need to explicitly push the tag: +```sh +git push RXX-YY-ZZ +``` + + +## Create and upload the tar file + +An easy way to create the tar file is to use the `git archive` command from +within your working copy, e.g. +```sh +git archive --prefix EvtGen/RXX-YY-ZZ/ -o EvtGen-XX.YY.ZZ.tar.gz RXX-YY-ZZ +``` + +The resulting file then has to be placed in the directory: +`/hepforge/projects/evtgen/downloads` +e.g. using `rsync`: +```sh +rsync -va EvtGen-XX.YY.ZZ.tar.gz login.hepforge.org:/hepforge/projects/evtgen/downloads/ +``` +NB the ssh port on login.hepforge.org is 222, so you should have an appropriate entry in your `~/.ssh/config` file, e.g. +``` +Host login.hepforge.org + User + Port 222 + PubkeyAuthentication yes + ForwardX11 no +``` + +Make sure that the group ownership is `evtgen` and the permissions are `-rw-rw-r--.`. +To do this, ssh into `login.hepforge.org` and issue the commands: +```sh +chgrp evtgen /hepforge/projects/evtgen/downloads/EvtGen-XX.YY.ZZ.tar.gz +chmod 664 /hepforge/projects/evtgen/downloads/EvtGen-XX.YY.ZZ.tar.gz +``` + diff --git a/dev-docs/Tasks.md b/dev-docs/Tasks.md new file mode 100644 --- /dev/null +++ b/dev-docs/Tasks.md @@ -0,0 +1,32 @@ +# Working with Maniphest Tasks + +In the HepForge Phabricator instance, tasks are managed by the +[Maniphest](https://phab.hepforge.org/maniphest/) application. +The tasks associated with EvtGen are collected in the +[EvtGen Workboard](https://phab.hepforge.org/project/board/135/), where +they are organised into columns depending on the task category (bug +reports, new features or models, modernisation, documentation). + +To create a new task you can click on the arrow in the top right corner of +the column in which the task belongs and select "Create task". +In the box that pops up you need to supply: +* A title, which should be a very short summary of the task +* Optionally you can assign a user to the task +* Optionally you can set the priority +* Supply a more detailed description +* Set the visibility to Public +* Set the editability to EvtGen +* Set the tags to EvtGen +* Optionally add subscribers, e.g. evtgen Admins + +It is also now possible to quickly create a task (which can later be +fleshed-out using the web interface) using the +[Arcanist](https://secure.phabricator.com/book/phabricator/article/arcanist/) +tool (see the [code review](CodeReview.md) documentation for instructions +on installing Arcanist) by doing: +```sh +arc todo "Title of task" --project EvtGen +``` +Adding the `--browse` option will automatically open the newly created task +in a web browser. +