API change review

Prior to each new minor version release (\d+\.\d+\.0), we do an API change review. The primary concern of this is to check that we keep our compatibility promises. It also serves as a good moment for the owners of technical previews to initiate full API design reviews. See also: QUIP 10.

Process

Each module's API change review is managed as a Gerrit code-review, in which the API headers of the imminent release are compared with those of the previous minor release. The review is presented as if it were a change to the release branch, even though all the changes in it are in fact already in the release branch. (This should, along with tagging the reviews as WIP, reduce the hazard of anyone actually merging one.) Gerrit duly displays what's changed, helping reviewers to focus their attention where it's needed. It also keeps track of comments on those changes and ensuing discussions.

If reviewers find compatibility issues, they are expected to open bugs and set priority. These issues are typically release blockers, so get priority 0; the submitter should also set the Fix Version(s) field to the imminent release, to enable the release team to track such issues.

Commenting on the review gives the bug report something to refer to; but is not sufficient in its own right (not even with a −2 review score). Once an issue is resolved on the release branch, the review can be updated. Once the new minor version has been released, the WIP reviews for it are abandoned.

Implementation

Starting at Qt 5.7, the reviews are generated as follows:

• a branch is created off the prior release tag
• all (maybe-API) headers of this branch that are gone in the new release are deleted

• (The aim of this is to ensure that git notices renames as such)

• all of the new release's headers (that some simple checks allow might define APIs) are checked out
• all changes relative to the prior tag are, at this point, in git's index

• (Any renames should have been noticed as such.)

• a script performs the equivalent of a git reset -p HEAD that

• excludes any header with a comment disclaiming compatibility (the We mean it! comments, and similar),
• excludes any simply deleted header (these shall be mentioned in the commit message) and
• excludes various "boring" changes:

• changes to copyright headers
• common patterns in recent development (e.g. adapting to use C++11 features from 5.7 onwards)

• if that leaves any changes to commit, they are committed.

Whoever is generating the review can now confirm that git diff -D is entirely boring and do a preliminary review of git show HEAD output, prior to pushing HEAD to Gerrit's relevant refs/for/ pseudo-branch.

The result aims to contain only the changes that are worth a reviewer's time to actually look at, with a minimum of irrelevant distractions to dull the critical faculties of those looking to spot ways we might have broken compatibility promises.

If compatibility issues are found and fixed, the scripts do support re-using the branch with the original commit to prepare an update, which can amend the earlier commit suitably for review. (The branch is based on the earlier release tag, for all that it's handled as if it were a change request for the release branch; so there should be no need to rebase it following changes.) The amended commit can then be pushed to Gerrit and appear as a new Patch Set in the original review.

Scripts

The scripts to do this live in qt/scripts/api-review/. When using them, it is simplest to add this directory to your PATH.

all-api-modules

This is the driver script; its --help output includes detailed instructions.

api-review-gen

This is run in each module to prepare its branch.

resetboring.py

This performs the automated git reset -p described above.

Archeology

• initial code-review for creation of the scripts
• QTQAINFRA-973 initiated the work to write these scripts

The last contains links to and description of the older process, in which a less-filtered diff of each module was reviewed as if it were a file to be added to the module.