Improve linting

[pitag-ha]

Description of what we have

The linting process consists of the following two batches:

• std-files: checks if the following files are present
  • README
  • LICENSE
  • CHANGES
  • opam
• opam: does the following
  • runs opam lint: returns OK if and only if opam lint returns Passed (i.e. returns FAIL already if there's a warning)
  • checks if "opam field description is present"
  • checks if "opam fields homepage and dev-repo can be parsed by dune-release". It doesn't check if those are on github.
  • checks if Only returns OK, if it's on github

At the moment, you can run dune-release lint either with option std-files/opam or as a whole. Furthermore, when running dune-release distrib, all lints get run for the branch of the distrib tag. For other commands, the lints don't get run.

A couple of minor comments:

• in "opam fields homepage and dev-repo can be parsed by dune-release": what we actually check for is "... homepage OR dev-repo ...". Furthermore, if I remember correctly, those URI's are needed to publish the distribution on github (without delegates). So probably that check should only pass if they/one of them are on github.
• the check "opam field doc can be parsed by dune-release" doesn't contribute to the lint summary: if there are more failures, it doesn't sum to the number of failures; if there's no other failure, the lint returns success.

More importantly, as discussed with @NathanReb and @gpetiot:

Proposal for improvement

Instead of associating to a lint if it's a std-files-check or opam-check, it would be better to associate to the lint the commands it is relevant for. For example, the opam-file presence and the opam lint check would (probably) get all commands associated, while "opam field doc can be parsed by dune-release" would only get publish doc associated. That way, when running a dune-release command, dune-release could automatically run the relevant lints (and only those).

Furthermore, the command dune-release lint could have as optional arguments the various dune-release commands, e.g. distrib or publish, and dune-release lint X would only run the lints needed for dune-release X.

We'd have to think about in which way to deliver the result of the general dune-release lint (without optional argument). Two possible options:

1. @gpetiot had the idea of a matrix representation:

[OK] aaa                  [distrib:OK][publish:OK][opam pkg:OK][opam submit:OK]
[WARN] bbb                [distrib:OK][publish:OK][opam pkg:KO][opam submit:KO]
[KO] ccc                  [distrib:KO][publish:KO][opam pkg:KO][opam submit:KO]

where xxx are different lint checks (I've written a first command on the issue about that).

2. We could keep it quite simple:

[OK] aaa                 
[WARN] bbb (needed for command X and command Y)
[KO] ccc (needed for command X)

And of course, we could combine both things by having one as default and activate the other one through a --verbose or --silent flag, respectively.

[pitag-ha]

About the idea of a matrix representation: I like the idea for the README, where we could associate one of the values relevant or irrelevant to a pair (lint check, command):

                      distrib                 publish                 opam pkg               opam submit
aaa                      rel                    rel                      rel                    irrel
bbb                      rel                    irrel                    irrel                  rel
ccc                      irrel                  rel                      rel                    rel

I'm a bit unsure about a matrix representation for the lint output with values OK or KO as in option 1. of the issue for three reasons:

• For me, the value [command X: OK] is a bit confusing, since one wouldn't know if that means that that lint is relevant for the command. If it is relevant, the value [OK] is a necessary condition to run that command, whereas if it isn't relevant, an [OK] there is neither a condition nor an obstruction. What would you think about having three possible values: OK, KO and irrelevant?
• We'd have to make clear that an OK is just a necessary condition for a command, whereas a KO is a clear obstruction.
• It might become quite long

[NathanReb]

After discussing this amongst ourselves we realized there wasn't much value in such a matrix and that instead we identified three kind of checks dune-release is running or should be running:

1. The mandatory tarball checks: the content of the tarball must build, the test must pass and opam lint should return no errors (warnings are okay here). These checks should be run as part of distrib and the command should fail if they do as it makes no sense to release the tarball if any of these fail.
2. The project hygiene checks: this pretty much is the current lint checks, it's used to tell if the project is well maintained according to dune-release's standards, e.g. it has a readme, a changelog, a license, a warning less opam file etc... It's there to ensure the project follows the latest best practices. These are good recommendations so they should be enabled by default but should be easy to disable as they are not strictly necessary to release an opam package.
3. The "is this package dune-releasable" checks: does the repository contain all the information dune-release needs to go through a full release to github. These checks are only relevant to github users that wish to follow the standard dune-release workflow. They should be able to run them ahead of the release to make sure the repo is in order.

Proposed plan

Before 2.0

1. [ ] We can work on 3 ahead of the 2.0 release by creating a new command, dune-release check:

• It should be configurable (through a flag) to be able to run on the local state of the repo rather than on a clean clone of it (just as distrib does). That will help users prepare for their first release for instance.
• By default it should run for all local packages but should be accept a list of packages as argument to run the checks for those packages only

For the 2.0 release

1. [ ] Remove everything but the mandatory checks from the distrib command
2. [ ] Add an opam lint (allowing warnings) to the tarball checks
3. [ ] Clean up the lint command so we remove any "is this package dune-releasable" check from it
4. [ ] Run the new lint command as part of the default command (this would be disabled by passing --skip-lint)
5. [ ] Run the new check command as part of the default command

[anmonteiro]

Right now dune-release lint doesn't alert me that there's a missing license: "..." field in the opam file. I'm not sure if that should be included in dune-release or opam lint more generally.

[Leonidas-from-XIV]

@anmonteiro I agree that this is a useful thing to check but I also think that this might be more useful in the general context of the OPAM ecosystem (e.g. opam-repository), so adding it to opam lint could be more impactful than just in dune-release lint.