Add the ability to choose the host context and create duplicates of the default context with different settings

[TheLortex]

This PR adds two features in dune-workspace files:

• (host <context_name>) allows to consider the given context as a cross-compiled context, and use context_name as its host context.
• (name <context_name>) can be used on the default context: this allows have a duplicate of the default context using different flags, for example.

My use case is the following:

(lang dune 1.10)
(context (default))
(context 
  (default 
    (name freestanding)
    (host default)
    (env 
      (_
        (c_flags -DFREESTANDING ...)
)))) 

Which sets up a cross-compiled context based on the default compilation context.

[avsm]

For context, this would be useful for building mirage unikernels with all C stubs systematically compiled using the right embedded cflags. mirage/mirage#969

[rgrinberg]

Amusingly, this kind of a host/target context setup is what we had for our initial version of cross compilation. But against our better senses, we went for something that more closely mimics the way cross compilation is currently done with findlib and the opam-cross repos. I'm definitely in favor of bringing up the simplified, and in my opinion more natural setup.

[ghost]

Well, it's also simplicity of use. It's nice to be able to just do dune build -x windows or add a simple (targets ...) field. In any case, it seems fine to allow the more flexible version as well for more advanced users

[ghost]

Suggested change

	let map = List.fold_left
	let map = Map.of_list_map_exn contexts ~f:(fun (_, (_, c)) -> c.name, c)

This makes the intent more explicit and the code more strict

[ghost]

If I read this code correctly, we might create several Context.t values with the same name but not physically equal. Do you agree? Given that there are mutable things in Context.t and the Context.t record is quite big, we should make sure to share these values.

I suggest to topologically sort the Workspace.contexts field in Workspace.t, this will allow us to assume that when we resolve for_host here, the Context.t value has already been computed.

[TheLortex]

Indeed, the only mutation that happens in resolve_host_contexts that might lead in a duplication of a Context.t is the for_host field update. But I think the case where it happens should be forbidden: it's the case where a sets b as host, and b sets c as host, and the order of resolution is a then b then c. In this case the context value of b duplicated during the resolve_host_context. I'm not sure this configuration is really useful: so instead of building a topological sort we can simply forbid this kind of host chain.
What do you think ?

[ghost]

Forbidding configurations that are not good seems good. However, I feel like it would be easier for someone reading the code to convince themselves that Context.t values are never duplicated if {context with ...} is never explicitly written. Building a topological sort is quite easy with Top_closure BTW

[aalekseyev]

Given this:

let equal x y = String.equal x.name y.name

if there is any chance at all that different context values can end up with the same name, the equal function should change to use physical equality.

I think it was a mistake to make it use anything other than physical equality in the first place.

[ghost]

Well, so far we have made sure to never create different context values with the same name, though it's true that we never enforced it programmatically. Comparing the names seems fine to me given that physically they can be only a single context with a given name, since each context correspond to a sub-directory in _build. To start with, we should make Context.t private, so that at least we only need to enforce this invariant in context.ml.

[avsm]

In this case, the mirage tool will generate the appropriate workspace files for the chosen backend, so this is not something that will be exposed to end users. It has the benefit that dune can parallelise builds across workspaces, which was the key limitation with using opam switch as mirage2 used to do.

[TheLortex]

@diml I've added the topological sort and the bad configuration check with some tests and little documentation.

[ghost]

Thanks, however I'd feel more comfortable if we really didn't have {ctx with ...} in the code. It's easy enough to check things work correctly now, however it'd be pretty easy to forget about it and break this invariant in the future. What we can do instead is maintain a mapping of contexts as we create them, and look up the host context immediately. The topological sort would ensure that the lookup would never fail.

[rgrinberg]

This PR should make it to 1.10 as well. I will try to review it this week.

[TheLortex]

@diml Okay so now the topological sort is done earlier in order to avoid {ctx with ..} and directly build the mapping. The drawback is that Fiber.parallel_map cannot be used anymore, as there are dependencies between contexts, so I need to unfold sequentially the generated fibers: is that problematic ?
By the way I'm not sure what should happen if both a (host ..) and a (targets ..) option are used in the same context. This should probably fail

[ghost]

@diml Okay so now the topological sort is done earlier in order to avoid {ctx with ..} and directly build the mapping. The drawback is that Fiber.parallel_map cannot be used anymore, as there are dependencies between contexts, so I need to unfold sequentially the generated fibers: is that problematic ?

Hmm, it seems reasonable to build the dependant contexts sequentially. However, independent one should be constructed in parallel.

By the way I'm not sure what should happen if both a (host ..) and a (targets ..) option are used in the same context. This should probably fail

Yep. We should fail right at parsing time.

