Volker Hilsheimer (10 June 2024 10:38) wrote: > We added QT_TECH_PREVIEW_API to 6.7 after 6.7.0 (because the tag > wasn’t available in time for 6.7.0) so that we can see that difference > when we remove it against from 6.8 (with the intention of informing > reviewers about that change), and that’s now completely invisible.
That's a case for special treatment for the present release; at which patch release did this new tag get added ? Do we intend that normally the M.m.0 release shall contain this tag for tech preview additions, or are they apt to be added for later M.m.p releases ? > Also header changes in 6.7 might be fixes (such as > https://codereview.qt-project.org/c/qt/qtbase/+/565633, which > shouldn’t have shown as a diff here). It's a change since 6.7 had its API change review, that I suppose has previously only been reviewed in the normal course of code review. The fact that it has also landed in 6.7 (because it's a fix) doesn't mean it's been part of a pre-release API change review. > In short, I think we should review the delta between current 6.8 and > current 6.7 (or at most latest 6.7 patch release), not 6.7.0. This means that, if someone has made an ill-advised change to public API and picked it to the stable branch, no pre-release change review shall see it. That is a significant material change to process. > If we think that we should formally review changes within 6.7 patch > cycles (ref discussion about permitting forward BiC breakage), then we > should not abuse the 6.8 header review, IMHO. The change analyser tags > changes in headers that get cherry-pick’ed with "API change > cherry-picked”, so > https://codereview.qt-project.org/q/hashtag:%22api+change+cherry-picked%22+status:merged > lists all those. I think, once we've been doing this through several minor versions, that filter is going to produce a lot of matches: and it'll be very non-obvious (in that view) which release each entry in it landed in, so I am sceptical about it being helpful in practice. It also requires a reviewer to wade through the whole list, one review at a time, rather than gathering these changes all in one place. One could, after all, say that https://codereview.qt-project.org/q/hashtag:%22needs+api-review_6.8%22+status:merged makes the 6.8 API change review redundant. While I'm sure Jani and I would be very happy to dispense with the whole API change review process, as a whole lot of work and stress, I am not convinced that argument really works. So yes, reviewing API changes relative to the last result of an API change review is going to mean re-reviewing things folk have reviewed before, but by gathering all of them in one place the process ensures those who take part in these reviews do see all of the changes between major releases. I am wary of the suggestion that we can weaken that requirement. We could, of course, potentially make the API change review produce (up to) two changes per Qt module, instead of one: the first covers 6.7.0 through 6.7.latest, the second 6.7.latest through 6.8. That's probably something I could teach the scripts to do, and it'd show your additions of tech-preview tags in the first and their removal in the second, for example. This would ensure API change reviewers do see all changes and, by splitting them up, with the first hopefully trivial, it'd reduce the second to a more manageable level. Eddy. -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
