Replace dynlink method by a 2-stage build #171
Conversation
|
@TheLortex I'm super excited about this since I think it can pave the way for external device types/implementations. I cannot get this to build though, can you link to the corresponding |
|
The PR currently breaks API with Mirage, but it's not necessary, I'm waiting for comments on this. There are two branches of https://github.com/TheLortex/mirage that implement the change of this PR:
|
app/functoria_app.ml
Outdated
|
|
||
| let register ?packages ?keys ?(init=default_init) name jobs = | ||
| (* Compile the configuration file. *) | ||
| let compile file_ml_path = |
There was a problem hiding this comment.
This function is slightly too big and convoluted. Could you add a bit of documentation describing 1) which files are created (and where) 2) what is expected to go into dune.config and what will be the result and 3) extract a few helpers functions to make the body of the function straightforward (as it should be :-) ).
This could be added either here or in the mli but an example of dune.config should definitely go into the mli.
There was a problem hiding this comment.
I have refactored this function, separated code and added some documentation !
Regarding dune.config it's probably good to document it in the readme (that needs an update anyway)
app/functoria_app.ml
Outdated
| | `Exited 0 -> Ok (Fpath.(to_string (target_dir / file))) | ||
| | `Exited _ | ||
| | `Signaled _ -> | ||
| Format.fprintf Format.str_formatter "error while executing %a\n%s" |
There was a problem hiding this comment.
This is:
let err = Fmt.strf "error while executing %a\n%s" Bos.Cmd.pp cmd out in
Error (`Invalid_config_ml x)It would even be better to move the creation of error values in separate functions (so it's easier to spot the different failure modes in one go) but this is not necessary to do for the given PR.
and | Signaled _ -> err_invalid_config_ml cmd out`
There was a problem hiding this comment.
I haven't separated functions but I updated it according to your review
also updates according to reviews in mirage#171
|
Looks good, merging! |
Hi, this is quite a heavy PR. I can try to minimize the diff if you want.
Why
The dynlink method to load
config.mlfeels quite fragile, as it requiresmirageandconfig.mlto be built with the same code / options. When compiling usingdune, these assumptions can be broken:config.mlcan be built with a local instance of the mirage source code.miragebut are enabled forconfig.ml.Proposal
Instead of dynlinking
config.ml, we can perform a 2-stage build of the configuration !(1) Locally build
config.ml(2) Execute
config.ml, which calls some entry function inFunctoria_appThere are several ways of doing it:
config.mlandmirage:Functoria_app.Makein two functors, one that builds theconfig.mland one that is called by the builtconfig.mlregister ...to a more appropriate namemirage:Makeand haveregister ...as the entry point of (2)Makeand hack with the names that are currently used.I've implemented the intermediate solution. This is probably far from perfect but I'd like to have feedback on that :)