-
Notifications
You must be signed in to change notification settings - Fork 450
feat(oxcaml): instantiate parameterized libraries #12561
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
base: main
Are you sure you want to change the base?
Conversation
f50988f to
c8b1102
Compare
bef411b to
9a9f67a
Compare
| -> ext:string | ||
| -> visibility:Visibility.t | ||
| -> 'path | ||
|
|
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.
Not sure if it's just justified to have this function and obj_file. Does obj_file have so many callers that you can't modify it?
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 count seven calls to obj_file so it's not ideal to get rid of it. I think the main issue is that the new code for the parameterised instantiation is using Module_name to identify the artifacts, while the existing dune code is using Module instead (e.g. this obj_file function)... but for instantiation we can't directly use Module because we would need to break the invariant that a module always has at least an .ml or .mli file (which we don't have for instantiated artifacts).
Unless you disagree, right now I think it's easier to keep the parameterised complexity in parameterised_rules, and consider doing a refactor to use Module in a follow-up PR? :)
src/dune_rules/gen_rules.ml
Outdated
| | ".parameterized" :: rest -> | ||
| let* sctx = sctx | ||
| and* scope = Scope.DB.find_by_dir dir in | ||
| Parameterized_rules.gen_rules ~sctx ~scope ~dir rest |
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 ~dir is always the root of the workspace, in which case the scope is just the current project. Is that correct?
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 believe so yes, the parameterised libraries are always instantiated at the root _build/<profile>/.parameterised so ~dir is always the root. Should I be using something else to get the scope? (or is there an issue with using the root dir?)
|
Looks like you're going in the right direction. There's a massive amount of code here, and I couldn't go thoroughly through everything in one go. I'll try do another pass soon. I'm a bit concerned about all the somewhat similar but not quite the same rules in parameterized_rules.ml. Do you intend to factor this out in a better way with the rest of the rules? |
360cbb1 to
8d5bd86
Compare
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]>
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]>
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]>
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]>
Signed-off-by: ArthurW <[email protected]>
Signed-off-by: ArthurW <[email protected]>
8d5bd86 to
27e034e
Compare
Signed-off-by: ArthurW <[email protected]>
80a022b to
4bbd3c2
Compare
|
Thanks for the review @rgrinberg! Would you have time for another round? :) I believe all the major issues have been fixed, there's more tests and documentation, and I've integrated feedback from @Alizter too. The only pending issues I know of is with unwrapped libraries, which currently work but support could be better. The pre-existing bug #6148 is easy to trigger, and the automatic generation of
Are you thinking of the build archive rule or something else? We looked at it with @Alizter and it's unclear that this can be simplified easily without refactoring |
Follow up on #12392 and #12336: This PR adds the ability to instantiate parameterized libraries in library/executable dependencies. This feature requires compiler support which is only available in OxCaml atm.
In stanza dependencies, the new form
(my_lib arg1 arg2 ... :as new_name)specifies that the parameterized librarymy_libshould be instantiated with a list of arguments (which must implement the parameters requested by the lib). The arguments are identified by the parameter interface they implement, such that the order in which they are provided doesn't matter. (It's nominal not positional. We sort them internally in dune, but only to ensure we don't duplicate efforts to instantiate both(f x y)and(f y x), since they are the same.)The optional
:as new_nameallow renaming the instantiation in user code, which is useful when the same library is instantiated multiple times (otherwise if missing, by defaultMy_libis used as the name of the instantiation). In the alias file, each instantiation is translated to amodule New_name = My_lib(Param1)(Arg1)(Param2)(Arg2)(* ... *) [@jane.non_erasable.instances]. (Note that eachArgNimplementsParamN, i.e.(Arg1 : Param1), so this is not a real functor application, hence the OxCaml attribute.)While libraries can do a partial application of their parameterized dependencies (as long as their list of parameters is a superset of missing dependency parameters), executables must fully instantiate their dependencies. Computing the transitively instantiated libraries requires passing the applied arguments to each of parameterized dependencies that requires this parameter. This impacts the existing transitive closures performed by dune, since now the same library may be required multiple times but with different arguments.
To link the executables, all the (transitively) instantiated libraries must also be built by the compiler with the new OxCaml command
ocamlopt -instantiate mylib.cmx arg1.cmx arg2.cmx ... -o mylib-Arg1-Arg2...cmx. Here a partial instantiation is not allowed (all arguments must be specified). Furthermore, this should be done for all modules ofmylib, in their topological order. We create an archive at the end to package everything.Instantiated modules use dashes in their filenames to "parenthesize" their arguments, which can themselves contain dashes if they are the result of another instantiation. The number of dashes indicates the depth of the application. For example, a filename
f-g--h---x-ycorresponds to the applicationF(G(H(X)))(Y). We reuse this idea for the name of the folders where we instantiate each module of a fully instantiated parameterized dependency. A dependency(mylib arg1 arg2)(fully applied) is instantiated in the _build subfolder.parameterized/mylib!arg1!arg2. We can't use dash separators here because it's already allowed in library names and would be ambiguous, so instead we arbitrarily picked the exclamation mark instead... but let us know if there's a better choice!Finally for opam installations, we export all the dependencies arguments (but not their concrete instantiation, since this will done by user executables when needed).
While I still have a couple of TODOs to address minor issues (or so I hope), this PR seems to be working... so I would appreciate an early review to check I haven't done anything stupid :)
Fix #12088