API change review: Difference between revisions

From Qt Wiki
Jump to navigation Jump to search
(Link to QUIP 10)
(Added the manual process of generating QML API Reviews.)
 
(2 intermediate revisions by 2 users not shown)
Line 6: Line 6:
See also: {{QUIP|10}}.
See also: {{QUIP|10}}.


== Process ==
==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.
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.
Line 21: Line 21:
Once the new minor version has been released, the WIP reviews for it are abandoned.
Once the new minor version has been released, the WIP reviews for it are abandoned.


== Implementation ==
==Implementation==


Starting at Qt 5.7, the reviews are generated as follows:
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
*a branch is created off the prior release tag
:* (The aim of this is to ensure that <kbd>git</kbd> notices renames as such)
*all (maybe-API) headers of this branch that are gone in the new release are deleted
* 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 <kbd>git</kbd>'s index
:*(The aim of this is to ensure that <kbd>git</kbd> notices renames as such)
:* (Any renames should have been noticed as such.)
 
* a script performs the equivalent of a <kbd>git reset -p HEAD</kbd> that
*all of the new release's headers (that some simple checks allow might define APIs) are checked out
:* excludes any header with a comment disclaiming compatibility (the <kbd>We mean it!</kbd> comments, and similar),
*all changes relative to the prior tag are, at this point, in <kbd>git</kbd>'s index
:* excludes any simply deleted header (these shall be mentioned in the commit message) and
 
:* excludes various "boring" changes:
:*(Any renames should have been noticed as such.)
::* changes to copyright headers
 
::* common patterns in recent development (e.g. adapting to use C++11 features from 5.7 onwards)
*a script performs the equivalent of a <kbd>git reset -p HEAD</kbd> that
* if that leaves any changes to commit, they are committed.
 
