Variants v2 by TheLortex · Pull Request #2169 · ocaml/dune

TheLortex · 2019-05-17T14:32:44Z

I've implemented most of what has been discussed in #2134

What we have is:

variants are implicit in the local scope.
variants can be explicitely specified using a (external_variants (<variant> <impl>) (...)) option in the library stanza.
the list of known tagged implementations is computed when the virtual library is installed and metadata is added in a new known_implementations field.
conflict errors yield when the virtual library is built:
- if two explicit implementations with the same variant
- if one implicit and one explicit implementation share the same variant but not the same name.
I still haven't figured out how to properly handle the -p case: I'm able to convert implementation stanzas to a new Variant_implementation stanza but I'm not sure how to make that work with the rest.

@diml @rgrinberg I'd like to have some feedback on this !

TheLortex · 2019-05-20T13:11:55Z

So now it should be reviewable.
What has changed:

external_variant is a stanza, instead of an option in the library stanza.
when using the -p option, implementations of virtual libraries that are in the list of packages are transformed into an external_variant stanza. Instead of computing the final variant list in instantiate, it's now computed in DB.create_from_library_stanzas which takes a new variants_stanzas parameter.

ghost

Looks good overall. I just left a few suggestions

src/dune_file.ml

src/dune_package.ml

src/dune_file.ml

src/gen_rules.ml

src/super_context.ml

rgrinberg · 2019-05-22T07:11:43Z

I'll review this PR shortly.

The reason why I wasn't in a rush is that I'm leaning towards including it in 1.11 instead. I'd like more time to evaluate this change in practice before making a release and I don't want to delay 1.10 any longer.

TheLortex · 2019-05-23T13:03:47Z

Should an error happen if someone use external_variant on a library which doesn't exist / isn't a virtual library ? For now it's just silent.

ghost · 2019-05-23T13:16:10Z

Yes, we should error out. If we silently ignore such cases, it means that we are accepting dead code in dune files, which is not good.

src/lib.ml

TheLortex · 2019-05-31T15:00:58Z

@rgrinberg I removed the public name only limitation and updated tests accordingly !

doc/variants.rst

src/lib.ml

* Explicit computation of variant list at virtual library build time * Implicit detection of local variants * Explicit declaration of external variants Signed-off-by: Lucas Pluvinage <lucas.pluvinage@gmail.com>

rgrinberg · 2019-06-06T00:52:33Z

Thanks @TheLortex

@TheLortex

CHANGES: - Don't select all local implementations in `dune utop`. Instead, let the default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex, review by @rgrinberg) - Check that selected implementations (either by variants or default implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by @rgrinberg) - Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml) - Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The set of variants is now computed when the virtual library is installed. Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex, review by @diml) - Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275, @rgrinberg) - Fix error message for missing C++ sources. The `.cc` extension was always ignored before. (ocaml/dune#2275, @rgrinberg) - Add `$ dune init project` subcommand to create project boilerplate according to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder) - Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo) - Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review by @rgrinberg and @diml) - Add a `cinaps` extension and stanza for better integration with the [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269, @diml) - Allow to embed build info in executables such as version and list and version of statically linked libraries (ocaml/dune#2224, @diml) - Set version in `META` and `dune-package` files to the one read from the vcs when no other version is available (ocaml/dune#2224, @diml) - Add a variable `%{target}` to be used in situations where the context requires at most one word, so `%{targets}` can be confusing; stdout redirections and "-o" arguments of various tools are the main use case; also, introduce a separate field `target` that must be used instead of `targets` in those situations. (ocaml/dune#2341, @aalekseyev) - Fix dependency graph of wrapped_compat modules. Previously, the dependency on the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg) - Allow to promote executables built with an `executable` stanza (ocaml/dune#2379, @diml) - When instantiating an implementation with a variant, make sure it matches virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322, @TheLortex, review by @rgrinberg) - Add a variable `%{ignoring_promoted_rules}` that is `true` when `--ingore-promoted-rules` is passed on the command line and false otherwise (ocaml/dune#2382, @diml) - Fix a bug in `future_syntax` where the characters `@` and `&` were not distinguished in the names of binding operators (`let@` was the same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml) - Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325, @rgrinberg) - Improve opam generation to include the `dune` dependncies with the minimum constraint set based on the dune language version specified in the `dune-project` file. (2383, @avsm) - The order of fields in the generated opam file now follows order preferred in opam-lib. (@avsm, ocaml/dune#2380) - Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384) - Add warning `66` to default set of warnings starting for dune projects with language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299) - Add (dialect ...) stanza (@nojb, ocaml/dune#2404) - Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412) - Do not warn about merlin files pre 1.9. This warning can only be disabled in 1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon)

@TheLortex