[TheLortex]

Hmm, it seems reasonable to build the dependant contexts sequentially. However, independent one should be constructed in parallel.

Now the result of the topological sort is split into independent group of contexts and Fiber.parallel_map is used again.

Yep. We should fail right at parsing time.

Done!

I also added two more tests for error cases.

[rgrinberg]

@TheLortex I've pushed some commits to clean things up. Please review.

[TheLortex]

Looks good to me !

[ghost]

I moved the checks to workspace.ml, it seems nicer to me to have such checks as soon as we parse the file.

[rgrinberg]

Just to make sure I understand the interaction with the existing targets setup. We explicitly disallow for "target" contexts to be used as hosts for other contexts, and we also don't allow a context with targets to have a different host.

@TheLortex that's intended, right?

[rgrinberg]

@diml does it also make sense to have a Workspace.Context.Name.t type? It could be reused between workspace and context to make things a little less stringly typed.

[ghost]

I'm finding it hard to understand the logic here. Another approach would be to add the following function to Fiber:

module type Memory = sig
  type key
  type 'a t
  val empty : 'a t
  val add : 'a t -> key -> 'a -> 'a t
  val find : 'a t -> key -> 'a option
end

(** [memoize memory f] returns a version of [f] that remembers the
    result of previous call to [f] in order to not recompute them. *)
val memoize
  :  (module Memory with type key := 'key)
  -> ('a -> 'b t)
  -> ('a -> 'b t) Staged.t

and then memoize the instantiation function before passing it to parallel_map.

[rgrinberg]

I've been thinking about this as well, but I'm wondering why do we need anything extra here. In the previous version of this feature, we managed without any topological sorting by doing something like this:

let rec contexts = lazy (
  List.map workspace.contexts ~f:(fun context ->
    let context =
      let* host = String.Map.find_exn (Lazy.force contexts) context.host in
      create_context ~context ~host
    in
    (context.name, context))
  |> String.Map.of_list_exn
) in
let* contexts = Lazy.force contexts |> String.Map.values |> Fiber.all in
...

Actually, I'm pretty sure we used a hashtable before, but I don't see why a map can't be used.

[ghost]

Indeed, that even simpler. There is just one little detail: unlike lwt or async, Fiber.t values are not shared implicitly and need to be shared explicitly, so this code would end up doing the work twice for some contexts. It's easy to fix by using Fiber.Once.t though

[ghost]

More precisely, you should think of Fiber.t values as one time use. Only Fiber.Ivar.t, Fiber.Future.t or Fiber.Once.t values can be shared and used multiple times.

[rgrinberg]

Yeah, makes sense. When I wrote this originally, we still had our original Future.t abstraction.

There's just one little blemish here btw, since some contexts may have targets, they introduce more than one context per name. However, in the current code we cannot use targets as hosts for other contexts anyway, so it doesn't matter. But perhaps it would be cleaner if we could include the targets in the map anyways since those can be computed outside the Fiber.t regardless.

[ghost]

Including the targets in the map seems good

[ghost]

@rgrinberg yep, that makes sense. Although, if we want to be complete, that's a serious refactoring; think about Path.extract_build_context for instance... If we are not complete, that might make the code slightly worse.

[rgrinberg]

Although, if we want to be complete, that's a serious refactoring; think about Path.extract_build_context for instance... If we are not complete, that might make the code slightly worse.

This would require us to move Context_name.t to Stdune. That seems hardly desirable. Perhaps we should move this function and others like it to a dune specialized Path module first?

As much as I would like to do this, I don't know of a good way to accomplish this. Adding src/path.ml would be nice, but then open Stdune would shadow it. The only workaround I see is using open Import instead of open Stdune. But then we'd have to re-export every module except for Path in Import manually. 😢

[ghost]

A dune specialised path module would be nice indeed. We should simply use a different name to make things simpler. Maybe Dune_path or Dpath

[ghost]

And in particular, Path.Build should move to this new module

[rgrinberg]

@diml see the latest commit where I process all the contexts in parallel. The topological sorting isn't really required now, but I think it's still nice to have. In particular, it's nice to know all the context names have been validated.

[rgrinberg]

If the commit is good, I will incorporate the target contexts into the map as well.

[ghost]

The topological sort is still useful to get a good error message, otherwise we'd get Failure "Fiber.Once.get: recursive evaluation" which is a bit raw :)

[ghost]

The commit looks good 👍

[rgrinberg]

I think i'm going to leave the code as is for now. The targets are indeed included in the map, it's just they cannot be looked up as strings, which doesn't seem so bad. In the end, I think we just need to refactor the constructors for contexts with targets and a context with a host.