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

Allow to set up merlin in a variant of the default context #4145

Merged
merged 4 commits into from
Apr 14, 2021
Merged

Allow to set up merlin in a variant of the default context #4145

merged 4 commits into from
Apr 14, 2021

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Jan 21, 2021

This is a "reopening" of #4132 from @TheLortex rebased on master.

I added a test case, bumped dune lang version to 2.9 and removed what I think was a redundant test (72df296).

@voodoos voodoos changed the title Merlin default field Allow to set up merlin in a variant of the default context Jan 21, 2021
@voodoos
Copy link
Collaborator Author

voodoos commented Jan 21, 2021

DCO is failing because the first commit is not signed by myself but @TheLortex

@voodoos voodoos added this to the 2.9 milestone Jan 21, 2021
@emillon
Copy link
Collaborator

emillon commented Jan 22, 2021

I think you can fix that by changing the commit author for that particular commit (cherry-pick should be doing that?).

emillon
emillon reviewed Jan 22, 2021
View reviewed changes
dune-project Outdated Show resolved Hide resolved
doc/dune-files.rst Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

What happens when we select more than one context as (merlin)?

@voodoos
Copy link
Collaborator Author

voodoos commented Jan 25, 2021 •
edited
Loading

What happens when we select more than one context as (merlin)?

It fails at parsing-time. I added a test to illustrate it.
The check is done here:

dune/src/dune_rules/workspace.ml

Lines 421 to 429 in 4e01186

match (ctx, acc) with
| Opam { base = { merlin = true; _ }; _ }, Some _
| Default { merlin = true; _ }, Some _ ->
User_error.raise ~loc:(Context.loc ctx)
[ Pp.text "you can only have one context for merlin" ]
| Opam { base = { merlin = true; _ }; _ }, None
| Default { merlin = true; _ }, None ->
Some name
| _ -> acc)

@ejgallego ejgallego added this to Pending Backport in Release 2.9 Apr 8, 2021
@ejgallego ejgallego moved this from Pending Backport to Pending Approval in Release 2.9 Apr 8, 2021
@voodoos voodoos force-pushed the merlin-default-field branch 3 times, most recently from 2494b55 to b8cf576 Compare April 9, 2021 14:51
rgrinberg
rgrinberg approved these changes Apr 9, 2021
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM. Needs a CHANGES entry.

voodoos and others added 4 commits April 14, 2021 15:19
@voodoos
Add test
36a845c 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Test with parsing failure on mutltiple merlin contexts
9f8ece6 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@TheLortex @voodoos
Allow to set up merlin in a variant of the default context.
3cbee3f 
Signed-off-by: Lucas Pluvinage <lucas@tarides.com>
@voodoos
Add changelog entry
1ceb155 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos voodoos merged commit c48b997 into ocaml:main Apr 14, 2021
@voodoos voodoos moved this from Pending Approval to Pending Backport PR in Release 2.9 Apr 14, 2021
voodoos added a commit to voodoos/dune that referenced this pull request Apr 20, 2021
@voodoos @TheLortex
Allow to set up merlin in a variant of the default context (ocaml#4145)
124bb34 
* Add test
* Test with parsing failure on mutltiple merlin contexts
* Allow to set up merlin in a variant of the default context.
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Co-authored-by: Lucas Pluvinage <lucas@tarides.com>
@voodoos voodoos removed this from Pending Backport PR in Release 2.9 Apr 20, 2021
voodoos added a commit to voodoos/dune that referenced this pull request Jun 4, 2021
@voodoos @TheLortex
Allow to set up merlin in a variant of the default context (ocaml#4145)
3cdb38a 
* Add test
* Test with parsing failure on mutltiple merlin contexts
* Allow to set up merlin in a variant of the default context.
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Co-authored-by: Lucas Pluvinage <lucas@tarides.com>
ejgallego pushed a commit that referenced this pull request Jun 4, 2021
@voodoos @TheLortex
Allow to set up merlin in a variant of the default context (#4145) (#…
8c30c9d 
…4502)

* Add test
* Test with parsing failure on mutltiple merlin contexts
* Allow to set up merlin in a variant of the default context.
* Add changelog entry

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Co-authored-by: Lucas Pluvinage <lucas@tarides.com>

Co-authored-by: Lucas Pluvinage <lucas@tarides.com>
@ejgallego ejgallego mentioned this pull request Jun 21, 2021
Merged
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 21, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
da0714f 
…ator, dune-private-libs, dune and dune-build-info (2.9-rc1)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 29, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
a13f842 
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jul 1, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
96c14a5 
…ator, dune-private-libs, dune and dune-build-info (2.9.0)

CHANGES:

- Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon)

- Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb)

- Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos)

- Allow to set up merlin in a variant of the default context
  (ocaml/dune#4145, @TheLortex, @voodoos)

- Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon)

- Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01)

- Fix generation of merlin configuration when using `(include_subdirs
  unqualified)` on Windows (ocaml/dune#4745, @nojb)

- Fix bug for the install of Coq native files when using `(include_subdirs qualified)`
  (ocaml/dune#4753, @ejgallego)

- Allow users to specify install target directories for `doc` and
  `etc` sections. We add new options `--docdir` and `--etcdir` to both
  Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723,
  @ejgallego, thanks to @JasonGross for reporting this issue)

- Fix issue where Dune would ignore `(env ... (coq (flags ...)))`
  declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg)

- Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to
  the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

- Fix a bug where instrumentation flags would be added even if the
  instrumentatation was disabled (@nojb, ocaml/dune#4770)

- Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable
  `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632)

- Fix installation with opam of package with dune sites. The `.install` file is
  now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645,
  @bobot, reported by @kit-ty-kate ocaml/dune#4198)

- Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun
  ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon emillon left review comments

@rgrinberg rgrinberg rgrinberg approved these changes

@nojb nojb Awaiting requested review from nojb

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@voodoos @emillon @rgrinberg @TheLortex