Add `(:include <file>)` terms to include_dir field

[gridbugs]

trafficstars

Addresses https://github.com/ocaml/dune/issues/3993

Still need to add a version number check but opening this now to get preliminary feedback while I work out the best way to do that.

[rgrinberg]

Looks good in general. Design wise, one thing that bothers me is that we're enabling :include but not the whole ordered set language in this field. That is inconsistent with how dune works elsewhere. Can we try and support the full ordered set language here instead?

[gridbugs]

I totally agree with your point about inconsistency. I don't think there's a way to treat the entire (include_dirs ...) field as an ordered set because it currently allows terms of the form (lib <libname>) which isn't valid OSL syntax. An alternative would be to allow terms that introduce OSL expressions, e.g.:

(include_dirs (osl (:include foo)))

[gridbugs]

Also, my current implementation makes it an error if the file referred to by (:include ...) contains an OSL with 0 or >1 elements. It wouldn't be hard to support multiple directories referred to by a single (include ...)-ed file, but I'm not sure if that's ever something that will be useful in practice. @rgrinberg any thoughts on this?

[rgrinberg]

I totally agree with your point about inconsistency. I don't think there's a way to treat the entire (include_dirs ...) field as an ordered set because it currently allows terms of the form (lib ) which isn't valid OSL syntax. An alternative would be to allow terms that introduce OSL expressions, e.g.:

It should still be possible to support. We can instantiate ordered set lang with other ast elements. In this case, it would be with this type:

    type t =
      | Dir of String_with_vars.t
      | Lib of Loc.t * Lib_name.t

You might need to duplicate some stuff from Ordered_set_lang.Unexpanded.t because our stuff isn't well factored, but it should still be doable.

[rgrinberg]

Also, my current implementation makes it an error if the file referred to by (:include ...) contains an OSL with 0 or >1 elements. It wouldn't be hard to support multiple directories referred to by a single (include ...)-ed file, but I'm not sure if that's ever something that will be useful in practice. @rgrinberg any thoughts on this?

It's not particularly useful, but it does make it consistent with other uses of (:include ..) so I think it's worth doing.

[gridbugs]

@rgrinberg based on our discussion I've redesigned this change so that the include_dirs field can contain an arbitrary ordered set of Include_dir.ts. I factored out the common logic in Ordered_set_lang.Unexpanded into a functor. I still need to make behaviour fall back to pre-3.5 when the version is <3.5 but updating the PR for early feedback.

[rgrinberg]

The PR looks good. I left some minor comments that should be trivial to address. Before we merge this PR, you should also:

• Update the change log at CHANGES.md
• Update the manual.

[rgrinberg]

CI says we need a promotion here.

[gridbugs]

I forgot to update a test when I changed an error message. That's fixed now, and I've also split the tests into multiple files with one file per test case.

[rgrinberg]

Looks good. I merged with a few tweaks.