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.

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

Revert workspace detection #5206

Merged
merged 4 commits into from
Nov 23, 2021
Merged

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Nov 22, 2021

As discussed in the last dev meeting this PR restore the old root detection behavior: Dune will keep looking for an outermost dune-workspace file instead of stopping on the first one.

This is necessary to have dune ocaml-merlin provide configuration for nested vendored project with their own workspace file.

Fixes #5187

(partial revert of #4921)

@voodoos voodoos requested review from a user and christinerose November 22, 2021 10:31
@bobot
Copy link
Collaborator

bobot commented Nov 22, 2021

I forgot during the meeting, but I wonder if @rgrinberg had a similar issue with RPC (find where the rpc socket, i.e. the data provider is) , and proposed a way.

@voodoos voodoos added this to the 3.0.0 milestone Nov 22, 2021
@ghost
Copy link

ghost commented Nov 22, 2021

I think what you are thinking of is discovering the list of Dune servers currently running. That won't be of much help here as merlin works even without a Dune server running. In particular, when users call dune builld repeatedly as one-off commands without ever starting it in watch mode.

bin/workspace_root.ml Outdated Show resolved Hide resolved
@ghost ghost mentioned this pull request Nov 22, 2021
@ghost
Copy link

ghost commented Nov 22, 2021

There was a weird merge in #4913. I'm re-working the doc.

@ghost
Copy link

ghost commented Nov 22, 2021

@voodoos I pushed a commit to main to delete some doc that was re-added by accident in #4923. Could you rebase you PR?

@voodoos voodoos force-pushed the revert-workspace-detection branch from a107548 to 4cc246c Compare November 22, 2021 12:49
@voodoos
Copy link
Collaborator Author

voodoos commented Nov 22, 2021

@jeremiedimino done

I had to reword a lot so it might be worth it to read it again to check that the behaviour is clear.
I think it is important to explain the precedence of dune-workspace over dune-project as it has bitten me and other users.

@ghost
Copy link

ghost commented Nov 22, 2021

Agreed. @christinerose could you have a look at the documentation changes?

Copy link
Collaborator

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Suggested a few grammar things.
Looks great!

doc/usage.rst Outdated Show resolved Hide resolved
doc/usage.rst Outdated Show resolved Hide resolved
doc/usage.rst Outdated Show resolved Hide resolved
doc/usage.rst Outdated Show resolved Hide resolved
doc/usage.rst Outdated Show resolved Hide resolved
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!

voodoos and others added 4 commits November 23, 2021 13:51
Signed-off-by: Ulysse Gérard <[email protected]>
Signed-off-by: Jeremie Dimino <[email protected]>
Signed-off-by: Ulysse Gérard <[email protected]>
Co-authored-by: Christine Rose <[email protected]>
Signed-off-by: Ulysse Gérard <[email protected]>
@voodoos voodoos force-pushed the revert-workspace-detection branch from ba0bd71 to 44ed978 Compare November 23, 2021 12:52
@voodoos voodoos merged commit 44d4bd4 into ocaml:main Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore not stopping at first dune-workspace
4 participants