Fix evaluate-time error with MDX stanza and unavailable packages

[craigfe]

This adds a test that reproduces a Dune bug when using mdx stanzas in combination with the -p command-line option.

Why this happens

Expanding the Mdx stanza involves looking up each stated package in the super context and generating an alias to install it (https://github.com/ocaml/dune/blob/master/src/dune/mdx.ml#L185-L188). When -p is passed, it's possible that not all packages will be available, causing an exception to be raised.

I would expect the MDX stanza to not be expanded (and the executable not to be required) when the supplied package name is not one of the set in the stanza. There is a question of what to do with internal package dependencies, for instance:

(mdx
 (files README.md)
 (packages irmin irmin-unix))

In this case, irmin-unix depends on irmin but not vice-versa, so ideally this stanza would be run when -p ... irmin-unix ... is passed. Perhaps it'll be necessary to associate a specific package to each stanza (along with libraries) to resolve this issue.

I'm happy to prepare a PR to fix this issue if anyone has ideas.

CC: @NathanReb

[NathanReb]

Do you know what the current behaviour is with regular rules that have (package ...) dependencies? This shouldn't be any different here. In particular I was considering replacing the packages field with a regular deps field.

I also ran into a slightly related bug where running dune build required mdx to be installed.

[craigfe]

I had a look around when looking for an easy fix, and my understanding is that mdx is unique in having a packages field (everything else having a singular package field). My expectation was that the MDX stanza would have both package and libraries fields, and the user is responsible for ensuring that the stated package pulls in all the necessary dependencies.

[NathanReb]

The packages field is strictly equivalent to a bunch of (deps (package ...) (package ...)). What I'm wondering is how a rule with such dependencies behave in the context of -p? Is it simply not executed or does the build fail?

I'll give that a spin!

[NathanReb]

Note that I'm planning to add a libraries field to the mdx stanza so that you can depend on libraries directly without having to load them with ocamlfind but it requires a few changes on how it's implemented.

The same kind of issues will then arise i.e. you won't be able to run your mdx test if the -p option doesn't let you build one of the libraries specified here I guess.

[NathanReb]

The package field i.e. the field specifying to which package the stanza and associated tests belong is a broader problem that was already raised with other tests such as inline_tests see this issue.

We could eventually add one to the mdx stanza but that wouldn't solve the dependencies issue at hand here which is that the stanza can depend on other packages and therefore won't work if -p isn't passed the list of all those packages. That's true of package and library dependencies.

One thing we need to fix though is that nasty error, even if it must just fail, it should do it in a clean and similar way to any rule that suffers the same issue.

[rgrinberg]

The packages field is strictly equivalent to a bunch of (deps (package ...) (package ...)). What I'm wondering is how a rule with such dependencies behave in the context of -p? Is it simply not executed or does the build fail?

It will fail when you build its targets because its dependencies aren't satisfied.

@craigfe I fixed the same issue in the (deps ..) field #3424. Do you think you could do a similar fix here?

[rgrinberg]

Alternatively, you can evaluate the packages field using Dep_conf_eval.unnamed. This way, we'll only have a single code path for this validation.

[craigfe]

@rgrinberg: I've pushed a commit that improves error handling in a similar manner. (Will avoid going the Dep_conf_eval.unnamed route for now, since @NathanReb is working on this code too.)

[rgrinberg]

This fix is not the right way to do it. It raises the error when the rule is evaluated rather than when it is executed.

Now I'm more convinced we should just reuse unnamed in Deps_conf_eval which handles this correctly.

[craigfe]

I've pushed a new commit that switches to Dep_conf_eval.unnamed, which looks much cleaner; thanks. (It wasn't clear to me how to avoid an evaluate-time error with the Build_system.Alias.package_install approach anyway.)

[rgrinberg]

Thanks. The fix looks good now. To clarify for anyone else reading, it was necessary to delay the error to prevent other rules in the same directory from being broken. Raising on evaluation aborts the entire set of rules for the directory, while this approach delays the error until a target of the broken rule is actually needed.

[rgrinberg]

Looks good, thanks. All I've done is improve the locations in your error message