Reviewing PRs for Spack
- If you start at a PR page
https://github.com/spack/spack/pull/<number>, navigate to the "Files changed" tab. This will display a diff - If you hover your mouse to the left of any line in the diff you have the option to add a comment. When you enter that comment you can either add it as a single comment or choose to "start a review" which will group your comments together
- When you are finished commenting you can select the "Review changes" button which gives you the option to summarize the feedback and then approve/reject
Based on suggestions from Adam J. Stewart (and mostly copied from an email):
You don't have to test the package install. The flake8/doc tests should pass (if not, wait for the submitter to fix that before proceeding). Other failures e.g. the unit/build tests are more likely to be transient issues with the CI.
These are the PRs where you're most likely to "look for places where Spack data structures would simplify things". If you have the time, searching other package managers like Homebrew, or installation tutorials like Linux from Scratch can be very beneficial when reviewing a new package.
You may also want to check whether a package with a similar name exists - the package may have been added already! Some types of packages follow certain naming conventions, e.g. adding a prefix of py- for python packages like py-setuptools.
If the only thing the user did was add a new version, I generally merge right away. Just make sure versions are sorted from newest to oldest, and make sure they confirm that any existing patches still apply properly for this new version.
These ones require a bit more thought. Is the variant really necessary? Could we just change the default to this new flag? Could the variant work better as a multi-valued variant? If someone makes modifications to an existing package, especially one that I know is popular or that I personally use, I'm going to be more strict about waiting for 1-3 people to test the PR before I merge it.
The next most important thing when reviewing a PR is to know who to ping. For example, if someone touches a package where I know the developers or someone who frequently contributes to that package (according to git history) then I try to ping them.
Generally it's good for every patch to include the following:
- A link to where it came from (or the user can mention they wrote it themselves)
- An explanation of what the patch does
Generally the patch should be of interest to all users of the package. Sometimes a dependent package needs to build a dependency with a patch - in that case the patch should be recorded in the dependent.
- Package PRs do not need to meet coverage thresholds in CI
- If you select "view" on any file in the github diff interface, you can then select "history" or "blame" for the file, which gives all the information about who wrote what (and the commit messages hopefully provide the "why").
For packages available from pypi, the preferred URL format is
https://pypi.io/packages/source/<first letter of package name>/<package name>/<package name>-<version>.tar.gz
For example: https://pypi.io/packages/source/C/Cheetah/Cheetah-2.3.0.tar.gz is preferred over https://files.pythonhosted.org/packages/b4/18/a9b2f0f09e99024f4a9ea1ab80b6807aaecb7d1552529ea3590ad3d7bc93/Cheetah-2.3.0.tar.gz since Spack is able to substitute other versions into the first URL
For packages which fetch source from gitlab, the gitlab API URL format is preferred. See: https://github.com/spack/spack/pull/7881