Use MDX to test the README example

[craigfe]

Resolves #986.

• Requires Dune 2.4.0 for the (mdx) stanza. Dune 2.0+ locks are currently broken (soon to be fixed by Fix locks in rules without targets ocaml/dune#3366). This adds a fake dependency test/irmin-unix/runtest → test/irmin-http/runtest to ensure that those are serialised.

• Requires unreleased MDX support for labels in HTML comments, so marked as draft until those are released.

[craigfe]

Following the recent MDX release (thanks @NathanReb!), this can now be considered for merge.

[craigfe]

CI fails due to ocaml/dune#3650, so this is blocked.

[samoht]

The issue in dune has been fixed in 2.7.0. Could you rebase your PR and see if that makes things go better?

[craigfe]

Travis isn't updating properly to demonstrate it, but I'm fairly sure that this issue hasn't been fixed in Dune 2.7.0.

In the linked PR, I improved the error message in the case where not all packages listed in the mdx stanza are available (such as when running with -p). However, the fundamental issue is that mdx stanzas can't be attached to a specific package in the way that tests can, so multi-package repositories cannot have mdx stanzas that do not work with each individual package individually.

@NathanReb is aware of the issue and seems to have plans to fix it, although I don't know if there's an issue anywhere tracking that. Until that's fixed, we're once again blocked on using MDX this way.

[samoht]

Why is this skipped?

[craigfe]

I think mdx has issues with running Dune from inside Dune:

 |```sh
 |$ dune build examples/readme.exe
-|$ dune exec examples/readme.exe
-|foo/bar => 'testing 123'
+|Error: Multiple rules generated for _build/default/examples/readme.exe:
+|- file present in source tree
+|- examples/dune:2
+|Hint: rm -f examples/readme.exe
+|[1]
+|$ dune exec examples/readme.exe
+|Error: Multiple rules generated for _build/default/examples/readme.exe:
+|- file present in source tree
+|- examples/dune:2
+|Hint: rm -f examples/readme.exe
+|[1]

but I may just be missing something 🙂

[samoht]

that's weird, as it's something that is done in RWO without much trouble. Again please report it upstream so that it's tracked and solved properly :-)

[craigfe]

Looks to me like RWO is cheating slightly by unsetting INSIDE_DUNE: https://github.com/realworldocaml/book/blame/master/book/testing/README.md#L544. I'll discuss this with @NathanReb when he gets back from holiday next week.

[samoht]

Please open an issue so I can follow the discussion too ;-)

[samoht]

@NathanReb is aware of the issue and seems to have plans to fix it, although I don't know if there's an issue anywhere tracking that. Until that's fixed, we're once again blocked on using MDX this way.

Please make sure an issue is open to keep track of this issue -- we should make sure this is fixed properly!

[craigfe]

The blocking problem is now tracked by ocaml/dune#3756 in Dune.

[samoht]

@voodoos is the feature now available in dune? Can we use it in irmin? :-)

[voodoos]

I was not aware of the request for a package field in the MDX stanza when I started working on the new version of the stanza which does feature a generic deps field instead of the old packages field. (For reference, the RFC is here: ocaml/dune#3955)

However the development of the new stanza was slowed down after we discover an issue with ocamlfind in opam monorepos (that is in fact not related to the MDX stanza).

We recently decided to resume work on it with @NathanReb. I rebased the MDX part and will probably rework the Dune part soon. I guess we can take that opportunity to add this package field ?

For now it is planned for milestone 3.0. @NathanReb do you have more input on this ?

[NathanReb]

For now it is planned for milestone 3.0. @NathanReb do you have more input on this ?

Not particularly but being able to attach an MDX stanza to a package is definitely a required feature for multi-opam repositories.
There might be issues for users that want to #require a package from within the repo in their MDX blocks as there won't be a generic way to express that dependency (unless (package ...) deps have been fixed to work in an opam context) but they will be able to work around it by using the (libraries ...) field of the new stanza instead.
To summarize: adding the (package ...) field is 100% 👍

[emillon]

FYI, (package) support has been merged and will be in dune 2.9 (next week) and dune 3.0.

[emillon]

Dune 2.9 is now out with that feature.

[zshipko]

LGTM, this makes updating the README much less error prone!

Seeing this makes me wonder if it would make sense for the tutorial code to live in the main repo so it can automatically be checked as well. I can imagine several reasons not to do that, but perhaps at some point in the future.