Skip to content

PR checks

Anil Madhavapeddy edited this page Dec 1, 2017 · 43 revisions

List of manual checks before merging a PR:

Names

  • the directory structure should follow packages/$NAME/$NAME.$VERSION/
  • the package name should not contain an ocaml suffix or prefix

Description

  • the first line of descr should be a valid short description.
  • the descr file should not contain typos.
  • check that the file isn't called desc by mistake.

URLs

  • the url file should contain a stable URL (ideally a tagged version, so not git, and not sha1 archives)
  • the url file should contain a (valid) checksum

OPAM files

  • The opam file should have a valid maintainer (ideally, the name of the creator of the PR, and not contact@ocamlpro.com)
  • If it has an install field, the opam file should have a sensible remove field (if it is a library ocamlfind remove $LIB). If the build system uses .install files, that's not necessary.
  • Packagers should be encouraged to reduce the number and size of files in the files/ subdirectory of a package if possible.
  • Packagers should be encouraged to make their package's constraints as wide as possible and reduce any redundancy. For example, if package A needs package B and B needs C but A does not directly depend on C, C should not be marked as a dependency of A.
  • Most packages don't use ocamlfind or ocamlbuild or jbuilder or ppx_* at run-time and these dependencies should be marked as {build} dependencies. Suggesting this is a good idea if there are other issues with the PR but the build annotation should not be used as a blocker to merging.
  • If sh is invoked with -c to evaluate a command expression, make sure the -ex options are also present.

Travis runs

  • The Travis runs are expected to run for some time. If one of the run is successful but very short (few seconds) that's usually means that the package has been skipped. Need to report on the PR if a package is skipped on all OCaml versions, this is usually caused by version conflicts.
  • If you are the first one to check the test runs and you see an error, try to copy/paste it back on the PR with a possible explanation on how to fix it.
  • Keep an eye out for OPAM 1.2-specific warnings, or OPAM 1.1 invalid fields (they are yellow in the output)
  • Check that the right version of the package is installed (it's tedious to check that on every OCaml version, so just pick a run on recent version: 4.01 or 4.02)
  • Packages should generally either work on OS X or be marked as incompatible with darwin.

### DataKit CI

  • These run both opam1 and opam2 tests, so keep an eye out for divergent behaviour between the two runs. opam2 might select different versions due to the builtin solver for example.
  • The reverse dependency tests check for any dependency breakages, so scan through those any other packages that might be affected.
  • Failing on OpenSUSE or CentOS6 isn't a merge blocker -- those are tested but on a best-effort basis.

Interaction between mergers

  • If someone emits some doubts on a PR (by commenting, asking for clarification, etc), this someone should be the one clicking on the "merge" button.

Improving quality

Generally, improving the general quality of the repository over time is also in scope of the maintainers work.

This means:

  • Improving the general tooling: CI scripts, opam lint, camelus, etc..
  • Improving the existing metadata: fixing lint errors, fixing incorrect constraints in bulk builds, etc..
Clone this wiki locally