Use dune-build-info to compute version number

[emillon]

This switches the handling of --version using dune-build-info instead of a script.

Here is how it works.

The version string is resolved in the following way:

• if (version) is set in (dune-project), it is used.
  This is what happens when using opam pins (through dune subst), or for released versions (through dune-release).
• otherwise, a description from git describe will be used.
  Caveat for this case: binaries under _build/default will not have this information, but dune install --prefix _install will copy a valid binary under _install/bin.

(see #454)

[emillon]

We'll discuss the implementation later but I'm curious to hear if that's compatible with your vendoring use cases @jberdine & @ceastlund?

[ceastlund]

We currently override the version information anyway, because we use neither dune nor git internally at present. Once we move from jenga to dune I suspect this will work fine for us.

[jberdine]

I'm curious to hear if that's compatible with your vendoring use cases @jberdine & @ceastlund?

As far as I can tell, yes, I think so. But in the past (with a previous version of dune-release I think) I had trouble where it would not use the build command from the opam file and would instead just invoke a generic `dune -p ... -j ...` which would fail. So I worry that this working is dependent on the changes in #1005 that I remain completely unconvinced about.

[jberdine]

Are you using version strings of the form 0.12-25-g6bdc881 in your .ocamlformat files ?

Yes, we at least currently can't use any released version.

To support that, we could match the version depending on the precision given by the user: If we use version strings of the form a.b.c-commit: • 0.12 matches any 0.12.*-* • 0.12.0 matches only 0.12.0-* • 0.12-hash match precisely one commit: 0.12.*-hash

IIUC doing that would make it impossible to e.g. depend on exactly the released 0.12 version, as the binary does not know it's commit hash (it could be built from a tarball rather than a git checkout) in order to check it against the specified version.

[Julow]

Are you using version strings of the form 0.12-25-g6bdc881 in your .ocamlformat files ?
Yes, we at least currently can't use any released version.
To support that, we could match the version depending on the precision given by the user: If we use version strings of the form a.b.c-commit: • 0.12 matches any 0.12.- • 0.12.0 matches only 0.12.0-* • 0.12-hash match precisely one commit: 0.12.*-hash
IIUC doing that would make it impossible to e.g. depend on exactly the released 0.12 version, as the binary does not know it's commit hash (it could be built from a tarball rather than a git checkout) in order to check it against the specified version.

Hashes are for dev binaries that are passed around. 0.12.0 would match only one possible version for opam users. For users who built the binary, any build between 0.12.0 and 0.12.1 (excluded) would be allowed, on purpose.

[jberdine]

For users who built the binary, any build between 0.12.0 and 0.12.1 (excluded) would be allowed, on purpose.

If you want an imprecise version check, then it should be added in addition to the existing precise version check, rather than losing the current functionality. It is currently possible for an .ocamlformat file to specify a released version like 0.12 and ensure that an ocamlformat binary from a checkout a few commits ahead cannot mistakenly be used. It should not be necessary to use an unreleased version in order to make the version checking precise enough to fail on anything but a binary built from the expected commit. It is important to catch the mistake of specifying a released version and using a binary built from a checkout that isn't at exactly the released commit. I have actually been considering adding the compiler version and ppxlib version as well, as they can change the output even for the same ocamlformat commit. This discussion makes me think that there are several different use case objectives, but I'm only familiar with the one that tries to ensure reproducible formatting.

[Julow]

I see. Allowing a range of versions may not be a good idea because even a very small regression or bug fix can change formatting.
For the compiler/OMP issue, we should test that and increase the constraints if necessary.

[jberdine]

For the compiler/OMP issue, we should test that and increase the constraints if necessary.

In the past we have seen e.g. a compiler upgrade result in the parser producing slightly different locations, which changed comment attachment, and as a result the same ocamlformat commit built with 4.05 and 4.06 (if I remember the versions correctly) would produce different output in some cases. I suspect that changes can also happen if versions of ppxlib which internally use different versions of the parsetree are used.

[jberdine]

It would be nice to revive this to simplify the version management and implement it in a more common way. Is there anything blocking this now?

One addition I would like to see to help testing and development if nothing else, is a version-stamped binary in a stable/reliable path. Perhaps something like a make target implemented using something like:

(executable
 (name ocamlformat)
 (promote until_clean)
 ...)

[emillon]

It seems that there are two distincts problems here:

• where to get info about the current binary (is it a release one, does it come from git, etc)
• what is the semantics of version numbers

For the second one, it seems that there's a distinction to be made between the tool version (release vs git vs vendored vs patched) and the behavior version (the thing that is checked by version=). This PR is about the first one; I opened #1363 to suggest the beginning of a solution to the second one, but that's not the whole story.

[jberdine]

I think that this is almost ready. Could you rebase and run make headers? Also, it would address the caveat currently noted in Version.mli if this PR added dune clause to promote the binary into the source tree. I think that should be done as part of this PR. The question of how to get a version number to automatically always change other than from git should be left for another PR.

[gpetiot]

I rebased and updated the headers. But dune install --prefix _install does not set the correct version for me (only unknown)

[emillon]

do you mean when running the binary that's copied under _install ?

[gpetiot]

Yes this binary returns an unknown version.

[gpetiot]

The stamping issue I reported happens on linux. But I didn't observe the dune warning you got.

[jberdine]

This will need to add a version constraint for dune > 2.6.0 to make sure to pull in ocaml/dune#3599.

[gpetiot]

I rebased and added the constraint on the dune-build-info version, there is still a quirk causing the version number to be 0.14.1-... instead of 0.14.2-... but it's because I messed the last release.

[jberdine]

there is still a quirk causing the version number to be 0.14.1-... instead of 0.14.2-... but it's because I messed the last release

Have you git pulled the tags recently? As far as I can tell, I fixed that a bit ago with some retagging.

[emillon]

Have you git pulled the tags recently? As far as I can tell, I fixed that a bit ago with some retagging.

You pushed a lightweight tag and not an annotated tag (produced by git tag -a). dune-build-info expects this kind of tags, compared to what gen_version.mlt used to do. (dune-release will do the right thing)

Also I think in general it's better not to try fixing these errors, as moving tags can break more things down the line.

[gpetiot]

Everything looks good to me.

[emillon]

Is this expected to run install twice?

[jberdine]

I also had this question, seems like the second dune install should be removed.

[jberdine]

The version constraint is not necessary (and the fix won't be there until 2.7.0)

How is it not necessary? Once this PR is merged, the versions will be wrong with dune-build-info < 2.7.0, right? If so, this constraint is necessary, and this PR must wait.

[emillon]

They will be wrong with dune-build-info < 2.7.0 if you use a flambda switch, yes. But that's not specific to ocamlformat so I'm not sure that this fix should live in ocamlformat metadata. I suppose we can wait until there's a fix available in dune for flambda users, and maybe in the meantime add conflicts between existing versions of dune-build-info and flambda.

[emillon]

Actually as pointed in the corresponding bug, it's only a problem on 4.10.0+flambda, and it's dune that you need to upgrade, not dune-build-info.
In terms of releases, we're in a situation where dune is waiting for an ocamlformat release compatible with base 0.14 for the test suite to run on recent versions of base, and this PR needs an unreleased dune to be useful to 4.10.0+flambda users, so let's keep this PR out of the next ocamlformat release and include it in the next one.

[jberdine]

Ok, that plan sounds good. For now I'll mark this as changes requested as we'll need to update once dune 2.7 is released. There is another question, which whether to also delay merging #1361 until after this one. I don't have a strong view either way, but #1361 doesn't seem to be urgent and delaying would avoid the opam template file business. What do others think?

[jberdine]

I also had this question, seems like the second dune install should be removed.

[emillon]

Seems good to batch this with #1361.

About bound, yes we just need to depend on dune-build-info and bumping the dune dependency to >= 2.7. To do this we can either add an explicit bound or use (lang dune 2.7) in dune-project.

[emillon]

I rebased and fixed the opam metadata now that there is a dune version with a fix available.

Two remaining questions:

• should we remove the second dune install in the make target? (I think so, I don't think it makes sense to install at both places at the same time)
• is it ok to just add an opam bound, or should we bump to (lang dune 2.7) as well? the latter would ensure that we keep the same metadata when merging Dunify release #1361 (and we'll need lang dune 2.7 to try cram tests anyway)

[gpetiot]

Bumping the dnue language version sounds good to me. About the double install rule I think I did that because it was necessary to have the substitution working to have the right version number. Don't know if I missed something or if it was a bug that is fixed now, if only one install works we can simplify it of course.

[emillon]

There's no need for any make install target at all in normal operation. So that's up to us to define the semantics if we want this target :)

[emillon]

I removed the makefile changes, we can add them once we have a concrete use case. Thanks!