Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535

voodoos · 2024-05-15T15:13:30Z

With release 5.2 the compiler introduces a new flag -H to introduce dependencies that are required for linking but should be hidden from the currently built module. Merlin for 5.2 follows with two new configuration directives: BH and SH that allows to provide hidden build and source paths.

This PR effectively takes advantage of these new directives to correctly communicate hidden dependencies to Merlin.

All versions of Merlin since 4.6 will accept that flag, but only the preview versions for 5.2 can actually take advantage of this additional information.

nojb · 2024-05-17T13:59:12Z

Hello @voodoos, thanks for this PR. Note that #9333 is being worked on at the moment (cc @MA0010). Once that is done, you will be able to simplify the patch because the lists of requires given by Compilation_context.requires will be annotated with a flag indicating whether each dependency is direct or transitive (= hidden), and you won't need to do this computation "just" for Merlin. Moreover, I think it would be great if we could merge this PR at the same time that Dune itself is able to support -H.

Accordingly, I suggest to suspend this PR until #9333 is done; does that sound OK? I hope that there will be a PR for #9333 in the next week or so.

nojb · 2024-06-28T08:08:44Z

@voodoos #10644 has been merged; so it should now be possible to finalize this PR by using Compilation_context.requires_hidden. Thanks for waiting!

Leonidas-from-XIV · 2024-10-04T14:55:10Z

Hi @voodoos could you rebase the PR? We were talking about this in the last dune-dev meeting and it would be nice to have it as part of the 3.17 release.

test/blackbox-tests/test-cases/merlin/implicit-transitive-deps.t/run.t

voodoos · 2024-10-07T12:16:36Z

@Leonidas-from-XIV rebased and signed-off

Leonidas-from-XIV

Could you please add a changelog entry? This is one of the highlights of the release, would be good for people to be aware of it :)

src/dune_rules/merlin/merlin.ml

Use new BH / SH directives Make specific tests for OCaml 5.2 Signed-off-by: Ulysse Gérard <[email protected]>

Signed-off-by: Ulysse Gérard <[email protected]>

test/blackbox-tests/test-cases/overlapping-deps.t

Leonidas-from-XIV · 2024-10-10T14:27:45Z

test/blackbox-tests/test-cases/overlapping-deps.t

+  -> required by _build/default/proj2/.merlin-conf/lib-bar
+  -> required by _build/default/proj2/bar.cma
+  -> required by %{cma:proj2/bar} at command line:1
+  [1]


Interesting that this triggers the error now, this could be a regression for the release but it seems like the error is what should've happened here anyway.

Leonidas-from-XIV · 2024-10-10T14:29:45Z

test/blackbox-tests/test-cases/deprecated-library-name/features.t

@@ -96,8 +96,7 @@ that wasn't found:
  1 | (executable (name prog) (libraries a))
                                         ^
  Error: Library "a" not found.
-  -> required by _build/default/c/.prog.eobjs/byte/dune__exe__Prog.cmi
-  -> required by _build/default/c/.prog.eobjs/native/dune__exe__Prog.cmx
+  -> required by _build/default/c/.merlin-conf/exe-prog


My main concern is that this changes so many tests, but given the dependency that is getting reported is an internal one and stays an internal one I believe this is not an issue.

It looks like the PR changes some evaluation order that makes Dune errors after following a different branch. Not sure what causes this however...

@rgrinberg Can you please weight in whether this is an issue? It doesn't seem to be wrong, but maybe the order is important in some way I am not aware of?

I found the difference: before we only "peeked" (with Resolve.peek) at the dependencies and now we "read" them.

I will revert that change as is seems wrong to introduce dependencies here.

Co-authored-by: Marek Kubica <[email protected]> Signed-off-by: Ulysse <[email protected]>

Signed-off-by: Ulysse Gérard <[email protected]>

rgrinberg · 2024-10-10T19:48:39Z

@nojb I think you've assigned yourself to this PR. Do you mind taking a final look, and if everything is OK, merging?

rgrinberg · 2024-10-10T19:49:51Z

doc/changes/10535.md

@@ -0,0 +1,2 @@
+- Use Merlin new configuration directives `BH` and `SH` for hidden dependencies.


I would mention that hidden dependencies correspond to the -h flag instead of internal merlin directives which most people are unaware of.

Indeed, it is always better to write Changes entries from the point of view of the user, not the developer. In this case, something like

- Make Merlin/OCaml-LSP aware of "hidden" dependencies used by `(implicit_transitive_deps false)` via the `-H` compiler flag.

would be more useful for the occasional reader.

I took the liberty of tweaking the Changes entry in this manner. Planning to merge once CI passes. Thanks!

nojb

LGTM

nojb · 2024-10-11T04:30:06Z

doc/changes/10535.md

@@ -0,0 +1,2 @@
+- Use Merlin new configuration directives `BH` and `SH` for hidden dependencies.


