feat(oxcaml): support parameter implementation#12002
feat(oxcaml): support parameter implementation#12002maiste wants to merge 16 commits intoocaml:mainfrom
Conversation
ec4d9c6 to
665cccd
Compare
art-w
left a comment
There was a problem hiding this comment.
Great! Can we add a test with a library that implements a parameter that doesn't exist? (this should be caught by the existing virtual modules logic, but the error message may need an update)
a0027e4 to
a1349eb
Compare
a1349eb to
e6e5805
Compare
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
This version works only in the singleton case at it aggressively add the module implements to all the module. In the case of the singleton it will break. Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
This commit extends the support to find the root module from the library name which must always exists locally because we declare the implementation. Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <dev@maiste.fr> Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
Signed-off-by: Etienne Marais <self@maiste.fr>
e6e5805 to
a45b410
Compare
| (fun () -> build_shared ~native_archives ~sctx lib ~dir ~flags) | ||
| ;; | ||
|
|
||
| let parameterize_root_module ~parameter ~root_module_name modules = |
There was a problem hiding this comment.
Instead of storing this information in the module, how about just add a field to the compilation context designating which module is implementing a parameter. Should be easier to understand and more efficient.
| | Ok None -> | ||
| (* The parameter must have a module name otherwise it can't exist. *) | ||
| assert false | ||
| | Error err -> Resolve.raise_error_with_stack_trace err |
There was a problem hiding this comment.
This error needs to be delayed rather than forced.
|
|
||
| $ . ./helpers.sh | ||
|
|
||
| We create two parameters, on public, the other one local. |
There was a problem hiding this comment.
| We create two parameters, on public, the other one local. | |
| We create two parameters, one public, the other one local. |
| We implements a non parameter library (neither a virtual module). It should | ||
| fail with the correct error message. |
There was a problem hiding this comment.
| We implements a non parameter library (neither a virtual module). It should | |
| fail with the correct error message. | |
| Supplying a non-parameter, regular library (not a virtual module), as an implementation should | |
| fail with the correct error message. |
| 3 | (implements missing_foo)) | ||
| ^^^^^^^^^^^ | ||
| Error: Library "missing_foo" not found. | ||
| -> required by alias default |
There was a problem hiding this comment.
Caveat that I am still getting up to speed here, but is this the right error message? I would rather expect something like parameter interface not found.
| -> required by alias default | ||
| [1] | ||
|
|
||
| We implements the parameter using library calling a wrong parameter name. |
There was a problem hiding this comment.
| We implements the parameter using library calling a wrong parameter name. | |
| We implement the parameter using library calling a wrong parameter name. |
| $ rm -rf _build foo_impl | ||
| $ make_dir_with_dune "foo_impl" <<EOF | ||
| > (library | ||
| > (name foo_impl) | ||
| > (implements missing_foo)) | ||
| > EOF | ||
| $ dune build | ||
| File "foo_impl/dune", line 3, characters 13-24: | ||
| 3 | (implements missing_foo)) | ||
| ^^^^^^^^^^^ | ||
| Error: Library "missing_foo" not found. | ||
| -> required by alias default | ||
| [1] |
There was a problem hiding this comment.
This looks like the exact same test as the one on on lines 41 to 53. Perhaps a needed update to differentiate them was missed? Or am I missing something on my end?
There was a problem hiding this comment.
As the user provides an element which doesn't exist, we can't differentiate if it is a virtual library or a parameter. We don't have the information to characterize the element.
There was a problem hiding this comment.
Ok, so we cannot differentiate these two cases, but then I supposed it doesn't help anything to test the same scenario twice?
There was a problem hiding this comment.
Initially, it was to make sure we fail if the implements was fed, either with a non-existent parameter (or library) or a non parameterized library.
It depends if we go to a strict separation with Virtual or not. If we do, we might want to keep this test to make sure we are not using a library that shouldn't be used.
|
I've assigned myself here since I'll be taking this over from @maiste. My initial review comments here just notes to self (with the exception of #12002 (comment), which is a question for @rgrinberg). |
|
I don't seem to be able to push to this branch, so I am inclined to just replace this with a new PR. I'll copy the context over. I'll do it tomorrow, unless I get objections. |
|
I don't see a problem with opening a new PR. However, you should be able to push on the branch unless you don't have sufficient rights on the repository. |
|
I have a branch locally updating this on top of. #12253 I will push to this or replace it tomorrow. Just ran out of time. |
|
Thanks @maiste. I think I must not have maintainer perms, because I just get permission denied errors consistently. |
|
Superseded by #12392 |
Closes #12085
This PR is the second of a set of four PRs related to the introduction of Parameterized Libraries in Dune.
It contains multiples changes:
implementsfield.TODO
is_parametershould be removed (as per refactor: remove [Lib_info.is_parameter] #12298 (comment)).