Proposal: Integrate Opam CI Lint Functionality into opam-publish

[punchagan]

Proposal

Recently, the opam-repository maintainers have been discussing incorporating the lint checks run as part of the opam-repository CI into the package publication tools. Running these lints locally before submitting pull requests to the opam-repository could smoothen the workflow of package authors, while also reducing the workload of the repository maintainers as well as the infrastructure.

Would the opam-publish maintainers be open to integrating this functionality and exploring how best to implement it?

Current Work

As a part of our work to improve package author-ing workflows, we have extracted some of the functionality/logic from the opam-repository CI code into a CLI sub-project, opam-ci-check, that can be used locally to run some of the CI steps, including linting.

We are currently working on extracting the lint functionality from opam-ci-check as a library with a minimal dependency cone to help ease the integration into package publication tools like opam-publish/dune-release.

Next Steps

If the maintainers are open to this idea, we would be happy to share a draft PR with an initial implementation for further discussion. We are happy to hear any advice, suggestions or concerns regarding this. Also, let us know if you'd like us to proceed differently here.

/cc @shonfeder

[punchagan]

We've wrapped up the refactor of the API to make it easy to extract the lint functionality as a separate library.

I have an initial implementation of the linting here.

[rjbou]

Thanks for the proposal. That was something that we have in mind since a long time.

We discussed it in last dev meeting, there are some things to keep in mind while implementing this feature.

opam-publish is not for the main OCaml opam repository only, it's the default to publish on ocaml/opam-repository. In that case, that default should use opam-ci-check lint, but it should remain possible to use another linter (default or external) for another repository (for example, coq/rocq opam repo uses opam-publish, maybe other ones too).

Also, it is necessary to keep synchronisation between linting used by ocaml/opam-repository and linting used in opam-publish, to avoid more work for repo maintainers. Let's say that the user have installed in its switch opam-publish with opam-ci-check version x and the ocaml/opam-repository bumped to opam-ci-check version y, adding new lints. Before opening the PR, opam-publish should warn the user to update its opam-publish install. It can be done with the information of the current version used by ocaml/opam-repository available somewhere (rest api?).

Please open the WIP PR, we'll be able to discuss that in it, and the code itself.

[punchagan]

Thank you for taking the time to discuss this proposal in the dev meeting, @rjbou !

Both the suggestions for consideration are great and I agree that it would be nice to keep them in mind while implementing this feature. The current WIP branch I have allows the linting to be done only on opam-repository, but doesn't take into account the version being used in CI. (opam-repository CI currently uses an unpublished version of opam-ci-check for the linting).

I'll open a draft PR and close the proposal issue to move the discussion closer to the code.

Thanks again for your time!

[shonfeder]

Hello! Just circling back to try to move this forward, as it would be a real benefit to us opam repo maintainers to get #166 merged.

I have one remark re:

Also, it is necessary to keep synchronisation between linting used by ocaml/opam-repository and linting used in opam-publish, to avoid more work for repo maintainers.

IMO, this is ideal but not necessary, and I think its probably not worth spending a lot of time at this point to try to achieve such a tight coupling on versions. It would be a great help to our maintenance efforts if we could ensure that packages published with opam-publish pass most lints, but whether or not the publication linting check is exactly in sync with what is running on opam CI mostly won't matter, and it will only pose a minor and marginal draw back if there happens to be some small mismatch from time to time.

In other words, I think this is a case where we get like 95% benefit without waiting for a perfect solution to version synchronization, and I think it will only make things better for our maintenance situation (and to the packaging experience).

[kit-ty-kate]

as it would be a real benefit to us opam repo maintainers to get #166 merged.

the main problem with #166 is that the package it relies on hasn't been released and thus merging it would make opam-publish unbuildable (and untested)

[shonfeder]

We will naturally release it. We were trying to get an OK on the design proposed in the PR before cutting the release, to avoid unnecessary rework. I think we didn't make that clear.

If the maintainers here are happy with the design, we are ready to cut a release.

[kit-ty-kate]

The API design is minimal so there isn't much to say, i'm fine with it.
However looking at the list of new dependencies (which i assume is listed in https://github.com/ocurrent/opam-repo-ci/blob/e31e90680c6e47cbd505fa4808070f32685381f5/opam-ci-check/lib/dune as no opam file is present in the project), there are a number of, to my eyes, superfluous dependencies (opam-client, obuilder-spec, ocaml-version, ppx_deriving, ppx_deriving_yojson) for the purpose of opam-publish which i feel would degrade the user experience of using opam-publish.

Once fixed, could you show us which commands we have to run to test this PR?

[punchagan]

The API design is minimal so there isn't much to say, i'm fine with it.

Thanks for taking the time to look at the API!

However looking at the list of new dependencies (which i assume is listed in https://github.com/ocurrent/opam-repo-ci/blob/e31e90680c6e47cbd505fa4808070f32685381f5/opam-ci-check/lib/dune as no opam file is present in the project), there are a number of, to my eyes, superfluous dependencies (opam-client, obuilder-spec, ocaml-version, ppx_deriving, ppx_deriving_yojson) for the purpose of opam-publish which i feel would degrade the user experience of using opam-publish.

Yes, we do not intend to have all these dependencies for the linting lib that we propose to integrate with opam-publish. We have a separate branch that's WIP, which we haven't spent time polishing/merging while we were discussing the possibility of this integration.

We'll clean this up and get back to you.

[shonfeder]

Yep, thanks for confirming that the API looks OK! We'll prep a release with the reduced dependency cone and update shortly :) 🙏