diff options
Diffstat (limited to 'doc')
| -rwxr-xr-x | doc/cli.sh | 1 | ||||
| -rw-r--r-- | doc/packaging-initial-review-checklist.md | 4 | ||||
| -rw-r--r-- | doc/packaging.cli | 714 |
3 files changed, 709 insertions, 10 deletions
@@ -57,6 +57,7 @@ function gen () # <name> --link-regex '%b(#.+)?%../../build2/doc/build2-build-system-manual.xhtml$1%' \ --link-regex '%bpkg(#.+)?%../../bpkg/doc/build2-package-manager-manual.xhtml$1%' \ --link-regex '%bbot(#.+)?%../../bbot/doc/build2-build-bot-manual.xhtml$1%' \ +--link-regex '%brep(#.+)?%../../brep/doc/build2-repository-interface-manual.xhtml$1%' \ --link-regex '%testscript(#.+)?%../../build2/doc/build2-testscript-manual.xhtml$1%' \ --output-prefix build2-toolchain- "${@}" $n.cli diff --git a/doc/packaging-initial-review-checklist.md b/doc/packaging-initial-review-checklist.md index 01b2008..6625e5f 100644 --- a/doc/packaging-initial-review-checklist.md +++ b/doc/packaging-initial-review-checklist.md @@ -4,12 +4,10 @@ - [ ] Repository in the [build2-packaging organization][build2-packaging] if third-party package. - [ ] Repository/project/package names consistent with upstream, [Debian][debian-pkgs] (see [repository name][rep-name], [package name][pkg-name]). -- [ ] If library without `lib` prefix, no clashes with executable named (see [package name][pkg-name]). +- [ ] If library without `lib` prefix, no clashes with executable names (see [package name][pkg-name]). - [ ] Uses git submodule and symlinks for upstream, submodule at correct release commit (see [upstream submodule][upstream-submodule], [upstream symlinks][upstream-symlink]). - [ ] Follows upstream layout (within reason) (see [package layout][upstream-layout]). - [ ] [Does not bundle dependencies][dont-bundle-deps]. - -- [ ] Successful builds on [queue.cppget.org][queue]. - [ ] Package archive sizes are not excessive and don't contain unnecessary files. - [ ] `manifest`: `name`/`project`/`version` values make sense. diff --git a/doc/packaging.cli b/doc/packaging.cli index 004e27e..71616d2 100644 --- a/doc/packaging.cli +++ b/doc/packaging.cli @@ -429,6 +429,10 @@ the repository root is the package root. See \l{bdep-new(1)} for details on how to initialize such a repository. In this guide, however, we will continue to assume a multi-package repository setup.| +\N|Make sure the first commit in the package repository contains no manual +changes. In other words, it should only add files as generated by +\c{bdep-new}. This is relied upon during the package review process (see +\l{#review-init-pr Create review pull request} for details).| \h2#core-repo-submodule|Add upstream repository as \c{git} submodule| @@ -4082,14 +4086,15 @@ in the source distribution preparation (see \l{#core-test-smoke-locally-dist Test locally: distribution}).| If everything looks good, then you are done: the package submission will be -reviewed and, if there are no problems, moved to \l{https://cppget.org -cppget.org}. If there are problems, then an issue will be created in the -package repository with the review feedback. In this case you will need to +moved to \l{https://cppget.org cppget.org} for further testing and review. If +this further testing or review identifies any problems with the package, then +an issue will be created in the package repository with the feedback (see +\l{#review Package Review} for details). In this case you may need to \l{#core-version-management-new-revision release and publish a version -revision} to address any problems. However, in both cases, you should first -read through the following \l{#core-version-management Package version -management} section to understand the recommended \"version lifecycle\" of a -third-party package. +revision} to address any serious problems. But before doing that (or releasing +a new version), you should first read through the following +\l{#core-version-management Package version management} section to understand +the recommended \"version lifecycle\" of a third-party package. Also, if there is an issue for this package in \l{https://github.com/build2-packaging/WISHLIST @@ -4769,6 +4774,12 @@ difficult to guarantee that this requirement is satisfied. As a result, you should refrain from making major changes, like substantially altering the build, in a revision, instead delaying such changes until the next version. +The recommendation is to limit revision changes to bug fixes and preferably +only in the package \"infrastructure\" (\c{buildfiles}, \c{manifest}, +etc). Fixes to upstream source code should be limited to critical bugs and be +preferably backported from upstream. To put it another way, changes in a +revision should have an even more limited scope than a patch release. + \h1#howto|Packaging HOWTO| @@ -5502,4 +5513,693 @@ information will need to be updated manually. In this case feel free to submit a pull request with the necessary changes or \l{https://build2.org/community.xhtml#help get in touch}. + +\h1#review|Package Review| + +Due to the complexity of packaging C/C++ software and to maintain a standard +of quality, package submissions must be reviewed by someone knowledgeable in +\c{build2} and experienced in packaging third-party software before they can +be deemed stable and moved from the \c{testing} to the \c{stable} section of +the \l{https://cppget.org cppget.org} repository. This applies to initial +package submissions, new versions, and new revisions. This chapter describes +the review process. + +First, let's recap the package transition policies from +\l{https://queue.cppget.org queue.cppget.org} to \l{https://cppget.org +cppget.org} and between the various \l{https://cppget.org/?about sections of +cppget.org}. + +All three types of submissions (initial, new version, and new revision) are +performed with \l{bdep-publish(1)} and automatically placed into +\l{https://queue.cppget.org queue.cppget.org} where they are built and tested +as archive packages in the same set of build configuration as +\l{https://cppget.org cppget.org}. + +\N|Publishing the package into the queue is the only automatic step and all +other transitions are currently performed manually after review or evaluation +by a \c{build2} core team member (though some steps may be automated in the +future).| + +When the package is published to \l{https://queue.cppget.org +queue.cppget.org}, it ends up in one of the \l{https://queue.cppget.org/?about +three sections}: \c{alpha}, \c{beta}, or \c{testing}. The destination section +is normally determined automatically by \l{bdep-publish(1)} based on the +package version: alpha semver versions go to \c{alpha}, beta \- to \c{beta}, +and the rest (as well as non-semver versions) go to \c{testing}. + +If the package published to \l{https://queue.cppget.org queue.cppget.org} +successfully builds in at least one build configuration, then it is migrated +to \l{https://cppget.org cppget.org}. When the package is migrated from +\l{https://queue.cppget.org queue.cppget.org}, it is placed into the +corresponding section of \l{https://cppget.org cppget.org}, that is, a package +from the \c{alpha} section of the former go to \c{alpha} of the latter, +\c{beta} \- to \c{beta}, and \c{testing} \- to \c{testing}. + +If the package was migrated to either the \c{alpha} or \c{beta} section, then +this is its final destination. If, however, the package was placed into the +\c{testing} section, then it stays there for some time (at least a week) to +allow further testing and review by any interested users. It is then migrated +to the \c{stable} section provided the following conditions are met: + +\ol| + +\li|The package has at least one positive review that was performed by an +experienced \c{build2} user.| + +\li|There are no serious issues reported with the package based on further +testing.|| + +If the package has no reviews, then it will remain in the \c{testing} section +until reviewed, potentially indefinitely. Likewise, if the package has a +negative review that identified blocking issues, then they must be addressed +by the package author in a revision, published to queue, and re-reviewed. +Until then the package will remain in \c{testing} but in severe cases (for +example, security vulnerability and no forthcoming fix), it may also be +dropped. + +\N|If the package has both positive and negative reviews, then the +contradictions will be reconciled by the \c{build2} core team members.| + +\N|Packages in the \c{alpha} and \c{beta} sections can also be reviewed +and a negative review may lead to the package being dropped in severe +cases.| + +For completeness, let's also mention the \c{legacy} section of +\l{https://cppget.org cppget.org}. A package that is no longer maintained +(either by upstream or by the \c{build2} project) may be moved to \c{legacy} +for two primary reasons: to signal to the users that it has serious issues +(such as security vulnerabilities) and/or not to waste CI resources on +building it. + +How are the transitions effected, exactly? Both \l{https://queue.cppget.org +queue.cppget.org} and \l{https://cppget.org cppget.org} package repositories +are backed by \c{git} repositories hosted at \l{https://github.com/cppget/ +github.com/cppget/}. Each transition described above (including the initial +submission by \l{bdep-publish(1)}) is recorded as a commit in one or both of +these repositories (study their commit histories to get a sense of how this +works). + +While all the transitions can be performed manually by moving files around and +using \c{git} directly, the \c{manage} script in +\l{https://github.com/build2/bpkg-util \c{bpkg-util}} provides a high-level +interface for the common transitions. + +Note also that the person doing a review and the person effecting a package +transition need not be the same. For security, package transitions can +currently only be performed by the \c{build2} core team members. + +With this background, let's now turn to the review process. At the outset you +may be wondering why perform it so late in the packaging process when the +final version has been released and published. Specifically, would it not be +better to review some form of work-in-progress so that any issues can be +corrected before releasing the final version? + +There are several reasons why we prefer to review the released version. +Firstly, we want to base our reviews on the build results of the final package +archives as they will be published to \l{https://cppget.org cppget.org}. +Basing reviews on anything other than that means we will need to re-examine +them when the final version is submitted. + +The second reason is consideration for the reviewer. Reviewing other people's +packaging work, often for software you personally have no interest in using, +is a thankless and often frustrating job (things tend to get frustrating when +you have to point out the same basic mistakes over and over again). As a +result, we want to make this job as streamlined as possible without +(hopefully) multiple rounds of reviews. Also, the finality of the submission +will hopefully encourage authors to try to submit finished work rather than +something incomplete, for example, in the hope that the reviewer will help +them fix it into shape. + +Having said that, nothing prevents members of the \c{build2} community from +performing ad hoc pre-reviews, for example, for packaging efforts of new +authors that require some help and guidance. + +\N|For background, this review process was heavily inspired by the Linux +kernel development. Specifically, Linux developers review proposed changes and +mark them with a number of tags, such as +\l{https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes +\c{Reviewed-by}}. Below is the relevant quote from that document that gives a +good description for what it means to offer a \c{Reviewed-by} tag and that +matches many of the aspects of this review policy: + +\b{Reviewer’s statement of oversight} + +By offering my \c{Reviewed-by} tag, I state that: + +\ol| + +\li|I have carried out a technical review of this patch to evaluate its +appropriateness and readiness for inclusion into the mainline kernel.| + +\li|Any problems, concerns, or questions relating to the patch have been +communicated back to the submitter. I am satisfied with the submitter’s +response to my comments.| + +\li|While there may be things that could be improved with this submission, I +believe that it is, at this time, (1) a worthwhile modification to the kernel, +and (2) free of known issues which would argue against its inclusion.| + +\li|While I have reviewed the patch and believe it to be sound, I do not +(unless explicitly stated elsewhere) make any warranties or guarantees that it +will achieve its stated purpose or function properly in any given situation.|| + +A \c{Reviewed-by} tag is a statement of opinion that the patch is an +appropriate modification of the kernel without any remaining serious technical +issues. Any interested reviewer (who has done the work) can offer a +\c{Reviewed-by} tag for a patch. This tag serves to give credit to reviewers +and to inform maintainers of the degree of review which has been done on the +patch. \c{Reviewed-by} tags, when supplied by reviewers known to understand +the subject area and to perform thorough reviews, will normally increase the +likelihood of your patch getting into the kernel. +| + +Before we delve into the review process, let's also discuss how one finds +packages to review. There are two common strategies: The first is to look at +the packages that you are using in your own projects and, if there are any +unreviewed versions that you would like to use, to consider reviewing them. +Alternatively, if you would like to help the \c{build2} community by reviewing +packages, pick any from the unreviewed list. For both cases you could use +\l{https://cppget.org/?advanced-search Advanced Package Search}, limiting the +result to specific packages in the first case. For example, you could search +for +\l{https://cppget.org/?advanced-search&rp=pkg%3Acppget.org%2Ftesting&rv=unreviewed +unreviewed packages in the \c{testing} section}. + +The review process for the three types of submissions (initial, new version, +and new revision) are described in the following sections. + +\N|At this stage of the \l{https://cppget.org cppget.org} repository +evolution, where it primarily contains third-party packages, we are only +concerned with reviewing the build and packaging support, ignoring everything +else (source code, documentation, etc). This, however, may change in the +future.| + + +\h#review-init|Reviewing initial package submission| + +A review process for an initial package submission is the most elaborate since +we need to review everything from scratch. + + +\h2#review-init-issue|Create review issue| + +The first step in reviewing the initial package submission is to create a +review issue on the package repository. The issue title should be in the +following form (here and below \c{X.Y.Z} is the version being reviewed): + +\ +Review of the `X.Y.Z` version (initial package submission) +\ + +\N|Before actually creating the issue you may also want to check if someone +else is already reviewing this package and thus has already created the issue. +While there is nothing wrong with having multiple reviews, you may want to +consider picking something else to review in order to increase coverage.| + +For issue description, copy and paste the contents of +\l{https://raw.githubusercontent.com/build2/build2-toolchain/master/doc/packaging-initial-review-checklist.md +packaging-initial-review-checklist.md}. + +Then create the issue. + + +\h2#review-init-pr|Create review pull request| + +The only place where GitHub supports code reviews is a pull request (PR). As a +result, we create a dummy draft pull request against the \c{master}/\c{main} +branch whose only purpose is to serve as a code review vehicle. The +procedure is as follows: + +\ol| + +\li|Clone the package repository (referred to as \c{<repo>}) locally: + +\ +git clone --recurse-submodules --shallow-submodules git@github.com:build2-packaging/<repo>.git +cd <repo> +\ + +| + +\li|Find the first commit: + +\ +git rev-list --all | tail -1 +\ + +Make sure it is the canonical \c{Initialize package repository} commit from +the \l{#core-repo-init Initialize package repository with \c{bdep\ new}} step: + +\ +git log -p \"$(git rev-list --all | tail -1)\" +\ + +\N|The changes made in this commit will not be part of the review so we need +to make sure nothing was lumped into it beside the project infrastructure +created by \c{bdep-new}. We have to skip this commit because the two branches +we will be creating the pull request from (see below) must have common +history.| + +| + +\li|Create the review branch: + +\ +git branch review-X.Y.Z \"$(git rev-list --all | tail -1)\" +git push origin review-X.Y.Z +\ + +| + +\li|Create pull request: + + \ol| + + \li|\nOpen the GitHub link printed by the above \c{git-push} command.| + + \li|\nChange \c{base:} branch to \c{review-X.Y.Z}, \c{compare:} branch to + \c{master}/\c{main}.| + + \li|\nFor PR title use: + + \ + Dummy draft pull request for version `X.Y.Z` review (do not merge) + \ + + | + + \li|\nIn PR description link to the review issue (\c{<N>} is the review + issue number): + + \ + See review issue #<N>. + \ + + | + + \li|\nChange the creation mode to \c{Create draft pull request} and create + the PR.|| + +|| + +\N|The review pull request is setup as if we wanted to merge the +\c{master}/\c{main} branch into \c{review-X.Y.Z}, which generally doesn't make +much sense (and is the reason why this PR should never be merged). However, +this setup allows us to do code reviews of all the changes since the first +commit. What's more, if the package author addresses some issues by releasing +revisions, the revision commits will be automatically added to this PR +and their changes available to further review.| + + +\h2#review-init-checklist|Go through review checklist| + +Go through the review checklist in the review issue ticking off successfully +completed items. + +While reviewing an item you may identify an issue or have something to note. +A note is a general observation that is worth mentioning to the package +author. For example, while checking this item: + +\ +[ ] If library without lib prefix, no clashes with executable names +\ + +You may note that while there are no clashes with executables installed in +\c{PATH} locations, there is a package in Debian that has a private executable +named like this. So you may make a note along these lines: + +\ +There is a file in Debian named ftest. It's not an executable installed +in a PATH location so I guess having this library named without the +lib prefix is technically allowed, though not ideal. Consider in the +future to follow the recommendation in the packaging guide and name +libraries with the lib prefix to sidestep such considerations. +\ + +An issue can be blocking or non-blocking. As the name suggests, a blocking +issue fails the review and must be addressed in a revision. A non-blocking +issue does not fail the review and can be optionally addressed in a revision +or in the next version. + +Whether an issue is blocking or not is a judgment call of the reviewer (which +is one of the reasons why a reviewer needs to be knowledgeable in \c{build2} +and have experience packaging third-party projects). The overall guideline is +to consider two dimensions: severity and impact. An issue that would prevent a +large proportion of users from using the package is most likely +blocking. Also, conceptual issues, like +\l{https://github.com/build2/HOWTO/blob/master/entries/compile-options-in-buildfile.md +using compile/link options that should not be specified in \c{buildfiles}}, +are always blocking. Finally, also consider whether it will be possible to fix +the issue later without breaking backwards-compatibility. For example, +renaming the package once it's published will be disruptive. If you are unsure +whether an issue should be considered blocking, contact other reviewers to +discuss. + +A note or an issue can be communicated to the package author in two ways: you +can add it to the outcome comment for the review issue (created in the +following step) or you can add it as a code review comment in the review +PR. + +The first way is more appropriate for general notes (like the example above) +and issues (like missing \c{README} file). While code review comments work +best when the issue is with a specific code fragment that can be pointed to. + +To add code review comments, go to the review PR created in the previous step, +select the \"Files changed\" tab, and start the code review. For each comment, +specify whether it is a blocking issue, a non-blocking issue, or a note. + + +\h2#review-init-outcome|Add review outcome comment| + +Once you are done with the checklist, add a comment to the review issue with +the outcome to notify the package author. + +If the review was successful (no blocking issues), start the comment with the +following paragraph (here \c{<AUTHOR>} is the package author's user name on +GitHub): + + +\ +@<AUTHOR> Thank you for your submission! This version has passed the +review. Below you will find a list of non-blocking issues and notes +that have been identified during review. You can address the issues +with a revision if you wish or, alternatively, in the next version. +\ + +Adjust the last two sentences accordingly if there are no issues/notes. + +If, however, there are blocking issues, start it with the following: + + + +\ +@<AUTHOR> Thank you for your submission! Unfortunately there are +blocking issues and this version has failed the review. The list +of blocking issues is provided below. Please consider addressing +them (as well as the non-blocking issues, if you wish) in a +revision and publishing it to continue this review. +\ + +Adjust the last sentence accordingly if there are no non-blocking issues. + +Then continue the comment with a list of blocking issues, non-blocking issues, +and finally notes (remove sections that have no items): + +\ +Blocking issues: + +... + +Non-blocking issues: + +... + +Notes: + +... + +\ + +If one or more issues or notes are captured as code review comments, then add +a link to the review PR. Otherwise, describe them in the comment. For example +(here \c{<NUM>} is the review PR number): + +\ +Blocking issues: + +* A number of blocking issues described in the code review: #<NUM> + +Non-blocking issues: + +* A number of non-blocking issues described in the code review: #<NUM> + +Notes: + +* There is a file in Debian named ftest. It's not an executable installed + in a PATH location so I guess having this library named without the + lib prefix is technically allowed, though not ideal. Consider in the + future to follow the recommendation in the packaging guide and name + libraries with the lib prefix to sidestep such considerations. +\ + + +\h2#review-init-success|Finish successful review| + +If the review is successful and once the outcome comment has been added, edit +the issue description and remove the first sentence that says the review is in +progress. + +Also close (note: not merge) the review PR and, if there are no non-blocking +issues, the review issue. + +\N|If there are non-blocking issues, then we leave the review issue open as a +reminder to the package author to address them in the next version.| + +Finally, send a notification email to \c{review@cppget.org} as described in +\l{#review-init-email Send review notification email}. + + +\h2#review-init-unsuccess|Continue with unsuccessful review| + +If the review is unsuccessful and once the outcome comment has been added, +send a notification email to \c{review@cppget.org} as described in +\l{#review-init-email Send review notification email}. + +Then wait for the package author to address blocking issues and publish a +revision (which will be reflected in the review PR). Also watch out for any +questions in the review issue or code review comments in the PR. + +Once the revision is published, re-review the relevant changes and confirm +they address blocking issues. Check off any outstanding items in the review +checklist and also note which non-blocking issues were addressed for the new +outcome comment. + +Then continue from the \l{#review-init-outcome Add review outcome comment} +step by adding a new outcome comment. If all the blocking issues were +addressed and no new blocking issues were identified, then the outcome is +successful. Otherwise, it is unsuccessful and another review round is +required: wait for another revision/comments, re-review, add another +outcome comment, etc. + + +\h2#review-init-email|Send review notification email| + +In case of both successful and unsuccessful reviews, send an email to +\c{review@cppget.org} in order to notify the \c{build2} core team about the +outcome. The requirements for this email are as follows: + +\ul| + +\li|\nThe \c{From} field should contain your real name. Your review is a +statement of oversight and without a real name it doesn't have much value.| + +\li|\nThe \c{Subject} filed should be in the following form: + +\ +Review <PROJECT> <VERSION> +\ + +Here \c{<PROJECT>} is the project name to which the reviewed package or +packages belong and \c{<VERSION>} is their version. Note that all the +packages in a multi-package project must be reviewed together.| + +\li|\nIn the body of the email include the following information: + +\ul| + +\li|List of packages reviewed.| + +\li|Review outcome: \c{pass} or \c{fail}| + +\li|Link to the outcome comment in the review issue.||| + +| + +For example: + +\ +From: John Doe <john@example.org> +Subject: Review spdlog 1.14.1+2 + +Packages reviewed: + +spdlog +spdlog-tests +spdlog-bench + +Review outcome: fail + +Outcome link: + +https://github.com/build2-packaging/spdlog/issues/1#issuecomment-123 +\ + +\N|For a successful review of a new revision or version of an existing package +that has no non-blocking issues or notes, there may be no review issue (see +\l{#review-new-ver Reviewing new version submission} for details). In such +cases the outcome link may be omitted.| + +\N|The review outcome is recorded in the package metadata that is stored in +the backing \c{git} repository. See \l{brep#package-review-manifest Package +Review Manifest} for details.| + + +\h#review-new-ver|Reviewing new version submission| + +\N|The following discussion assumes that you have read through \l{#review-init +Reviewing initial package submission}.| + +The extent of changes to the build and packaging support in a new version can +range from no changes, to only minor changes, to a complete rewrite. As a +result, the review procedure for a new version varies depending on the changes +and broadly consists of the following three alternatives: + +\ol| + +\li|If there are no substantial changes and no issues (blocking or +non-blocking), then we can skip creating the review issue and go straight to +the notification email.| + +\li|If there are no substantial changes but there are issues (blocking or +non-blocking), then we create the review issue but skip the checklist. +Creating the review PR is also optional.| + +\li|If there are substantial changes then we should use the \l{#review-init +Reviewing initial package submission} procedure to re-review the package from +scratch with the checklist.|| + + +\h2#review-new-ver-changes|Determine the extent of changes| + +In order to select the appropriate review procedure we need to determine the +extent of the changes in the new version compared to the previous version, +which we will refer to as the \"base version\". + +\N|The previous version on which we are basing this review needs to be already +reviewed. Failing that, reviewing the difference doesn't make much sense. If +the immediately preceding version is not reviewed, you have two choices: +either review it first or base your review on an earlier, already reviewed +version. If its review is unsuccessful, then you will need to pay attention +to the issues identified in the previous review.| + +The recommended next step is to get a sense of the changes by examining the +difference between the base and the new versions. This can be done in several +ways. You could clone the package repository locally and use your favorite +\c{git} tool (\c{git-diff}, \c{gitk}, etc) to view the cumulative changes +between the two release commits. Alternatively, you can use the GitHub +commit comparison support to compare the two release tags: + +\ +https://github.com/build2-packaging/<project>/compare/v1.2.0...v1.3.0 +\ + +\N|Because the source code for the package comes from a \c{git} submodule, the +changes that we see will be conveniently limited to the build and packaging +support plus the related documentation.| + +Study the changes and determine which review procedure is appropriate. While +all the consideration described in \l{#review-init Reviewing initial package +submission} (and its checklist) apply to new versions, additional attention +must be paid to backwards compatibility. Unfortunately, it's not uncommon for +inexperienced package authors to break backwards compatibility at the build +level (for example, by renaming exported targets, configuration variables, +etc) even though the upstream respects its backwards compatibility obligations +as signaled by the version. + +The common case when reviewing a new version is no changes to the build and +packaging support other than the version increment in \c{manifest}. If that's +the case or you don't see any issues with other changes, then you can proceed +directly to \l{#review-init-email Send review notification email}. In this +case you can omit the outcome link. + +\N|Another common case that can use the same shortcut is when the upgrade to +the new version was contributed as a PR by someone other than the package +author. If such a PR was reviewed and merged by the package author, then this +same review can also count as a package review and the package author can +\l{#review-init-email Send review notification email} using the PR as the +outcome link.| + +At the other, thankfully more rare, extreme you may find the package +substantially changed or completely rewritten. This, for example, can happen +in response to a major version release if upstream decides to re-architect +their source code layout. But it can also be the result of more benign +changes. For example, if upstream adds a dependency on a testing framework in +its tests, then the \c{build2} package will need to split the tests into a +separate package. In case of such substantial changes it is recommended to +follow the \l{#review-init Reviewing initial package submission} procedure. + +With the two extremes covered, this leaves the case of some changes that have +issues, blocking or non-blocking. In this case the next step is to create the +review issue. + + +\h2#review-new-ver-issue|Create review issue| + +If the changes in the new version have some issues, then we create the review +issue on the package repository. The issue title should be in the following +form (here and below \c{X.Y.Z} is the version being reviewed): + +\ +Review of the `X.Y.Z` version +\ + +\N|Before actually creating the issue you may also want to check if someone +else is already reviewing this package and thus has already created the issue. +While there is nothing wrong with having multiple reviews, you may want to +consider picking something else to review in order to increase coverage.| + +Unlike the initial package submission, here we don't use the review checklist +since most items won't apply (but you are welcome to refer to it if you +like). Instead, you can put your feedback directly in the issue description, +similar to \l{#review-init-outcome Add review outcome comment}. + +If you find it helpful, you can also create a review pull request similar to +\l{#review-init-pr Create review pull request}. In this case use the base +version's release commit as the starting commit for the review branch. + + +\h2#review-new-ver-success|Finish successful review| + +If the review is successful (no blocking issues), close (note: not merge) the +review PR if one was created and, if there are no non-blocking issues, the +review issue. + +\N|If there are non-blocking issues, then we leave the review issue open as a +reminder to the package author to address them in the next version.| + +Finally, send a notification email to \c{review@cppget.org} as described in +\l{#review-init-email Send review notification email}. + + +\h2#review-new-ver-unsuccess|Continue with unsuccessful review| + +If the review is unsuccessful, send a notification email to +\c{review@cppget.org} as described in \l{#review-init-email Send review +notification email}. + +Then wait for the package author to address blocking issues and publish a +revision (which will be reflected in the review PR, if any). Also watch out +for any questions in the review issue or code review comments in the PR. + +Once the revision is published, re-review the relevant changes and confirm +they address blocking issues. Also note which non-blocking issues were +addressed for the outcome comment. + +Then continue from the \l{#review-new-ver-issue Create review issue} step +except this time adding your feedback as an outcome comment rather than in the +issue description. If all the blocking issues were addressed and no new +blocking issues were identified, then the outcome is successful. Otherwise, it +is unsuccessful and another review round is required: wait for another +revision/comments, re-review, add another outcome comment, etc. + + +\h#review-new-rev|Reviewing new revision submission| + +The procedure for reviewing a new revision submission is essentially the same +as \l{#review-new-ver Reviewing new version submission} (including sending the +notification email). However, there are the additional requirement of the +revision containing only minor changes and being strictly backwards-compatible +with the version it replaces. See \l{#dont-big-changes-revision Don't make +extensive changes in a revision} for background and details. + " |