Guidelines for maintainers

From D Wiki
Revision as of 14:20, 24 February 2021 by Berni44 (talk | contribs) (Warn about [SQUASH])
(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

This document is intended to be a guide maintainer of dlang repositories on GitHub. However, it might contain interesting details for contributors who are interested in knowing more about the inner workings of the D GitHub processes.

Quick checklist

  • Do all CI pass?
  • Are there any changes on the documentation or coverage output?
  • Is it a regression fix? (check: it should be based ontro stable)
  • Is it a non-trivial change? (check: either linkage with Bugzilla or a changelog entry)
  • Is it a new symbols? (check: has it been pre-approved by Andrei?)

This is an initial list. Please help to extend it based on your own experiences.

Review workflow (squashed commits & write access to PRs)

The ideal workflow is that a PR gets commits appended until its final approval, s.t. you only need to review the added changes.

General ideas

  • PRs should only contain a small set of changes
  • Contributors can prefix their appended commit messages with e.g. "[SQUASH]" (Please not, that this will squash all commits of a PR not only the ones marked. Additionally it is not a squash, but a fixup, which means, that commit messages get lost.)

GitHub has two features to help us here:

Commit squashing

  • All commits get squashed into one commit before the merge
  • This is enabled for all DLang repos
  • "auto-merge-squash" does squashing as auto-merge behavior

For more infos please see the official article.

Write access to PRs

  • This is an awesome feature that hasn't been used much so far
  • It allows maintainer to do those nitpicks themselves (squashing all commits, fixing typos, ...) instead of going with the usual ping-pong cycle
  • It's enabled by default for new PRs
  • If someone turned it accidentally off, it's really okay to ask him/her as this is a massive time saver

git allows to define alliases in your ~/.gitconfig: 

[alias]
pr  = "!f() { git fetch -fu ${2:-upstream} refs/pull/$1/head:pr/$1 && git checkout pr/$1; }; f"

With this git alias you can checkout any PR: 

> git pr 5150

In case you don't want to enter the branch to push to, you can use this small snippet: 

#!/bin/bash
tmpfile=$(mktemp)
repoSlug=$(git remote -v | grep '^upstream' | head -n1 | perl -lne 's/github.com:?\/?(.*)\/([^.]*)([.]git| )// or next; print $1,"/",$2')
prNumber=$(git rev-parse --abbrev-ref HEAD | cut -d/ -f 2)
curl -s https://api.github.com/repos/${repoSlug}/pulls/${prNumber} > $tmpfile
trap "{ rm -f $tmpfile; }" EXIT
headRef=$(cat $tmpfile | jq -r '.head.ref')
headSlug=$(cat $tmpfile | jq -r '.head.repo.full_name')
git push -f git@github.com:${headSlug} HEAD:${headRef}

For more details on pushing changes to a PR, please see the official article.

Auto-Merge

There are "auto-merge" and "auto-merge-squash" labels which are based on an auto-merge system that takes the status of all required CIs into account (the auto-tester tries the merge after its test passed, but doesn't look out for other CIs.) You can toggle a PR for auto-merge by simply adding this label or for the keyboard-enthusiasts: press "l", press "a" and hit enter.

Warning: this new auto-merge system is officially still WIP because: - "auto-merge"-labelled PRs aren't yet prioritized on the auto-tester - We would like to set _all_ CIs to enforced

For more details, please see its documentation.

CI

  • We are working on making the CIs more reliable. If you see a transient error, please let us know!
  • In any other case, the "red cross" on a CI has a meaning - surprise!

Here's a small summary of the CIs and their tasks:

auto-tester

  • runs autotester that automatically compiles and runs the dmd, druntime, and phobos unittests on all officially-supported platforms
  • if there are spordiadic failures, they can be deprecated by clicking on the "Deprecate" button of a PR
  • new contributors need to be approved (this can be done on the main page of the auto-tester)
  • Maintainer: @braddr
  • Support repo

DAutoTest

It builds the entire documentation and allows to preview changes. In particular

  • /library-prerelease is the DDox built on the documentation (one file per function)
  • /phobos-prerelease is the DDoc built of the documentation (one file per module)
  • for PRs to master: only -prerelease should appear in the list of changes (the documentation on /library or /phobos is built from the stable branch)
  • Maintainer: @CyberShadow


Tip: install Git Patch Viewer and set it to automatically recognize DAutoTest's difs: 

dtest.dlang.io/diff/.*

DAutoTest Git Patch Viewer.png

ProjectTester

  • A couple of selected projects are run to ensure that no regressions are introduced
  • Maintainers: @MartinNowak, Dicebot

CircleCi

  • Tests are run with code coverage enabled
  • Phobos only: Over the last weeks, we have tried to unify Phobos to share a more common coding style. The CI is in place to ensure this status quo for the future
  • Phobos only: All unittest blocks are separated from the main file and compiled separately. This helps to ensure that the examples on dlang.org that are runnable and don't miss any imports.
  • Phobos only: runs Dscanner
  • It is planned to move these checks to the ProjectTester

It should be fairly trivial to find out the regarding error here. Just click on the "CircleCi" link and open the red tab that is marked as failing and scroll down to the error message. On a posix system the CircleCi tests can also be executed locally with the style target: 

make -f posix.mak style

Code coverage

If you are too lazy to click on the annotated coverage link, you can install the browser extension which will enrich the PR with code coverage information.

Warning: unfortunately `codecov/project` is often showing "random" movement. Please see #5202 for more infos on this.

Phantom Zone

Phantom Zone is a state assigned to PRs that have value, but would require too much effort from the maintainers to revive without providing enough benefit. When sending a PR to the "Phantom Zone", please consider the following:

  1. Before sending a PR to the Phantom Zone make an effort to invest in the contribution and the contributor.
  2. Justify why the PR is being placed in the Phantom Zone
  3. Explain to the contributor how the PR can get out of the Phantom Zone

Saved replies

GitHub allows to save replies. You can set them in your GitHub settings Here are a couple of commonly used replies:

Missing changelog

This still lacks a changelog entry. Please see [the changelog folder](../tree/master/changelog) for instructions.
Hence, I added the "pending changelog" label.

Missing spec PR

This still needs a PR to the [specification](https://github.com/dlang/dlang.org/tree/master/spec) at [dlang.org](https://github.com/dlang/dlang.org). Hence, I added the label "missing spec PR".

Please refer to the [dlang.org CONTRIBUTING guide](https://github.com/dlang/dlang.org/blob/master/CONTRIBUTING.md) for instructions to build dlang.org locally. If you use Windows, don't worry, you can do your changes "blindly" and preview them at DAutoTest.

Phantom Zone

This PR entered the Phantom Zone
-----------------------------------------------


This PR has entered the [Phantom Zone](http://forum.dlang.org/post/ouuutodvhmnghzbeoqen@forum.dlang.org) as it still needs to have the reviewers' concerns addressed and rebased.

Reason for entering the Phantom Zone
----------------------------------------------------

This PR is nice, and normally I would revive such a PR if the author was no longer active. I would also revive it if it were an important bug fix or something of higher priority.  This PR, however, is just a refactoring, so I'm going to put it in the [Phantom Zone](http://forum.dlang.org/post/ouuutodvhmnghzbeoqen@forum.dlang.org) and close it for now.

How do I get this PR out of the Phantom Zone
-------------------------------------------------------------

Easy: Address the comments -> open a new PR (mention this one + short summary in the description).

Milestones

  • They are intended to show which PRs are basically ready to be shipped OR should be shipped soon
  • Please use them whenever you see nothing blocking a PR (except for a final merge decision)
  • Ideally about one week before the close of the merge window (i.e. the end of the milestone), the focus on the remaining items of the current milestone should be increased

So far it worked well in the past and present:

See also the Release process.

Dlang-Bot

The friendly Dlang-Bot is trying to automate boring tasks.

What it does atm:

  • Shows whether a PR will be part of the changelog ('X' means NO, '✔' means YES)
  • Auto-merges a PR once all required CI pass
  • Closes a Bugzilla issue if the respective PR has been merged
  • Moves a Trello card if the respective PR has been merged
  • Cancels stale Travis builds (this helps to free the Travis queue at dlang/dmd)

What is planned for the future:

  • Automatically remove "needs work" / "needs rebase" on a push event
  • Recognize common labels in the title (e.g. "[WIP]")
  • Automatically tag inactive and unmergable PRs
  • Add a "needs review" label to unreviewed PRs with passing CIs
  • Show auto-detectable warnings (e.g. regression PR that isn't targeted at 'stable')
  • <add your issue to the wish list>

Please see also its documentation for a more up-to-date list.

Changelog

There's a changelog folder for all three repos (DMD, Druntime, Phobos).

  • The key idea is to have a file per changelog entry, which has the advantage that a changelog can be added to a PR without creating merge conflicts.
  • Take advantage of this feature and don't merge a PR without a changelog entry ;-)
  • The separation in the DMD repo between "compiler changes" and "language changes" has been moved, s.t. the "changelog" repo at DMD just contains compiler changes and the changelog at dlang.org is supposed to contain language changes of the upcoming release.


Use the formal "Approve" button

  • GH formalized the review process
  • Please use a the "approve" feature instead of LGTM comment

This is important, because all PRs require an approval! So please approve before you auto-merge (it won't work otherwise).

In the same way GH also allows to attach a "request for changes" on a PR. If you have a serious remark, please use this "request a change" feature instead of a plain comment as these "request" will be nicely shown as warning at the end of a PR (GH will even block the merge of a PR until the criticized bits are fixed). Moreover "changes requested" will also be shown on the summary of all PRs and helps others when they browse the PR list.

New name additions to Phobos

  • All new symbols to Phobos should be pre-approved by Andrei (@andralex). Please request an review from him.
  • As Andrei gets quite a load of emails, you should email him directly.

See Also

Retrieved from "https://wiki.dlang.org/?title=Guidelines_for_maintainers&oldid=9967"