Add check command

[pitag-ha]

The PR adds the new check command which allows the user to make sure that the whole release process will be smooth: it checks if the opam-file is dune-release compatible and - unless stated otherwise by the user via the skip options - performs the checks that are done on the distribution archive: if it builds, if it passes its tests and if it passes the dune-release lints.

For all that, the same options are taken into account as will be when releasing. I've left out the options --dist-opam, --readme and --change-log, since they are also left out in dune-release lint. But it would probably be good to make that consistent at some point..

Before the commit adding the check command there are a couple of commits improving and fixing some minor issues in that context (see commit messages).

[gpetiot]

Besides a few minor comments it looks good to me, thanks! Don't forget to add a changelog entry though

[NathanReb]

This looks good but I think there are a few things to improve and clarify. I left a few review comments but I also have things to report here.

I tried it locally on a dummy project and got the following:

[ OK ] main package dummy.opam is dune-release compatible.
[ OK ] connected to internet
[ OK ] package(s) build
[ OK ] package(s) pass the tests
[FAIL] lint of _build/.dune-release-check and package dummy failure: 1 errors.
For information on the lint errors, run `dune-release lint`.

I think the lint report should be printed here, having to run a separate command to get the results seems kind of unfriendly to the user. Is there a particular reason that made it hard to show the lint results here?

Another thing that strike me is the part about the internet connection. I think that if we want to go down that road there are better things to check like does the repo exist, does the user opam fork exists, is the token properly set and does the user can talk to the github API, do they have their ssh key properly set etc...
In a first iteration I think it is safer to stick to strictly local checks, we can always think of how to improve this further later. The current internet check probably doesn't add much value.

Also a quick note: I think the PR needs a rebase!

[NathanReb]

Why is this not taking the value of dry_run here?

There's a genuine question about what we should do about dry-run in this command but as you seem to pass it to the check_project function I assume it is used there and therefore should be used here. i.e. if we want to allow dry-run for check, it should not create the clone either here.

To be honest I don't think check should allow dry-run since it is only doing things locally and not touching versioned files.

[pitag-ha]

Good catch, thanks!

To be honest I don't think check should allow dry-run

I think the same, but lint allows dry-run and I wanted to be coherent. I guess we're going to remove dry-run from lint in 2.0.0. Is that right? Thinking about dry-run in check twice, I also think it's better to directly not have it (even being incoherent with lint for a short time). Is that what you meant? If so, I change it.

Btw, there was a similar problem with some of the command line options: --dist-opam, --readme and --change-log. I think both lint and check should have them. But lint doesn't at the moment. I decided to be coherent with lint there, too. What do you think about that?

[pitag-ha]

Is there a particular reason that made it hard to show the lint results here?

Letting the lint results get logged directly would look as follows:

[ OK ] main package dummy.opam is dummy compatible.
[ OK ] package(s) build
[ OK ] package(s) pass the tests
[ OK ] File README is present.
[FAIL] File LICENSE is missing.
[ OK ] File CHANGES is present.
[ OK ] File opam is present.
[ OK ] lint opam file dummy.opam.
[ OK ] opam field description is present
[ OK ] opam fields homepage and dev-repo can be parsed by dummy
[ OK ] opam field doc can be parsed by dummy
[FAIL] lint of _build/.dummy-check and package dummy failure: 1 errors.

In a 2-package project it would even look like this:

[ OK ] main package dummy.opam is dummy compatible.
[ OK ] package(s) build
[FAIL] package(s) pass the tests
[ OK ] File README is present.
[FAIL] File LICENSE is missing.
[ OK ] File CHANGES is present.
[ OK ] File opam is present.
[ OK ] lint opam file dummy_sub.opam.
[ OK ] opam field description is present
[ OK ] opam fields homepage and dev-repo can be parsed by dummy
[ OK ] opam field doc can be parsed by dummy
[FAIL] lint of /home/sonja/Documents/Tarides/projects/dummy and package dummy_sub failure: 1 errors.
[ OK ] File README is present.
[FAIL] File LICENSE is missing.
[ OK ] File CHANGES is present.
[ OK ] File opam is present.
[ OK ] lint opam file dummy.opam.
[ OK ] opam field description is present
[ OK ] opam fields homepage and dev-repo can be parsed by dummy
[ OK ] opam field doc can be parsed by dummy
[FAIL] lint of /home/sonja/Documents/Tarides/projects/dummy and package dummy failure: 1 errors.

So I think that the lint output would pollute the output quite a bit. But you're right that having to run a separate command if the lint fails isn't a great solution either. I figured that for now it didn't matter to much, since in 2.0.0 we should improve the lint anyways (in multi-package projects, only run the hygiene checks once since they are package independent).

What do you think about that output "pollution"? Do you think that would be ok for now (and better than having to run another commend)? Maybe I could figure out if it would be easy to indent the lint output (to make clear which check comes from where).

I think that if we want to go down that road there are better things to check like does the repo exist, does the user opam fork exists, is the token properly set and does the user can talk to the github API, do they have their ssh key properly set etc...

That's a very good point! I've removed the internet check and left it on a different branch in case at some point we want to check more things.

[NathanReb]

Can you rebase it please?

[NathanReb]

Looks good!

I just have minor comments regarding the tests but otherwise it's all 👍