PR checks

List of manual checks before merging a PR:

Mandatory checks

Dune checks

• If one the packages being added depends on dune:
  • Check that the version constraint is correct according to their dune-project in the source repository.
  • 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)
  • Check that the dune dependency does not have the {build} tag

Dependency checks

• Check that the depopts field does not contain any constraints
• Check that the version number does not have a v prefix (except for most of JaneStreet packages)
• Check that dependency constraints to JaneStreet packages have the v prefix.
• If a PR changes some of the constraints, if there is any doubt, wait for Camelus to say it does not make uninstallable any of its revdeps. If Camelus is down or slow:
  • Either wait for the CI to finish and see if on of the check successfully installed it
  • Or run Camelus manually
• 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: if the tests pass, the bounds are ok as they are (even if some dependency does not have any).
• Equality constraints:
  • 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 and have the same versions (e.g. js_of_ocaml, js_of_ocaml-lwt, ...), then encourage users to use the {= version} constraints in subpackages. 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.
• Check that {build} dependencies are used properly (e.g. ppx packages should not have this tag)

Archives

• Archives should always have a checksum (exceptions apply, e.g. ocaml-variants)
• If an existing package changes their archive checksum, check that any of the meaningful content hasn't changed and post the diff, if any, in the PR for our own records.
• If there are extra sources pointing at github commits, their url must include ?full_index=1. See #23849

Misc

• Check that all the packages being added are in the following directory structure: packages/$NAME/$NAME.$VERSION/. If one isn't there, they won't show up in CI.
• The package name should not contain an ocaml suffix or prefix (only applies for new packages). Exceptions can apply if the ocaml prefix is relevant.
• Check that packages are in the opam 2.0 format and that no descr or url files are added. Everything should be in the opam file.
• If sh is invoked with -c to evaluate a command expression, make sure the -ex options are also present.

Minor checks

• Add an explicit dependency towards ocaml if missing.
• 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 (ideally, the name of the creator of the PR, and not contact@ocamlpro.com)
• Packagers should be encouraged to reduce the number and size of files in the files/ subdirectory of a package if possible.

How to request a change

• If the source branch is open to maintainers' change and if the fix is straight forward:
  • Use the edit option on Github and ask the user to return the fix upstream so that the same fix won't have to be dealt with again in the next release.
• Otherwise, ask the user to make the necessary change and mark the PR with either the question or needs reporter action label
• If possible, do not use Github's suggestions, as people don't return the fix upstream with this and it takes too much time to wait for a change.

CI

• 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: [
    "oraclelinux-7"
    "centos-8"
  ]
  

  The platform name to put in the field is the same as the based dockerfile used (ocaml/opam). If it is expected to stay the same, the field can be added back in the original opam file.

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..

Some common failures:

• ocaml 5: if packages fail 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 bad checksum, please check if they are still in the cache https://opam.ocaml.org/cache/md5/MD5SUM[:2]/MD5SUM (similarly for sha256 or sha512), upload them on https://github.com/ocaml/opam-source-archives and update the url in the opam file
• 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"}