CHANGES: - Don't select all local implementations in `dune utop`. Instead, let the default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex, review by @rgrinberg) - Check that selected implementations (either by variants or default implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by @rgrinberg) - Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml) - Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The set of variants is now computed when the virtual library is installed. Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex, review by @diml) - Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275, @rgrinberg) - Fix error message for missing C++ sources. The `.cc` extension was always ignored before. (ocaml/dune#2275, @rgrinberg) - Add `$ dune init project` subcommand to create project boilerplate according to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder) - Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo) - Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review by @rgrinberg and @diml) - Add a `cinaps` extension and stanza for better integration with the [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269, @diml) - Allow to embed build info in executables such as version and list and version of statically linked libraries (ocaml/dune#2224, @diml) - Set version in `META` and `dune-package` files to the one read from the vcs when no other version is available (ocaml/dune#2224, @diml) - Add a variable `%{target}` to be used in situations where the context requires at most one word, so `%{targets}` can be confusing; stdout redirections and "-o" arguments of various tools are the main use case; also, introduce a separate field `target` that must be used instead of `targets` in those situations. (ocaml/dune#2341, @aalekseyev) - Fix dependency graph of wrapped_compat modules. Previously, the dependency on the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg) - Allow to promote executables built with an `executable` stanza (ocaml/dune#2379, @diml) - When instantiating an implementation with a variant, make sure it matches virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322, @TheLortex, review by @rgrinberg) - Add a variable `%{ignoring_promoted_rules}` that is `true` when `--ingore-promoted-rules` is passed on the command line and false otherwise (ocaml/dune#2382, @diml) - Fix a bug in `future_syntax` where the characters `@` and `&` were not distinguished in the names of binding operators (`let@` was the same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml) - Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325, @rgrinberg) - Improve opam generation to include the `dune` dependncies with the minimum constraint set based on the dune language version specified in the `dune-project` file. (2383, @avsm) - The order of fields in the generated opam file now follows order preferred in opam-lib. (@avsm, ocaml/dune#2380) - Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384) - Add warning `66` to default set of warnings starting for dune projects with language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299) - Add (dialect ...) stanza (@nojb, ocaml/dune#2404) - Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412) - Do not warn about merlin files pre 1.9. This warning can only be disabled in 1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon) - Add a new `inline_tests` field in the env stanza to control inline_tests framework with a variable (ocaml/dune#2313, @mlasson, original idea by @diml, review by @rgrinberg). - New binary kind `js` for executables in order to explicitly enable Javascript targets, and a switch `(explicit_js_mode)` to require this mode in order to declare JS targets corresponding to executables. (ocaml/dune#1941, @nojb)

TheLortex force-pushed the variants-2 branch from 96859e0 to edf681c Compare May 20, 2019 13:04

TheLortex marked this pull request as ready for review May 20, 2019 13:12

TheLortex changed the title ~~Variants v2 (WIP)~~ Variants v2 May 20, 2019

ghost suggested changes May 21, 2019

View reviewed changes

TheLortex force-pushed the variants-2 branch 2 times, most recently from a20befc to 9ac3609 Compare May 22, 2019 12:28

TheLortex requested review from emillon and rgrinberg as code owners May 23, 2019 13:01

TheLortex force-pushed the variants-2 branch from 5790f9b to f406a0b Compare May 27, 2019 13:04

rgrinberg force-pushed the variants-2 branch from e75a39b to 20c1e16 Compare May 31, 2019 09:47

rgrinberg reviewed May 31, 2019

View reviewed changes

src/lib.ml Outdated Show resolved Hide resolved

TheLortex force-pushed the variants-2 branch 2 times, most recently from 9f6dddd to 1ad1d27 Compare May 31, 2019 14:59

ghost suggested changes Jun 3, 2019

View reviewed changes

doc/variants.rst Outdated Show resolved Hide resolved

src/lib.ml Outdated Show resolved Hide resolved

TheLortex force-pushed the variants-2 branch from 0f02f20 to 5a41633 Compare June 4, 2019 14:03

ghost approved these changes Jun 4, 2019

View reviewed changes

Variants v2

fef8ce7

* Explicit computation of variant list at virtual library build time * Implicit detection of local variants * Explicit declaration of external variants Signed-off-by: Lucas Pluvinage <lucas.pluvinage@gmail.com>

rgrinberg force-pushed the variants-2 branch from 5a41633 to fef8ce7 Compare June 5, 2019 23:37

rgrinberg merged commit fd498ee into master Jun 6, 2019

rgrinberg mentioned this pull request Jul 18, 2019

[new release] dune-build-info and dune (1.11.0) ocaml/opam-repository#14542

Closed

rgrinberg mentioned this pull request Jul 22, 2019

[new release] dune-build-info and dune (1.11.0) ocaml/opam-repository#14567

Merged

Alizter deleted the variants-2 branch November 10, 2022 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variants v2#2169

Variants v2#2169
rgrinberg merged 1 commit intomasterfrom
variants-2

TheLortex commented May 17, 2019

Uh oh!

TheLortex commented May 20, 2019

Uh oh!

ghost left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented May 22, 2019

Uh oh!

TheLortex commented May 23, 2019

Uh oh!

ghost commented May 23, 2019 •

edited by ghost

Loading

Uh oh!

Uh oh!

TheLortex commented May 31, 2019

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

TheLortex commented May 17, 2019

Uh oh!

TheLortex commented May 20, 2019

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented May 22, 2019

Uh oh!

TheLortex commented May 23, 2019

Uh oh!

ghost commented May 23, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TheLortex commented May 31, 2019

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost commented May 23, 2019 •

edited by ghost

Loading