Indeed, it is always better to write Changes entries from the point of view of the user, not the developer. In this case, something like

- Make Merlin/OCaml-LSP aware of "hidden" dependencies used by `(implicit_transitive_deps false)` via the `-H` compiler flag.

would be more useful for the occasional reader.

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

voodoos · 2024-10-11T08:15:45Z

Thank you @nojb !

voodoos mentioned this pull request May 15, 2024

[5.2] Build occurrences index for Merlin #10422

Merged

nojb self-assigned this May 17, 2024

nojb mentioned this pull request Jun 11, 2024

Add support for -H compiler flag #10644

Merged

voodoos force-pushed the merlin-hidden-deps branch from e2531ee to bcccf15 Compare July 1, 2024 13:35

rgrinberg added the merlin label Jul 15, 2024

voodoos force-pushed the merlin-hidden-deps branch from bcccf15 to 257a373 Compare October 4, 2024 16:11

voodoos commented Oct 4, 2024

View reviewed changes

test/blackbox-tests/test-cases/merlin/implicit-transitive-deps.t/run.t Show resolved Hide resolved

voodoos force-pushed the merlin-hidden-deps branch from a5b1ea6 to e26d0ce Compare October 7, 2024 12:13

voodoos force-pushed the merlin-hidden-deps branch from 7465100 to b3cc73e Compare October 7, 2024 14:55

Leonidas-from-XIV reviewed Oct 7, 2024

View reviewed changes

src/dune_rules/merlin/merlin.ml Outdated Show resolved Hide resolved

voodoos force-pushed the merlin-hidden-deps branch from 3531597 to b4641d9 Compare October 9, 2024 16:24

voodoos added 8 commits October 10, 2024 16:21

Add all dependency source dirs to Merlin's configuration.

75979a2

Use new BH / SH directives Make specific tests for OCaml 5.2 Signed-off-by: Ulysse Gérard <[email protected]>

Promote test results for OCaml 5.1

415c35c

Signed-off-by: Ulysse Gérard <[email protected]>

fmt

b1a5400

Signed-off-by: Ulysse Gérard <[email protected]>

Promote changed tests backtraces.

fe4819f

Signed-off-by: Ulysse Gérard <[email protected]>

Bump Merlin conf version

75ade1b

Signed-off-by: Ulysse Gérard <[email protected]>

Promote more consistent behaviour of overlap checks

2f436ef

Signed-off-by: Ulysse Gérard <[email protected]>

Merlin: Remove unused source_dirs argument.

cd4550b

Signed-off-by: Ulysse Gérard <[email protected]>

Add changelog entry for ocaml#10535

775915d

Signed-off-by: Ulysse Gérard <[email protected]>

voodoos force-pushed the merlin-hidden-deps branch from b4641d9 to 775915d Compare October 10, 2024 14:21

Leonidas-from-XIV reviewed Oct 10, 2024

View reviewed changes

voodoos and others added 2 commits October 10, 2024 16:36

Fixect typo

2350eab

Co-authored-by: Marek Kubica <[email protected]> Signed-off-by: Ulysse <[email protected]>

Only peek at dependencies

6d113f5

Signed-off-by: Ulysse Gérard <[email protected]>

Leonidas-from-XIV approved these changes Oct 10, 2024

View reviewed changes

rgrinberg approved these changes Oct 10, 2024

View reviewed changes

nojb approved these changes Oct 11, 2024

View reviewed changes

nojb added 2 commits October 11, 2024 06:30

Merge branch 'main' into merlin-hidden-deps

22ad48e

Changes

feb1cd5

Signed-off-by: Nicolás Ojeda Bär <[email protected]>

nojb merged commit b39ad6d into ocaml:main Oct 11, 2024
26 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535

Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535

voodoos commented May 15, 2024

nojb commented May 17, 2024

nojb commented Jun 28, 2024

Leonidas-from-XIV commented Oct 4, 2024

voodoos commented Oct 7, 2024

Leonidas-from-XIV left a comment

Leonidas-from-XIV Oct 10, 2024

Leonidas-from-XIV Oct 10, 2024

voodoos Oct 10, 2024

Leonidas-from-XIV Oct 10, 2024

voodoos Oct 10, 2024

rgrinberg commented Oct 10, 2024

rgrinberg Oct 10, 2024

nojb Oct 11, 2024

nojb Oct 11, 2024

nojb left a comment

nojb Oct 11, 2024

voodoos commented Oct 11, 2024

		@@ -0,0 +1,2 @@
		- Use Merlin new configuration directives `BH` and `SH` for hidden dependencies.

Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535

Use new hidden deps Merlin support for handling implicit-transitive-deps false #10535

Conversation

voodoos commented May 15, 2024

nojb commented May 17, 2024

nojb commented Jun 28, 2024

Leonidas-from-XIV commented Oct 4, 2024

voodoos commented Oct 7, 2024

Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voodoos commented Oct 11, 2024