:*excludes any header with a comment disclaiming compatibility (the <kbd>We mean it!</kbd> 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 <kbd>git diff -D</kbd> is entirely boring and do a preliminary review of <kbd>git show HEAD</kbd> output, prior to pushing <kbd>HEAD</kbd> to Gerrit's relevant <kbd>refs/for/</kbd> pseudo-branch.
Whoever is generating the review can now confirm that <kbd>git diff -D</kbd> is entirely boring and do a preliminary review of <kbd>git show HEAD</kbd> output, prior to pushing <kbd>HEAD</kbd> to Gerrit's relevant <kbd>refs/for/</kbd> pseudo-branch.
Line 46: Line 53:
The amended commit can then be pushed to Gerrit and appear as a new Patch Set in the original review.
The amended commit can then be pushed to Gerrit and appear as a new Patch Set in the original review.


=== Scripts ===
===Scripts===
 
The scripts to do this live in <kbd>qt/scripts/api-review/</kbd>.  When using them, it is simplest to add this directory to your <kbd>PATH</kbd>.
 
;<kbd>all-api-modules</kbd>:This is the driver script; its <kbd>--help</kbd> output includes detailed instructions.
;<kbd>api-review-gen</kbd>:This is run in each module to prepare its branch.
;<kbd>resetboring.py</kbd>:This performs the automated <kbd>git reset -p</kbd> described above.
 
==QML API Review==
In addition to this, the QML api changes also need to be reviewed.
 
The general idea is to compare the qml folders from an install of the previous version with an install of the next version. The recommended procedure is as follows:
 
First, we need to make a base qml installation. This consists of the repos qtbase, qtshadertools and qtdeclarative. Clone these twice: once for the previous and once for the next version. Then, checkout the previous and next versions.
 
Then make a build and install folder for each version. Here is an example file tree:
.
├── 6.1
│   ├── build
│   │   └── qtbase
│   ├── install
│   └── src
│      └── qtbase
└── 6.2
    ├── build
    │   └── qtbase
    ├── install
    └── src
        └── qtbase
go into the qtbase build folder and configure qtbase like this (assuming qtbase is in the src folder like above):
../../src/qtbase/configure -release -opensource -confirm-license -make tools -prefix ../../install
It needs the release option to generate the QML types correctly. The tools are also needed to compile the rest of the base installation.
 
Assuming it was configured successfully, compile and install it (ninja install, typically).
 
The rest of the modules are configured the same way: in the build folder of the module, run:
../../install/bin/qt-configure-module ../../src/<module>
Configure, compile and install the two other modules needed for the base install: qtshadertools and qtdeclarative, for both versions.
 
Now that the base installation is ready, copy it to a known location, we also need to preserve the build folder for later:
cp -r install base-install && cp -r build base-build
Now, for each module to be reviewed, make sure the clean base install is the install folder (not necessary for the first module):
cp -r base-install install && cp -r base-build build
make a folder in build, configure the module like above, compile and install. Now extract the qml folder and clean up:
cp -r install/qml <module>-qml && rm -rf install && rm -rf build
End for loop. Now you should have something that looks like this:
.
├── 6.1
│   ├── base-build
│   ├── base-install
│   ├── build
│   │   └── qtbase
│   ├── install
│   ├── module-qml
│   └── src
│      └── qtbase
...
See what the changes are from the base to the module qml:
diff -r base-install/qml module-qml
Usually, it's just one folder. Copy it to a new folder:
mkdir <module>-diff && cp -r module-qml/Qt<module> <module>-diff
Now you should have a -diff folder for each module for the previous and next versions and we can now compare them, but we do so via Gerrit, so we first need to each module (again). Note that this clone needs to have the Gerrit hooks set up correctly.
 
The clone should be on branch dev, and then a new directory should be made in it: tmpqml. We use it to store the changes of the installed qml folders. Once it is created, we need to put the change of the previous version in it:
rsync -av --delete --exclude '*.so' 6.1/<module>-diff/ <module>/tmpqml/
This may seem overkill for something that could have been done with a copy, but this command will be reused later.
 
Once the files have been copied to tmpqml, add all of them and commit. This commit should be marked as IGNORE ME and be abandoned. It only serves as a base for our interesting commit.
 
Now rerun the same rsync command above, but replace the old (6.1 in the example) version with the new version. That way, it will write its changes into tmpqml and let us make a git commit for it, which we will do.
 
This is the interesting commit, and the one which should be reviewed. It is effectively a diff between the qml files of the previous version module and the next version module. It should handle renames and other behavior correctly, as Git usually does.
 
This change is handled like the other API Change Reviews as above.
 
==Archeology==


The scripts to do this (shall, once [https://codereview.qt-project.org/157299 integrated]) live in <kbd>qtsdk/packaging-tools/</kbd>. When using them, it is simplest to add this directory to your <kbd>PATH</kbd>.
*[https://codereview.qt-project.org/157299 initial code-review] for creation of the scripts
;<kbd>all-api-modules</kbd>: This is the driver script; its <kbd>--help</kbd> output includes detailed instructions.
*[https://bugreports.qt.io/browse/QTQAINFRA-973 QTQAINFRA-973] initiated the work to write these scripts
;<kbd>api-review-gen</kbd>: This is run in each module to prepare its branch.
;<kbd>resetboring.py</kbd>: This performs the automated <kbd>git reset -p</kbd> described above.


== Archeology ==
* [https://codereview.qt-project.org/157299 initial code-review] for creation of the scripts
* [https://bugreports.qt.io/browse/QTQAINFRA-973 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.
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.

Latest revision as of 10:45, 31 August 2021

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.

QML API Review

In addition to this, the QML api changes also need to be reviewed.

The general idea is to compare the qml folders from an install of the previous version with an install of the next version. The recommended procedure is as follows:

First, we need to make a base qml installation. This consists of the repos qtbase, qtshadertools and qtdeclarative. Clone these twice: once for the previous and once for the next version. Then, checkout the previous and next versions.

Then make a build and install folder for each version. Here is an example file tree:

.
├── 6.1
│   ├── build
│   │   └── qtbase
│   ├── install
│   └── src
│       └── qtbase
└── 6.2
    ├── build
    │   └── qtbase
    ├── install
    └── src
        └── qtbase

go into the qtbase build folder and configure qtbase like this (assuming qtbase is in the src folder like above):

../../src/qtbase/configure -release -opensource -confirm-license -make tools -prefix ../../install

It needs the release option to generate the QML types correctly. The tools are also needed to compile the rest of the base installation.

Assuming it was configured successfully, compile and install it (ninja install, typically).

The rest of the modules are configured the same way: in the build folder of the module, run:

../../install/bin/qt-configure-module ../../src/<module>

Configure, compile and install the two other modules needed for the base install: qtshadertools and qtdeclarative, for both versions.

Now that the base installation is ready, copy it to a known location, we also need to preserve the build folder for later:

cp -r install base-install && cp -r build base-build

Now, for each module to be reviewed, make sure the clean base install is the install folder (not necessary for the first module):

cp -r base-install install && cp -r base-build build

make a folder in build, configure the module like above, compile and install. Now extract the qml folder and clean up:

cp -r install/qml <module>-qml && rm -rf install && rm -rf build

End for loop. Now you should have something that looks like this:

.
├── 6.1
│   ├── base-build
│   ├── base-install
│   ├── build
│   │   └── qtbase
│   ├── install
│   ├── module-qml
│   └── src
│       └── qtbase
...

See what the changes are from the base to the module qml:

diff -r base-install/qml module-qml

Usually, it's just one folder. Copy it to a new folder:

mkdir <module>-diff && cp -r module-qml/Qt<module> <module>-diff

Now you should have a -diff folder for each module for the previous and next versions and we can now compare them, but we do so via Gerrit, so we first need to each module (again). Note that this clone needs to have the Gerrit hooks set up correctly.

The clone should be on branch dev, and then a new directory should be made in it: tmpqml. We use it to store the changes of the installed qml folders. Once it is created, we need to put the change of the previous version in it:

rsync -av --delete --exclude '*.so' 6.1/<module>-diff/ <module>/tmpqml/

This may seem overkill for something that could have been done with a copy, but this command will be reused later.

Once the files have been copied to tmpqml, add all of them and commit. This commit should be marked as IGNORE ME and be abandoned. It only serves as a base for our interesting commit.

Now rerun the same rsync command above, but replace the old (6.1 in the example) version with the new version. That way, it will write its changes into tmpqml and let us make a git commit for it, which we will do.

This is the interesting commit, and the one which should be reviewed. It is effectively a diff between the qml files of the previous version module and the next version module. It should handle renames and other behavior correctly, as Git usually does.

This change is handled like the other API Change Reviews as above.

Archeology

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.