Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Variants v2 #2169

Merged
merged 1 commit into from
Jun 6, 2019
Merged

Variants v2 #2169

merged 1 commit into from
Jun 6, 2019

Conversation

TheLortex
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

@TheLortex TheLortex marked this pull request as ready for review May 20, 2019 13:12
@TheLortex TheLortex changed the title Variants v2 (WIP) Variants v2 May 20, 2019
ghost
ghost suggested changes May 21, 2019
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I just left a few suggestions

src/dune_file.ml Outdated Show resolved Hide resolved
src/dune_package.ml Outdated Show resolved Hide resolved
src/dune_file.ml Outdated Show resolved Hide resolved
src/gen_rules.ml Outdated Show resolved Hide resolved
src/gen_rules.ml Outdated Show resolved Hide resolved
src/gen_rules.ml Outdated Show resolved Hide resolved
src/gen_rules.ml Outdated Show resolved Hide resolved
src/super_context.ml Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

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 TheLortex force-pushed the variants-2 branch 2 times, most recently from a20befc to 9ac3609 Compare May 22, 2019 12:28
@TheLortex
Copy link
Collaborator Author

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
Copy link

ghost commented May 23, 2019 •
edited by ghost
Loading

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.

rgrinberg
rgrinberg reviewed May 31, 2019
View reviewed changes
src/lib.ml Outdated Show resolved Hide resolved
@TheLortex TheLortex force-pushed the variants-2 branch 2 times, most recently from 9f6dddd to 1ad1d27 Compare May 31, 2019 14:59
@TheLortex
Copy link
Collaborator Author

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

ghost
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 @rgrinberg
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 rgrinberg merged commit fd498ee into master Jun 6, 2019
@rgrinberg
Copy link
Member

Thanks @TheLortex

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 18, 2019
@rgrinberg
[new release] dune-build-info and dune (1.11.0)
aa4bf16 
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)
@rgrinberg rgrinberg mentioned this pull request Jul 18, 2019
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jul 22, 2019
@rgrinberg
[new release] dune-build-info and dune (1.11.0)
7d4dbf7 
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)
@rgrinberg rgrinberg mentioned this pull request Jul 22, 2019
Merged
@Alizter Alizter deleted the variants-2 branch November 10, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@emillon emillon Awaiting requested review from emillon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TheLortex @rgrinberg