Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

work-in-progress Dune conversion #1025

Merged
merged 12 commits into from
Feb 18, 2021

Conversation

gasche
Copy link
Member

@gasche gasche commented Feb 15, 2021

This PR includes the current state of my work on moving Batteries to use a Dune build system.
(I have timed out on what I can do in the next few days/weeks, but the current state is arguably a good stopping point.)

What works:

  • all the important bits from build/ are handled correctly
  • building src, including documentation!
  • building the testsuites, including inline tests
  • building benchsuite benchmarks
  • build some of the examples examples

What is left to do:

  • transitioning the toplevel support from a bunch of .cmo files to an ocamlfind sub-package
  • checking that the dune-built batteries work with and without threads
  • installation (changing the Makefile install rule to copy dune build artifacts)
  • cleanup the rest of the Makefile
  • removing the ocamlbuild logic
  • cleaning up the install structure to use the Dune-generated META file and install logic (if that works)

To summarize the TODO list: currently Batteries install a specific META files with toplevel support (BatteriesHelp and various .cmo support modules are provided, activated by the battop.ml script). This is not compatible with Dune, which intentionally does not give access to .cmo files (basically, it wants people to install .cma files only), and generates a META file that does not support the relevant archive(toploop) ... predicate for implicit toplevel loading. I believe that a good solution would be to change the toplevel-support code to be its own findlib subpackage (batteries.toplevel), and change battop.ml to use this. Then hopefully dune install would work as expected.

The PR is built on top of my previous dune work, as submitted in #1016. I have not yet rebased it on top of the split changes proposed by @UnixJunkie. @UnixJunkie, if you would like to also split the current PR in smaller pieces, please feel free to. I separated it in smaller commits to make it easier to review in any case.

The previous method would "guess" that the source version of _oasis
can be found from the current build directory by just using
../_oasis. This works for ocamlbuild but not for dune.
This file gets concatenated by qtest to the extraction of each
src/bat*.ml file with inline tests. This is the easiest way we found
to disable a few warnings (which are disabled when compiling the
src/bat*.ml files normally) inside inline tests.
@gasche
Copy link
Member Author

gasche commented Feb 15, 2021

The PR is built on top of my previous dune work, as submitted in #1016. I have not yet rebased it on top of the split changes proposed by @UnixJunkie.

This is now fixed. The PR is now based on top of master (the rebase was very easy).

@UnixJunkie
Copy link
Member

In any case, the ocamlbuild-based system is still working, right?

In the end, won't the 'make install' just invoke 'dune install'?

It will review and merge all this. It will take me some time though.

@UnixJunkie UnixJunkie self-assigned this Feb 16, 2021
@gasche
Copy link
Member Author

gasche commented Feb 16, 2021

In any case, the ocamlbuild-based system is still working, right?

Yes. I worked around the .mlv difficulty I was telling you about in #1014 (comment) by having a lot of copy rules from foo.mlv to foo.mlv in src/. Once the ocamlbuild part is gone, you will be able to remove those copy rules and rename the files in .ml directly, and get rid of the non-standard extension.

In the end, won't the 'make install' just invoke 'dune install'?

This depends on whether dune install can produce a working installation of Batteries. I see two issues:

  • The current META.in is careful to have an "unthreaded" submodule and then the main module which depends on batteries.unthreaded and includes batteriesThread.cma. For now I have not tried to replicate this organization on the dune side; we probably should -- by moving the modules in src/batteriesThread.mllib to their own subdirectory src/thread.

  • Batteries (the unthreaded package) has this META line:

      archive(toploop)   ="batteries.cma batteriesConfig.cmo batteriesHelp.cmo batteriesPrint.cmo"
    

    Again, this is not something that can be expressed on the dune side to my knowledge, so we need to move batteriesHelp.cmo to its own library (in toplevel/) and ensure that the on-demande-loading logic in battop can be adapted to work with a findlib package rather than a bunch of .cmo files. (I think that it should work fine, and it should probably have been done in this way in the first place.)

Copy link
Member

@UnixJunkie UnixJunkie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so many changes...
We should drop the benchmarks out of the window (or save them in another repository,
out of batteries), because they have a significant maintenance cost.

@UnixJunkie UnixJunkie marked this pull request as ready for review February 18, 2021 01:49
@UnixJunkie UnixJunkie merged commit ad1e76b into ocaml-batteries-team:master Feb 18, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants