Skip to content

Fix Merlin configuration server in a non-default build context#4127

Merged
voodoos merged 6 commits intoocaml:mainfrom
voodoos:merlin-github4125
Jan 20, 2021
Merged

Fix Merlin configuration server in a non-default build context#4127
voodoos merged 6 commits intoocaml:mainfrom
voodoos:merlin-github4125

Conversation

@voodoos
Copy link
Copy Markdown
Collaborator

@voodoos voodoos commented Jan 15, 2021

This PR fixes #4125.

The workspace initialization was wrong in the dune ocaml-merlin binary.
I added a test which is disabled by default as it requires a global switch default to exist.

@voodoos voodoos changed the title Merlin github4125 Fix Merlin configuration server in a non-default build context Jan 15, 2021
@voodoos voodoos requested a review from rgrinberg January 15, 2021 17:48
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Jan 16, 2021

It should be possible to get the tests to work by default. Have a look at the workspace tests for multi contexts tests.

@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Jan 18, 2021

It should be possible to get the tests to work by default. Have a look at the workspace tests for multi contexts tests.

Looking at other tests (grep (opam ) I only found two:

  • One (workspaces.t/opma) is disabled, saying

A workspace context can be defined using an opam switch. This test is disabled
because we don't really have a way to mock an opam switch.

  • And the other uses a switch named default so dune's test-suite already assumes such a switch is present on the system. I will use that switch then.

@TheLortex TheLortex mentioned this pull request Jan 18, 2021
Closed
ghost
ghost reviewed Jan 18, 2021
View reviewed changes
@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Jan 19, 2021

I cherry picked the commit from #4132 adding the possibility to specify which variant of the default context is used for Merlin configuration.

@voodoos voodoos requested a review from a user January 19, 2021 15:00
@ghost
Copy link
Copy Markdown

ghost commented Jan 19, 2021

I cherry picked the commit from #4132 adding the possibility to specify which variant of the default context is used for Merlin configuration.

I don't think we should do this. It is going to make it more difficult to backport only the bugfix in 2.8. We could backport both the bugfix and the new feature, but that is going to break our usual compatibility rules. Let's be strict here and add the (merlin) field in 2.9. If we want this feature soon, we can just release 2.9 soon.

ghost
ghost suggested changes Jan 19, 2021
View reviewed changes
@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Jan 19, 2021

I cherry picked the commit from #4132 adding the possibility to specify which variant of the default context is used for Merlin configuration.

I don't think we should do this. It is going to make it more difficult to backport only the bugfix in 2.8. We could backport both the bugfix and the new feature, but that is going to break our usual compatibility rules. Let's be strict here and add the (merlin) field in 2.9. If we want this feature soon, we can just release 2.9 soon.

You are right, I dropped it.

ghost
ghost reviewed Jan 20, 2021
View reviewed changes
ghost
ghost approved these changes Jan 20, 2021
View reviewed changes
Copy link
Copy Markdown

@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 to merge once changelog is updated

@voodoos
Add test for opam contexts
8c8e6d9 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Disable log creation
cf19fcc 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Add CHANGES entry
cf50ac1 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Use safer default workspace init.
e3c00ce 
In the absence of a specified dune-workspace file, always check for one witht the standard name.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Pass params to workspace init if present
ce3d7d8 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos
Test update
825f0be 
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos voodoos merged commit 8152606 into ocaml:main Jan 20, 2021
ghost pushed a commit that referenced this pull request Jan 21, 2021
@voodoos @jeremiedimino
Fix dune ocaml-merlin context detection (#4127)
cf41a35 
* Disable log creation
* Use safer default workspace initialisation.
* Add test for opam contexts

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@ghost ghost mentioned this pull request Jan 21, 2021
Merged
bobot
bobot reviewed Jan 27, 2021
View reviewed changes
test/blackbox-tests/test-cases/merlin/github4125.t/dune-workspace
(opam
(name cross)
(switch default)
(merlin)))
Copy link
Copy Markdown
Collaborator

@bobot bobot Jan 27, 2021

Choose a reason for hiding this comment

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

I believe this test only work if we have a switch named default in our opam configuration. @voodoos

Copy link
Copy Markdown
Collaborator Author

@voodoos voodoos Jan 27, 2021

Choose a reason for hiding this comment

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

Sorry for that, I thought other tests were already relying on such a switch but they must have been disabled since.
@rgrinberg said it might be possible to test this kind of feature but I did not found examples of such things in the test-base.

I think disabling this test is fine if don't find a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bobot bobot bobot left review comments

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Merlin does not work when enabled in a non-default build context

3 participants

@voodoos @rgrinberg @bobot