feat(oxcaml): add the library_parameter stanza #11963
Conversation
| Specifies what modules are part of the library parameter. By default, Dune will use | ||
| all the ``.mli`` files in the same directory as the ``dune`` file. This | ||
| includes ones present in the file system as well as ones generated by user | ||
| rules. You can restrict this list by using a ``(modules <modules>)`` field. |
There was a problem hiding this comment.
A library parameter can only have a single .mli file, so the documentation should not say "all the .mli files in the directory"... In fact I'm not sure if we should keep the (modules ...) field, since its value should always be parameter_name
There was a problem hiding this comment.
Should we go in the direction where we keep the modules field or if, by default, library_parameter only take the parameter-name and exclude the rest of the modules? My point of view is that modules would keep Dune users in familiar waters but taking parameter-name would remove the risk to specify multiple modules... I'm not sure about the best behaviour to adapt here.
In the rework version, I have simplified the phrasing for our need.
WDYT @rgrinberg?
maiste
force-pushed
the
library_parameter
branch
2 times, most recently
from
423c46b to
ac62dfa
Compare
|
@maiste is this ready for another round of review? |
|
Yes it is 😄 I have addressed all of @art-w comments. |
maiste
force-pushed
the
library_parameter
branch
from
ac62dfa to
0dd1910
Compare
|
@rgrinberg gentle ping. I have rebased the PR with |
rgrinberg
left a comment
rgrinberg
left a comment
There was a problem hiding this comment.
LGTM. I think the library_parameter stanza is overly permissive at the moment. Might be worth documenting this in the tests.
|
Thanks for the review @rgrinberg! Just to understand before pushing new tests, what do you think in the stanza is overly permissive? Is the fields we kept that should have a default value instead (like optional)? Or is it something else? |
src/dune_rules/stanzas/parameter.ml
Outdated
| and+ package = field_o "package" (located Stanza_common.Pkg.decode) | ||
| and+ enabled_if = Enabled_if.decode ~allowed_vars:Any ~since:None () | ||
| and+ synopsis = field_o "synopsis" string | ||
| and+ stdlib = |
There was a problem hiding this comment.
I don't think this field makes sense for library parameters
There was a problem hiding this comment.
Even if you want to use Base as the standard library? Otherwise, I can remove it.
There was a problem hiding this comment.
It won't work for base because it's a regular library. This is a field that is only useful for the stdlib itself. In other words, it's for using dune to build OCaml itself (for a better development workflow)
|
Sorry my previous comment was a bit opaque. I expanded on the fields I think are unnecessary and which tests I would like to see. |
maiste
force-pushed
the
library_parameter
branch
from
48a2416 to
fb81e0f
Compare
|
Thanks for the clarifications! I have rewritten the git history to make it cleaner (to use merge commit when merging the PR). I have pushed a test to check the result when using a module. As you said it is currently too permissive as we don't check for the module size. We have extra tests we backported from JS tests but that are paired with implementation, instantiation and parameterization. With @art-w, we intended to push them later along with the paired code. |
|
@Alizter @rgrinberg I have made the changes according to your feedback. If this version is good, I will rework the git history to have something cleaner (working with the DCO check) and merge it. Small question: even if this is experimental, should I add a |
|
If you think it's useful for users to know about this addition, you're welcome to add it. |
maiste
force-pushed
the
library_parameter
branch
2 times, most recently
from
e13fcdc to
751c4fd
Compare
|
I have pushed a CHANGES.md with the experimental label to make it clearer it is not something stable yet. I have also rewritten the history. @art-w and @rgrinberg if this is good in the current state for you, I'll merge the PR. |
maiste
force-pushed
the
library_parameter
branch
from
751c4fd to
e237f70
Compare
|
It looks to be in a good state for me. |
Co-authored-by: Rudi Grinberg <[email protected]> Co-authored-by: ArthurW <[email protected]> Signed-off-by: Etienne Marais <[email protected]>
It uses the (use oxcaml 1.0) extension to make sure the experimental feature is not exposed directly to Dune users. Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
This commit adds a first batch of tests regarding the parametrized library. These tests are extended in future commits to add more checks as it requires extra behaviour to check it deeper. Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
Signed-off-by: Etienne Marais <[email protected]>
maiste
force-pushed
the
library_parameter
branch
from
e237f70 to
e45b5fa
Compare
4 participants
This PR is the first of a set of four PRs related to the introduction of Parameterized Libraries in Dune.
This feature is only available with OxCaml until it is upstream to the compiler.
It contains multiples changes:
library_parameterstanzaCloses #12084