Skip to content

PR checks

Jump to bottom
Kate edited this page Feb 14, 2024 · 43 revisions

Opam-repo reviews guideline

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 https://opam.ocaml.org/doc/Manual.html#opamfield-depopts). Exceptions apply (e.g. see opam-file-format)
  • make sure the version number doesn’t start with v (except for janestreet packages, and packages that have already been published in opam-repo long ago and made the mistake)
    • make sure janestreet packages that use this convention (not all do), do have the 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)
    • TODO (explain how the build tag interact with opam)
  • check that {post} dependencies are used properly
  • archives should always have a checksum (exceptions apply, e.g. ocaml-variants)
  • avoid equality constraints (e.g. {= "0.9.0"}) and when possible ask the users to change their constraints to to a lower-bound constraint (>=). Equality constraints are not user friendly as they won't allow people to install the packages with another one with different constraints in the same switch.
    • however, if the new packages being added should come together, have the same versions and come from the same repository (e.g. js_of_ocaml, js_of_ocaml-lwt, ...), then encourage users to use the {= version} constraints in subpackages if the author expects to do release in a grouped manner. Otherwise it is almost a guaranty for the previous releases to break at some point in the future and for users to be puzzled why packages that come together are out of sync.
  • if there are extra sources pointing at github commits, their url must include ?full_index=1. See https://github.com/ocaml/opam-repository/pull/23849
  • make sure opam-version: "2.0" as unsupported versions will make opam fail (https://github.com/ocaml/opam/issues/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 (git branches are forbidden, exceptions apply)
  • 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
  • packagers should be encouraged to reduce the number and size of files in the files/ subdirectory of a package if possible

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
  • check that the value added by the package is sufficient
  • package names should not contain an ocaml suffix or prefix (exceptions can apply if the ocaml prefix is relevant)

For modifications:

Note for maintainers:

  • 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

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