diff --git a/mps/.travis.yml b/mps/.travis.yml index fffc72288ac..3a9d340e5a6 100644 --- a/mps/.travis.yml +++ b/mps/.travis.yml @@ -5,9 +5,11 @@ # See design.mps.test.ci. # Some branches don't need builds. Add them here to avoid using build -# resources and unnecessary build messages. See -# . -if: NOT branch IN (branch/2023-01-11/github-ci) +# resources and unnecessary build messages. +branches: + except: + - branch/2023-01-07/pull-request-merge-procedure + - branch/2023-01-11/github-ci language: c # see . diff --git a/mps/contributing.rst b/mps/contributing.rst index e806164f653..38de05c8c6c 100644 --- a/mps/contributing.rst +++ b/mps/contributing.rst @@ -1,11 +1,14 @@ +======================= Contributing to the MPS ======================= + We are very happy to receive contributions to the Memory Pool System so that we can improve it for everyone. Review ------ + The MPS is highly engineered and rigorously controlled in order to prevent defects. This approach has lead to an extremely small number of bugs in production since its first commercial use in 1997. There are a @@ -22,26 +25,19 @@ The style guide in guide.impl.c.format_ contains basic rules for style. Licensing --------- -Prior to the 2020-05 re-licensing of the MPS under the BSD 2-clause -license, we required contributors to agree to the following -contribution agreement, so that we could continue to commercially -license the MPS and thereby fund future development. We have not yet -decided what may replace this requirement. - I grant Ravenbrook Ltd an irrevocable, perpetual, worldwide, - non-exclusive licence to do anything with [your contribution] that I - would have the right to do. This includes (but is not limited to): - - 1. reproducing it and doing any other act that is restricted by - copyright; - - 2. the right to sublicence to others the code and any derivative - work. - -A member of Ravenbrook staff may ask you to expressly (in writing) agree. -You just need to reply with “I agree.” We apologise for the inconvenience. +All contributions are deemed to have been made under the same license +as the material to which the contribution is made, unless you +expressly state otherwise in your contribution request. In nearly all +cases this is the `BSD 2-clause license +`_. You retain the +copyright to such contributions. Thank you --------- + Finally, thank you for making the MPS more useful to everyone. + +.. validated with rst2html -v contributing.rst > /dev/null +.. end diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 3c5c0f84ad5..5d83ad18b12 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -2,13 +2,13 @@ Memory Pool System pull request merge procedure =============================================== +:tag: proc.merge.pull-request :author: Richard Brooksby :organization: Ravenbrook Limited :date: 2023-01-07 :confidentiality: public :copyright: See `C. Copyright and License`_ -:readership: MPS developers -:status: draft +:readership: MPS developers, trainee integrators 1. Introduction @@ -17,133 +17,483 @@ Memory Pool System pull request merge procedure This document contains a procedure for merging a branch received via a GitHub "pull request". -This is a draft procedure based on: -- "Re: Possible MPS Help" [GDR_2014-01-09]_ -- "Memory Pool System branching and merging procedures" [GDR_2020-09-03]_. +Time to execute: -Some of the questions that need resolving are noted in square -brackets. +- first time: 2 hours +- second time: 30 minutes +- with practice: < 10 minutes -Ravenbrook is currently (2023-01) migrating the MPS project to git -(and GitHub) and this is likely to modify this procedure. +these are measurements_ taken during simple merges that passed automated tests. + +.. _measurements: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1381771818 + +Ravenbrook is currently (2023-01) `migrating the MPS project to git +(and GitHub) `_ and that +will greatly simplify this procedure. + +This procedure assumes a pull request has been received via GitHub, +but that dependency is light. The procedure can be varied for other +sources, even where the "pull request" is received by email or some +other traceable document. + +This document was created as a combination of the process improvement +steps from our Perforce-based "Memory Pool System branching and +merging procedures" [GDR_2014-01-09]_ with Gareth Rees' email +containing the working steps for git / GitHub merges, "Re: Possible +MPS Help" [GDR_2020-09-03]_ . -2. Pre-merge checklist +2. Purpose +---------- + +The purpose of this procedure is: + +1. get required changes in to the MPS + +2. protect the MPS from introduction of defects + +3. maintain and protect the permanent record of the MPS in Perforce + and Git, so that defects can be discovered, fixed, and prevented + +As with any procedure, you can vary this one to meet this purpose, but +you should probably read section "`6. Rationale`_". + + +3. Pre-merge checklist ---------------------- -#. Is there a document (issue, job) recording the problem that is - solved by the changes in the branch? +Start by making a record for the merge. Make a comment on the pull +request with a permalink to the procedure you're following (this one), +like:: + + Executing [proc.merge.pull-request](https://github.com/Ravenbrook/mps/blob/973fc087c9abff01a957b85bd17c4a2be434ae73/procedure/pull-request-merge.rst) + +The answers to the checklist questions should be "yes". If the answer +to a question isn't "yes", record that, and why (and maybe suggest +what to do about it). For example:: + + 2. An automated test case isn't feasible as it would mean + introducing broken shell scripts to GitHub and watching for CI + errors. + +When you finish the checklist, decide whether to start +`the merging procedure`_ by considering `2. Purpose`_. + +#. Is there a permanent visible document (issue, job), referenced by + the branch, recording the problem that is solved by the changes in + the branch? #. Does the branch solve the problem? -#. Is there an automated test case that demonstrates that the problem - is solved? +#. Is there an automated test case that demonstrates the problem + (without the branch) and that that the problem is solved (with the + branch)? -#. If there are interface changes, is there documentation? +#. If there changes to the `MPS interface`_, are they documented? -#. If the changes are significant and user-visible, have you updated - the release notes (``manual/source/release.rst``)? +#. If the changes are significant and user-visible, is there an update + to the release notes (``manual/source/release.rst``)? -#. Has there been a code review? +#. Has there been a code review, and only review edits since? -#. Does the branch build and pass tests on all target platforms? - [This may be validated by Travis CI. Insert details here. RB - 2023-01-07] +#. Is the merge approved by an approving review? -#. Does the branch merge cleanly into the masters? If not, consider a - catch-up merge from the masters. [This may be validated by GitHub - and/or Travis CI. Insert details here. RB 2023-01-07] + The `Ravenbrook MPS repo on GitHub`_ is set to require an approving + review and GitHub will block a push (step 8) without one. If you + push an unapproved merge to Perforce, this will cause gitpushbot to + fail, and leave Perforce and GitHub out of sync. *Do not push to + Perforce without GitHub approval.* -#. Does a merge of the branch with master build and pass tests on all - target platforms? [This may be validated by Travis CI. Insert - details here. RB 2023-01-07] +#. Has the contributor licensed their work? -[Checklist items for Customer-specific branches from branch-merge.rst -omitted for now. RB 2023-01-07] + By default, the work is licensed if the contributor has not + expressed any kind of variation on the licensing of their work from + the material to which they are contributing. (See `"Licensing" in + "Contributing to the MPS" <../contributing.rst#licensing>`_.) + + If they have expressed a variation, talk to them or get someone to + talk to them about the matter. *Do not start the merging + procedure*. + +#. Does the branch, and its merge, build and pass tests? + + CI should have run builds of both the branch, and a *trial merge* + of the pull request with master. Success by CI is a strong + indication that `the merging procedure`_ will be quick and + successful. + + Look for build results in the pull request on GitHub. Expand "Show + all checks", and look for build success messages for both the + branch and for the pull request (the trial merge). If these + results are missing, inform sysadmins that CI isn't functioning. + + You can also look for a build results in the `Travis CI build + history for the repo`_ and in the `GitHub workflows for the repo`_. + + If there is a failed build *of the branch* you should not execute + `the merging procedure`_, but talk to the contributor about fixing + the branch. + + If the branch built OK but there is a failed build *of the trial + merge*, you can still execute `the merging procedure`_ if you + believe that the failure is due to merge conflicts that you are + willing to resolve. + + If you have no build and test results for the merge, then you can + still try to execute `the merging procedure`_ if: + + #. you believe there are only merge conflicts, + #. you're willing to try to resolve those conflicts, and + #. you're prepared to test on all target platforms. + +.. _Travis CI build history for the repo: https://app.travis-ci.com/github/Ravenbrook/mps/builds + +.. _GitHub workflows for the repo: https://github.com/Ravenbrook/mps/actions + +.. _MPS interface: https://www.ravenbrook.com/project/mps/master/manual/html/topic/interface.html -3. Prerequisite steps +4. Prerequisite steps --------------------- -#. Ensure your public key is in //.git-fusion/users/USER/keys/ in - Perforce, submitting it if necessary. +These steps will only rarely need repeating. -#. Ensure your e-mail address is in //.git-fusion/users/p4gf_usermap - in Perforce and matches your Perforce user record, submitting it if - necessary. +#. You need basic competence with Git: enough to understand what the + commands in `the merging procedure`_ do. -#. Clone the mps-public repo and name the remote “ravenbrook”:: +#. If the merge has conflicts, you will need competence in using Git + to resolve merge conflicts. - git clone -o ravenbrook ssh://git@perforce.ravenbrook.com:1622/mps-public +#. If you want to vary the procedure, you will need to understand how + Perforce Git Fusion [Perforce_2017]_ connects Ravenbrook's Perforce + repository to the `Ravenbrook MPS repo on GitHub`_. -#. Add the GitHub repo as a new remote:: +#. Ensure your public SSH key is submitted in Perforce at + //.git-fusion/users/USER/keys/ - git remote add github git@github.com:Ravenbrook/mps.git +#. Ensure your e-mail address is submitted in Perforce at + //.git-fusion/users/p4gf_usermap and matches your Perforce user + record. + +#. Clone the Ravenbrook MPS GitHub repository and name the remote + "github". This will give you access to CI to build and test the + merge. (If you're an MPS developer you can use your existing + repo.) :: + + git clone -o github git@github.com:Ravenbrook/mps.git + cd mps + +#. Set your e-mail address for commits to the repo to match the one in + your Perforce user record, e.g. :: + + git config user.email spqr@ravenbrook.com + +#. Add the Git Fusion mps-public repo, which is the interface to + Ravenbrook's Perforce. :: + + git remote add perforce ssh://git@perforce.ravenbrook.com:1622/mps-public -4. Merging a development branch -------------------------------- +.. _the merging procedure: -1. Ensure that the contributor has executed the appropriate assignment - of copyright. +5. Merging procedure +-------------------- -2. Fetch the branch that you are going to merge from GitHub, for +Note: At any point before a successful "push" in step 7, this +procedure can be abandoned without harm. All work is local to your +working repo before that point. + +1. `Fetch the pull request branch`_ to a local branch using the MPS + `durable branch naming convention`_, "branch/DATE/TOPIC". + + If the branch already has a conventional name, and it's in the + `Ravenbrook MPS repo on GitHub`_ then fetch it with the existing + name, e.g. :: + + git fetch github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax + + Otherwise, if the pull request is in the `Ravenbrook MPS repo on + GitHub`_, fetch it from the pull request and give it a conventional + name, like this :: + + git fetch github pull/$PR/head:$BRANCH + + For example :: + + git fetch github pull/93/head:branch/2023-01-06/speed-hax + + (This could happen if either the pull request is from a fork or the + branch has an unconventional name.) + + If the branch to be merged is in a third-party repo, such as a fork + not on GitHub, you can fetch it using a remote, e.g.:: + + git remote add captain-contrib https://gitlab.com/captcontrib/mps.git + git fetch captain-contrib mps-speed-hax:branch/2023-01-06/speed-hax + + Double check you've got the branch name right. Using the wrong + branch naming `causes permanent pollution in the Ravenbrook + Perforce repository + `_. + +2. Optionally, let other people know that you're working on a merge + into master. Negotiate to avoid racing them to push to the master + codeline (step 7) because that will create extra merging work. + +3. Ensure your local master is up to date with Perforce:: + + git pull --ff-only perforce master + + If you get an error, then GitHub's master and Perforce's master are + in out of sync, and this procedure fails. + + Ensure your local master is not ahead of Perforce:: + + git push --dry-run perforce + + If this shows anything other than "Everything up-to-date." then + GitHub's master and Perforce's master are in out of sync, and this + procedure fails. + + [It may be possible to fix that here and now and continue. That's + a subject for a whole nother procedure that we don't currently + have. RB 2023-01-12] + +4. Merge the branch in to your local master:: + + git merge --no-ff branch/2023-01-06/speed-hax + + Edit the commit message to link it to *why* you are merging. Say + something like:: + + Merging branch/2023-01-06/speed-hax for GitHub pull request 93 + . + + Do *not* just say "pull request 93" without a link, because that + number is local to, and only valid on GitHub. Bear this in mind + for other references. Do add any other links that would increase + traceability. + + You may need to resolve conflicts. If you can't resolve conflicts + yourself, you may need to involve the original author of the + branch. If you still can't resolve conflicts, this procedure + fails. + +5. Maybe build and test locally. If either + + - the merge was non-trivial + - there has been any rebasing (see step 7) + - there were failed or missing build results from CI + + then build and test the merge result locally if possible. For example:: - BRANCH=branch/2020-08-23/walk-docs - git fetch github "$BRANCH” - -3. Make a local tracking branch:: - - git checkout "$BRANCH” - -4. Decide whether you are going to merge by fast-forward or not. This - all depends on how you want the result of the merge to appear in - Perforce. - - A fast-forward merge means that the individual commits from the - branch are replayed on top of master and so each commit from the - branch becomes a separate change in Perforce, with its own change - description. This would be appropriate when each commit stands on - its own. To make a fast-forward merge, rebase the branch on master, - resolve any conflicts, and force-push it back to GitHub:: - - git rebase ravenbrook/master - # resolve conflicts here - git push --force-with-lease github "$BRANCH:$BRANCH" - - A non-fast-forward merge makes a single merge commit with two - parent commits (the heads of master and the branch - respectively). This would be appropriate when the commits on the - branch don’t stand on their own and you want to have a single - change in Perforce. - - [This needs review, and in any case a decision procedure is needed - here. What would be the default outcome from the GitHub interface? - RB 2023-01-07] - -5. Merge the branch to master and resolve any conflicts:: - - git checkout master - # resolve conflicts here - git merge "$BRANCH” - -6. Run tests, for example:: - make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa - if you are on Linux. + See `design.mps.tests <../design/tests.txt>`_ for details and other + platforms. -7. Push master (and if non-fast-forward, the branch) to mps-public:: + If tests do not pass, review your conflict resolution from the + merge (step 4), and if that doesn't fix things, the procedure + fails, and you need to go back to the source of the branch, + e.g. the pull request and its original author. Something's wrong! - git push ravenbrook master "$BRANCH” +6. Maybe build and test using CI. As with step 5, if either -8. Wait for gitpushbot to push the result to GitHub. + - the merge was non-trivial + - there has been any rebasing (see step 7) + - there were failed or missing build results from CI + + then push the merge to a fresh branch in the `Ravenbrook MPS repo + on GitHub`_. This should trigger CI to build and testing on all + target platforms. :: + + git push github HEAD:merge/2023-01-06/speed-hax + + You will need to wait for results from CI. Look for a build + results in the `Travis CI build history for the repo`_ and in the + `GitHub workflows for the repo`_. + + See build (step 5) about what to do if tests do not pass. + +7. Submit your merged master and the branch to Perforce:: + + git push perforce master branch/2023-01-06/speed-hax + + **Important**: Do *not* force this push. + + If this fails, someone has submitted changes to the master codeline + since you started. + + You can attempt to rebase your work on those changes:: + + git pull --rebase perforce + + then go back to testing (step 5). + + Alternatively, you could undo your merging work:: + + git reset --hard perforce/master + + then go back to merging (step 4). + +8. Optionally, if and *only if* the Perforce push (step 7) succeeded, + you can also push to GitHub:: + + git push github master branch/2023-01-06/speed-hax + + If you don't do this, then within `30 minutes + `_ + check that the merge appears in `the commits in the Ravenbrook MPS + repo on GitHub `_. + + If they do not appear: + + 1. Check email for error messages from gitpushbot and resolve them. + + 2. Check (or ask a sysadmin to check) that gitpushbot is running + on Berunda and restart it if necessary, or ask a sysadmin to do + this. + +9. Eyeball the pull request and related issues on GitHub to make sure + the merge was recorded correctly. Check that any issues *not + completely resolved* by the merge were not closed. Re-open them if + necessary. + +.. _Fetch the pull request branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally + + +6. Rationale +------------ + +This section explains why the procedure is like it is. It's intended +for people who want to vary the procedure on the fly, or make +permanent changes to it. In the latter case, update this section! + +[This section should argue the case in terms of section "`2. Purpose`_". +RB 2023-01-14] + + +6.1. Why not rebase or squash merge? +.................................... + +We would like to avoid rewriting history and the destruction of +information on the grounds that it destroys information that could be +important to the engineering of the MPS, such as tracking down +defects, comprehending the intention of changes. So want to +discourage rebasing or squashing. + +We want to avoid fast-forwards of master. A fast-forward means there +is no commit that records the fact that there has been a merge, by +whom, from where, for what purpose, etc. It discards that +information. Therefore we want to discourage fast-forwards of master +in favour of merges. (Annoyingly, GitHub only provides `branch +protection that enforces the opposite +`_!) +See also `6.3. Why the "durable" branch names?`_. + +We also want to avoid `squash merges +`_. +A squash merge compresses development history into a single commit, +destroying the record of what happened during development and the +connection to the branch. + +Many developers use fast-forwards and squashes to simplify the +branching history so that it's easier to understand. Better tools and +interfaces are no doubt required for analysing Git history. These +will emerge. And they will be able to analyse the history that we are +creating today. + +There is also a strong tendency among developers to "correct" mistakes +and edit history to reflect "what should have happened" or "what I +meant to do", treating history like code. But it's the function of +version control to protect software against well-intentioned mistakes. +Git is bad at remembering changes to history (it has no meta-history) +and so we should not edit it. + + +6.2. Why not press the GitHub merge button? +........................................... + +We cannot use the GitHub pull request merge button because it would +put the GitHub master branch out of sync with (ahead of) Perforce. +Currently, Perforce is the authoritative home of the MPS, and the Git +repository is a mirror. + +According to `GitHub's "About pull request merges" +`_: + + When you click the default Merge pull request option on a pull + request on GitHub.com, all commits from the feature branch are added + to the base branch in a merge commit. + +`Travis CI builds and tests this merge in advance `_: + + Rather than build the commits that have been pushed to the branch + the pull request is from, we build the merge between the source + branch and the upstream branch. + +When we use a GitHub CI on pull requests, that's also run on the merge +results. As `GitHub's pull request event documentation +`_ +says: + + GITHUB_SHA for this event is the last merge commit of the pull + request merge branch. + +So, `once Git becomes the home +`_ we will be able to use +the button to to replace sections 4 and 5, the procedure, but not +section 3, the pre-merge checklist. We may be able to incorporate the +checklist into GitHub's interface using a `pull request template +`_. + + +.. _durable branch naming convention: + +6.3. Why the "durable" branch names? +.................................... + +It's common in Git culture to delete branches once they've been +merged [Ardalis_2017]_ but this destroys information that has been +invaluable to MPS quality in the past. + +It destroys the connection between the branch name and a series of +changes made together, intentionally, for a purpose. That makes it +hard to identify those changes together. It makes it hard to *refer* +to those changes from documents and code (referring to the hash of the +last commit is not as good). It makes it hard to investigate the +intention of changes discovered by tools such as ``git blame`` or ``p4 +annotate``. + +Essentially, it throws away history and dissolves the branch into the +big global graph of git commits. That's not good configuration +management. + +The MPS has an ongoing policy of retaining all of its intentional +history, and that includes branch names. Branch names in the MPS +repository are intended to last forever. That is why they have +"durable" names. + +This policy has persisted over decades through more than one SCM +system, and will persist when Git has been replaced by the next one. + +Note: `GitHub branch protection rules`_ are `enabled +`_ on the +`Ravenbrook MPS repo on GitHub`_ and should prevent deletion. + +.. _Ravenbrook MPS repo on GitHub: https://github.com/Ravenbrook/mps + +.. _GitHub branch protection rules: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#require-linear-history A. References ------------- +.. [Ardalis_2017] "Why Delete Old Git Branches?"; Steve Ardalis; + 2017-07-20; + . + .. [GDR_2020-09-03] "Re: Possible MPS help"; Gareth Rees; 2020-09-03; . @@ -152,16 +502,31 @@ A. References , . +.. [Perforce_2017] "HelixCode Git Fusion Guide (2017.2)"; Perforce + Software; 2017; + . + B. Document History ------------------- ========== ===== ================================================== 2023-01-07 RB_ Created. +2023-01-13 RB_ Updates after `first attempt at execution`_. +2023-01-14 RB_ Updates after `second (successful) execution`_. +2023-01-23 RB_ Adding measurements. +2023-01-25 RB_ Responding to mini-review_. +2023-01-31 RB_ Adding instructions for recording the merge. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com +.. _first attempt at execution: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1380206348 + +.. _second (successful) execution: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1381771818 + +.. _mini-review: https://github.com/Ravenbrook/mps/pull/97#discussion_r1085584810 + C. Copyright and License ------------------------ @@ -191,5 +556,5 @@ THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -.. checked with rst2html -v index.rst > /dev/null +.. checked with rst2html -v pull-request-merge.rst > /dev/null .. end