ygrek / ocurl

OCaml bindings to libcurl
https://ygrek.org/p/ocurl
MIT License
59 stars 32 forks source link

Use Dune for building #56

Closed nojb closed 2 years ago

nojb commented 2 years ago

This PR switches the build system from make to Dune (NB the configure script is still used).

The main advantage is to allow to compose ocurl easily in larger Dune monorepos, but it also yields a nice simplification of the build system (see the diff).

nojb commented 2 years ago

This is awesome, thanks! Do you mind squash-rebasing on current master and please keep legacy release target, I will see if dune-release works for me now.

Rebase done. I restored the old release target (and renamed the new one dune-release).

While doing this I remember something I forgot to mention in the issue description above. Dune enforces that when a project consists of a single package they should have the same name. Here "project" is the thing that you use when you write opam install <project name> and "package" refers to the notion used by Findlib.

Currently the project name is ocurl but the Findlib package name is curl, so a choice needs to be made: either 1) rename the project curl as well (not very distinctive), or 2) rename the Findlib package ocurl (better IMO).

In the PR I did 2). In order to avoid breaking packages that depend on the curl name, I added a couple of deprecated_library_name stanzas to main dune file that have the effect of installing a dummy META file under the "old" names curl and curl.lwt declaring dependencies on the "new" names ocurl and ocurl.lwt.

ygrek commented 2 years ago

I would on the contrary think that findlib name is more important compatibility-wise..

Dune enforces that when a project consists of a single package they should have the same name. Here "project" is the thing that you use when you write opam install and "package" refers to the notion used by Findlib.

is this dune property or dune-release? I may side-step this issue entirely by not generating opam package with dune?

nojb commented 2 years ago

I would on the contrary think that findlib name is more important compatibility-wise..

OK. Note that with the PR in its current form you keep both ocurl OPAM name and curl Findlib name (via the compatibility META shim), and ocurl can also be used as Findlib name. The alternative is to change the OPAM name from ocurl to curl, in which case no compatibility shim is needed.

Dune enforces that when a project consists of a single package they should have the same name. Here "project" is the thing that you use when you write opam install and "package" refers to the notion used by Findlib.

is this dune property or dune-release? I may side-step this issue entirely by not generating opam package with dune?

I believe this is a dune property.

ygrek commented 2 years ago

Another argument is that the toplevel module name is Curl, so better keep library name curl. Made the change accordingly (make ocurl.opam a transitional package depending on curl.opam), tweaked ocamlc flags to my liking, restored back CHANGES.txt (rendered markdown hides the difference between + (additions) and others), and it is good to go.

Thanks a lot, that was a good piece of work.

Cherry-picked as 4eb931452400b857e12817faac7b9e65fffc1a42

nojb commented 2 years ago

Cherry-picked as 4eb9314

Thanks!