Skip to content

PR checks

Jump to bottom
Shon Feder edited this page Sep 5, 2024 · 43 revisions

Opam-repo reviews guideline

IMPORTANT: This document should be read in conjunction with Policies.

PSA: if one of these checks could be done using one of the CI system, it might be worth requesting the feature in either opam-repo-ci or opam lint.

For any additions:

  • make sure the opam-repo-ci linter succeeds (exceptions apply but there shouldn't be any "errors")
  • if the package uses dune, check that they use the standard way of calling dune. The dune build, dune runtest and dune exec command must have -p %{name}% or --profile release (check in the Makefile if make is used)
  • depopts should not contain any constraints (see note in the opam manual). Exceptions apply (e.g. see opam-file-format)
  • make sure that Jane Street packages that usually use the v prefix conversion for their version name, have said v prefix.
  • check that the known dependencies have an appropriate lower-bound constraint (for instance a package changed dramatically from one version to another and the package won't work with this version of the dependency). This is now checked by the CI lower-bounds tests, however it is to be noted that this test is an approximation and might not be 100% accurate
  • check that {build} dependencies are used properly (e.g. ppx packages should not have this tag). See the opam manual for more information.
  • check that {post} dependencies are used properly (see ocaml/opam#5010)
  • if there are extra sources pointing at github commits, their url must include ?full_index=1. See #23849
  • make sure opam-version: "2.0" as unsupported versions will make opam fail (see ocaml/opam#5631). Hopefully that won’t be the case whenever the repository migrate to opam 3.0 syntax
  • make sure shell scripts use set -e or sh -e, even when called using sh -c
  • add an explicit dependency towards ocaml if missing. This makes it easier to constrain against new versions of the compiler later.
  • check that the URLs are correct and do not contain things that might change later.
  • remove fields should not be present since opam 2.0 (rare exceptions can apply if for example a file has to be modified upon uninstallation)
  • the opam file should have a valid maintainer so that we can ping or send an email to them if needed
  • if the package being added is a pre-release, make sure it has flags: avoid-version together with available: opam-version >= "2.1" (for the latter point, except for the compiler packages that have support for opam 2.0 using "ocaml-beta" {opam-version < "2.1.0"})

For new releases of already available packages:

  • check that the project is actually the same and someone is not trying to take over. If they are, make sure this is official and the previous maintainer agreed in writing publicly available.

For new packages that weren't in the repository before:

  • check the name isn't too close to an existing one and that both can be distinguished

For modifications:

  • same as the "for any additions" section, except the linter doesn’t have to succeed (though it should still be free of "errors", vs. "warnings")
  • extra care should be taken to make sure packages don’t get broken, on all supported platforms
  • If an existing package changes their archive checksum, make sure there is no other way to get the original archive (see FAQ), in which case you can upload them on ocaml/opam-source-archives and update the url in the opam file insetad. If not, or if you choose not to, check that any of the meaningful content hasn't changed and post the diff, if any, in the PR for our own records.

General steps:

  • when doing a change in a PR, ask the maintainer to return it in the opam file upstream and make sure the change has been upstreamed so that you don’t have to do the change everytime they do a release
  • 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
  • If the revdeps check failed and the failures are not known, ask the maintainer if it is expected
    • If it is expected: Open a separate PR to fix the constraint, merge it first and restart the revdeps check (click on the "rebuild" button for the Dockerfile generation in the revdeps section)
    • If it is not expected, label the PR needs reporter action and wait for a fix.
  • If a failure appears and is deemed unsolvable (unavailable system package, system library/system C compiler too old, ...) the distributions the package fails on should be added to the x-ci-accept-failures field. e.g.: x-ci-accept-failures: ["centos-8"]. The platform name to put in the field is the same as the based dockerfile used (ocaml/opam).

Some common failures:

  • if a package fails due to missing ocamlopt, we need to make them conflict with ocaml-option-bytecode-only
  • if menhir is used with dune, it will be called with the --infer-write-query flag, the necessary bound is at least "menhir" {>= "20180528"}
  • packages may fail with 
     # File "src/dune", line 7, characters 12-17:
 # 7 |  (libraries bytes))
 #                 ^^^^^
 # Error: Library "bytes" not found.
    the solution is to update the dune dependency to look like ("dune" {>= "1.4.0"} | "dune" {< "1.4.0"} & "base-bytes") and the ocaml one to ("ocaml" {>= "4.08" & < "5.0"} | "ocaml" {>= "5.0"} & "base-bytes").
  • if the error is about the Stream module being undefined, most recent packages will continue working by adding a dependency on "camlp-streams". Note that old packages may still not work and instead require the upper bound "ocaml" {<= "5.0"}
  • opam-repo-ci might tell you that dune subst shouldn't be used without {dev}. The reason why can be read about in ocaml/dune#3647
Clone this wiki locally