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

Remove variants #3491

Merged
merged 6 commits into from
May 29, 2020
Merged

Remove variants #3491

merged 6 commits into from
May 29, 2020

Conversation

rgrinberg
Copy link
Member

@rgrinberg rgrinberg commented May 14, 2020 •
edited
Loading

The experimental variants feature is removed in 2.6. In particular:

  • default implementations are a standard part of the language
  • (using library_variants) is a nice error message
  • Fix the semantics of default_implementation. virtual library and default implementation must exist in the same package.

@jeremiedimino do we need nice error messages when users use write (variant ..) fields? Or is it enough to say that the feature was deleted on the (using ..) declaration?

@rgrinberg rgrinberg requested a review from a user May 14, 2020 22:49
@rgrinberg rgrinberg requested review from emillon and nojb as code owners May 14, 2020 22:49
@rgrinberg rgrinberg force-pushed the remove-variants branch 2 times, most recently from 11ae16a to 91214f7 Compare May 15, 2020 02:59
@ghost
Copy link

ghost commented May 19, 2020

Let's go for the second option. We don't need to go out of our way for experimental features. In such cases, the code matters more.

ghost
ghost suggested changes May 19, 2020
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.

Nice, I like PRs with a lot of read :)

doc/dune-files.rst Outdated Show resolved Hide resolved
doc/variants.rst Outdated Show resolved Hide resolved
doc/variants.rst Outdated Show resolved Hide resolved
src/dune/dune_file.ml Outdated Show resolved Hide resolved
@rgrinberg rgrinberg requested a review from ejgallego as a code owner May 21, 2020 00:08
@rgrinberg rgrinberg added this to the 2.6 milestone May 23, 2020
rgrinberg
rgrinberg commented May 28, 2020
View reviewed changes
doc/variants.rst Outdated

Default implementation
======================

This feature is also guarded by ``(using library_variants ...)``.
This feature is guarded by a different language declaration depending on the
version of dune that you're using.
Copy link
Member Author

Choose a reason for hiding this comment

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

These side notes should be moved to the bottom of the section

ghost
ghost reviewed May 28, 2020
View reviewed changes
doc/variants.rst Outdated Show resolved Hide resolved
rgrinberg
rgrinberg commented May 28, 2020
View reviewed changes
src/dune/dune_file.ml Outdated Show resolved Hide resolved
ghost
ghost suggested changes May 28, 2020
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, just need some doc tweak

@rgrinberg
Remove variants
50f2d3b 
The experimental variants feature is removed in 2.6. In particular:

* default implementations are a standard part of the language
* (using library_variants) is an error

Signed-off-by: Rudi Grinberg <[email protected]>
rgrinberg added 5 commits May 28, 2020 17:50
@rgrinberg
Update documentation
758ed57 
Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
remove accidental physical equality
c006f72 
Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
Add default_implementation package check
5fdf7c2 
Make sure that default implementations exist in the same package as
their virtual libraries

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
move experimental property to syntax
e0649ba 
Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
Handle deleted experimental syntax extensions
dad471b 
Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg merged commit df70a93 into ocaml:master May 29, 2020
@rgrinberg rgrinberg mentioned this pull request Jun 6, 2020
Merged
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jun 6, 2020
@rgrinberg
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
f80ea5a 
…lugin, dune-private-libs and dune-glob (2.6.0)

CHANGES:

- Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2`
  results in an error. (ocaml/dune#3444, fix ocaml/dune#3443, @bikallem)

- Add and `enabled_ if` field to the `install` stanza. Enforce the same variable
  restrictions for `enabled_if` fields in the `executable` and `install` stanzas
  than in the `library` stanza. When using dune lang < 2.6, the usage of
  forbidden variables in executables stanzas with only trigger a warning to
  maintain compatibility. (ocaml/dune#3408 and ocaml/dune#3496, fixes ocaml/dune#3354, @voodoos)

- Insert a constraint one the version of dune when the user explicitly
  specify the dependency on dune in the `dune-project` file (ocaml/dune#3434 ,
  fixes ocaml/dune#3427, @diml)

- Generate correct META files for sub-libraries (of the form `lib.foo`) that
  contain .js runtime files. (ocaml/dune#3445, @hhugo)

- Add a `(no-infer ...)` action that prevents inference of targets and
  dependencies in actions. (ocaml/dune#3456, fixes ocaml/dune#2006, @roddyyaga)

- Correctly infer targets for the `diff?` action. (ocaml/dune#3457, fixes ocaml/dune#2990, @greedy)

- Fix `$ dune print-rules` crashing (ocaml/dune#3459, fixes ocaml/dune#3440, @rgrinberg)

- Simplify js_of_ocaml rules using js_of_ocaml.3.6 (ocaml/dune#3375, @hhugo)

- Add a new `ocaml-merlin` subcommand that can be used by Merlin to get
  configuration directly from dune instead of using `.merlin` files. (ocaml/dune#3395,
  @voodoos)

- Remove experimental variants feature and make default implementations part of
  the language (ocaml/dune#3491, fixes ocaml/dune#3483, @rgrinberg)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jun 7, 2020
@rgrinberg
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
411eb5d 
…lugin, dune-private-libs and dune-glob (2.6.0)

CHANGES:

- Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2`
  results in an error. (ocaml/dune#3444, fix ocaml/dune#3443, @bikallem)

- Add and `enabled_ if` field to the `install` stanza. Enforce the same variable
  restrictions for `enabled_if` fields in the `executable` and `install` stanzas
  than in the `library` stanza. When using dune lang < 2.6, the usage of
  forbidden variables in executables stanzas with only trigger a warning to
  maintain compatibility. (ocaml/dune#3408 and ocaml/dune#3496, fixes ocaml/dune#3354, @voodoos)

- Insert a constraint one the version of dune when the user explicitly
  specify the dependency on dune in the `dune-project` file (ocaml/dune#3434 ,
  fixes ocaml/dune#3427, @diml)

- Generate correct META files for sub-libraries (of the form `lib.foo`) that
  contain .js runtime files. (ocaml/dune#3445, @hhugo)

- Add a `(no-infer ...)` action that prevents inference of targets and
  dependencies in actions. (ocaml/dune#3456, fixes ocaml/dune#2006, @roddyyaga)

- Correctly infer targets for the `diff?` action. (ocaml/dune#3457, fixes ocaml/dune#2990, @greedy)

- Fix `$ dune print-rules` crashing (ocaml/dune#3459, fixes ocaml/dune#3440, @rgrinberg)

- Simplify js_of_ocaml rules using js_of_ocaml.3.6 (ocaml/dune#3375, @hhugo)

- Add a new `ocaml-merlin` subcommand that can be used by Merlin to get
  configuration directly from dune instead of using `.merlin` files. (ocaml/dune#3395,
  @voodoos)

- Remove experimental variants feature and make default implementations part of
  the language (ocaml/dune#3491, fixes ocaml/dune#3483, @rgrinberg)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jun 7, 2020
@rgrinberg
[new release] dune-build-info, dune, dune-configurator, dune-action-p…
feeccf5 
…lugin, dune-private-libs and dune-glob (2.6.0)

CHANGES:

- Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2`
  results in an error. (ocaml/dune#3444, fix ocaml/dune#3443, @bikallem)

- Add and `enabled_ if` field to the `install` stanza. Enforce the same variable
  restrictions for `enabled_if` fields in the `executable` and `install` stanzas
  than in the `library` stanza. When using dune lang < 2.6, the usage of
  forbidden variables in executables stanzas with only trigger a warning to
  maintain compatibility. (ocaml/dune#3408 and ocaml/dune#3496, fixes ocaml/dune#3354, @voodoos)

- Insert a constraint one the version of dune when the user explicitly
  specify the dependency on dune in the `dune-project` file (ocaml/dune#3434 ,
  fixes ocaml/dune#3427, @diml)

- Generate correct META files for sub-libraries (of the form `lib.foo`) that
  contain .js runtime files. (ocaml/dune#3445, @hhugo)

- Add a `(no-infer ...)` action that prevents inference of targets and
  dependencies in actions. (ocaml/dune#3456, fixes ocaml/dune#2006, @roddyyaga)

- Correctly infer targets for the `diff?` action. (ocaml/dune#3457, fixes ocaml/dune#2990, @greedy)

- Fix `$ dune print-rules` crashing (ocaml/dune#3459, fixes ocaml/dune#3440, @rgrinberg)

- Simplify js_of_ocaml rules using js_of_ocaml.3.6 (ocaml/dune#3375, @hhugo)

- Add a new `ocaml-merlin` subcommand that can be used by Merlin to get
  configuration directly from dune instead of using `.merlin` files. (ocaml/dune#3395,
  @voodoos)

- Remove experimental variants feature and make default implementations part of
  the language (ocaml/dune#3491, fixes ocaml/dune#3483, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon Awaiting requested review from emillon

@nojb nojb Awaiting requested review from nojb

@ejgallego ejgallego Awaiting requested review from ejgallego

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

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