search

Khady / ocaml-argon2

Ocaml bindings to Argon2
MIT License
29 stars 8 forks source link

Update topkg to support OCaml 4.06 and 4.07 #1

Closed gasche closed 5 years ago

gasche commented 5 years ago

This PR updates argon2's topkg setup to build correct under OCaml 4.06 and 4.07. (The former setup included topkg-ext.ml which requires -unsafe-string.)

The documentation plugin wouldn't build, so I got rid of it. The default ocamldoc output has improved in last versions, so I think that's a reasonable tradeoff. I don't know how to set up things to use @dbuenzli's favourite style again.

I tested topkg {build, doc, clean}, and they seem to work.

opam pin works as excepted, you get the current commit hash as version number in META/ocamlfind.

topkg distrib builds an archive as expected, but it uses the git commit hash as version number, even if I have tagged my HEAD (in the branch update-topkg) with tag 0.3 or v0.3. To get an archive for a candidate 0.3 release, I need to use topkg distrib --dist-version 0.3. (cc @dbuenzli, should I report a bug?)

topkg lint reported some issues I fixed, but it also complains about a mismatch between _opam, which depends on ctypes-foreign, and _tags, which doesn't seem to use it. In fact _tags uses ctypes.foreign. I don't know how to fix or silence this warning, so I let it be.

Because META uses %%VERSION%% (unchanged), you cannot distribute software to opam-repository using just git archives, you need to upload/attach the archive built from topkg distrib (github lets you do this).
The only thing I would need to remove that requirement (if you want to use git archives) would be a --pkg-version option to the pkg.ml invocation, so that I could use stock git repository and specify the version to substitute from the version-specific opam file in the opam-repository. @dbuezli, should I file an issue request?

(I tried to look at topkg's source code to fix/implement/understand something myself, but it looks like currently the tool uses fairly different control paths, in Topkg_pkg.build, in the pin case (which does the kind of substitutions I'm hoping for) and the non-pin case (which apparently doesn't, I guess it delegates the work to distrib).

gasche commented 5 years ago

cc @Khady. The executive summary would be:

(I think that hardcoding the version number in topkg/META at release time would probably be an acceptable compromise to preserve the release workflow without having to spend any more time working on the packaging.)

Khady commented 5 years ago

Thanks for your work on this @gasche. Did you do it only to get this package to compile with recent versions of ocaml and please the CI or because some people are actually using this package?

I'm thinking to merge this PR in a first time. And migrate to dune + dune-release in a second time.

If no one is using this package, I think it should be removed from the opam repository (but I don't know what is the procedure to do that). It is not under high maintenance and can be use for sensitive operations. An alternative is https://github.com/dsheets/ocaml-sodium/

If there are active users, then I would be happy to work more actively on this project.

dbuenzli commented 5 years ago

The documentation plugin wouldn't build, so I got rid of it. The default ocamldoc output has improved in last versions, so I think that's a reasonable tradeoff. I don't know how to set up things to use @dbuenzli's favourite style again.

I think these recent changes might have broken the stylesheet. I'm not seing them yet because I don't release software that is specific to these versions and use the lowest supported release to make my releases. But I don't have the time to look into that right now sorry (if someone has PR to odig which has the master version of the stylesheet are welcome).

topkg distrib builds an archive as expected, but it uses the git commit hash as version number, even if I have tagged my HEAD (in the branch update-topkg) with tag 0.3 or v0.3. To get an archive for a candidate 0.3 release, I need to use topkg distrib --dist-version 0.3. (cc @dbuenzli, should I report a bug?)

What does git describe tell you ? Does it report the tag ? I suspect you did not do an annotated tag. (You can also have a look at topkg tag --help).

The only thing I would need to remove that requirement (if you want to use git archives) would be a --pkg-version option to the pkg.ml invocation, so that I could use stock git repository and specify the version to substitute from the version-specific opam file in the opam-repository. @dbuezli, should I file an issue request?

You can but as far as I'm concerned topkg is in maintenance mode. More generally I'm against people letting github generate tarballs for them for the following reasons:

  1. The checksum of archives generated by github have not always been stable in the past.
  2. If you believe version number of the software you define should not be commited to the repo (already had one of these "forgot to bump version" commit ?) but come from the VCS handling the repo where they naturally come from, there's no way you can do a release without applying a function on a checkout before making your release tarball.

Note that topkg publish handles all the bureaucracy of creating a release and attaching the reproducible tarball generated by topkg distrib so it shouldn't be a problem (though the toy github delegate might a bit rough on the edge w.r.t. to auth and error reporting, see toy-github-topkg-delegate --help for more information).

gasche commented 5 years ago

Did you do it only to get this package to compile with recent versions of ocaml and please the CI or because some people are actually using this package?

The former, sorry; on the occasion of a new ocamlbuild release, I took a look at the first N packages in the reverse dependencies that break with -safe-string, and marked them <4.06.0. When the latest version is broken, I try to update the package if I can.

I don't know if there are users of the package today, but some may show up tomorrow and it's nicer to have the package in, in case.

Thanks @dbuenzli for the comments. I think I'll leave matters as they are for now. Note that I haven't tried your upstream/uptodate stylesheet, only the old one that was vendored in this package.