-
Notifications
You must be signed in to change notification settings - Fork 453
feat(oxcaml): support parameter implementation #12392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f7c52d8 to
08b368d
Compare
shonfeder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying over the outstanding change requests from #12002
a9b566d to
dcb3793
Compare
|
Thanks for the fixes, @art-w! I'll let you take this from here :D IIUC, this is ready for review, so I will mark it as such. |
src/dune_rules/virtual_rules.ml
Outdated
| (Lib_name.to_string implements) | ||
| ]; | ||
| (match Lib_info.kind info with | ||
| | Parameter | Virtual -> () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Parameter case should be "assert false" I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm in the current implementation the Parameter case is useful, as a library that has an implements still get turned into a "vlib" which associates the library with the (virtual or parameter) lib it implements. I suppose we could avoid reusing parts of the virtual logic however, with a new sum type above. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's confusing indeed then. Turning it into a sum type (or a record with a kind field) would clarify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a refactoring of Virtual_rules to distinguish between the three implements possibilities: no implements, implementing a virtual module or implementing a parameter. I think virtual_rules.ml could be further renamed to implements.ml, if the name "virtual" has too much connotation? :)
rgrinberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just finished a round of review.
|
Thanks for the quick review! I've marked the simpler issues as resolved by the last commit, but I've left open the ones where I would appreciate your input :) |
| in | ||
| match argument with | ||
| | None -> [] | ||
| | Some parameter -> [ "-as-argument-for"; Module_name.to_string parameter ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remind where we add a dependency on the cmi of the parameter? Is it added a library dependency somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the dependency to the parameter is added by the pre-existing code for vlibs:
Lines 1025 to 1034 in 07d0a93
| let* requires = | |
| Memo.return | |
| (let open Resolve.O in | |
| let* requires = requires in | |
| match implements with | |
| | None -> Resolve.return requires | |
| | Some impl -> | |
| let+ impl = impl in | |
| impl :: requires) | |
| in |
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
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 <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
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 <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Shon Feder <[email protected]>
Signed-off-by: Shon Feder <[email protected]>
Signed-off-by: ArthurW <[email protected]>
Signed-off-by: ArthurW <[email protected]>
Signed-off-by: ArthurW <[email protected]>
Signed-off-by: ArthurW <[email protected]>
Signed-off-by: ArthurW <[email protected]>
3e40fdc to
c9fa595
Compare
| open Import | ||
| open Memo.O | ||
|
|
||
| let setup_copy_rules_for_impl ~sctx ~dir vimpl = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was moved (unchanged) to Vimpl.setup_copy_rules: https://github.com/ocaml/dune/pull/12392/files#diff-654c4360f33ebd2b51644332b9d26dcf9d2e39b1a2125ae4bc9f6400a7cc412cR74
| "Library %s isn't virtual and cannot be implemented" | ||
| (Lib_name.to_string implements) | ||
| ]; | ||
| let+ vlib_modules, vlib_foreign_objects = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also moved (unchanged) to Vimpl.make: https://github.com/ocaml/dune/pull/12392/files#diff-654c4360f33ebd2b51644332b9d26dcf9d2e39b1a2125ae4bc9f6400a7cc412cR20
|
I think this is ready for another review :) . I had to move large chunks of code in the last commit to clarify the |
Closes #12085
Supersedes #12002
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)).