fix(melange): run melc ppx with merlin #6476

jchavarri · 2022-11-15T10:26:19Z

A couple of questions:

I am not sure what to do with to_dot_merlin. It seems all FLG directives are commented? I don't understand what's the purpose
Unsure if -bs-jsx 3 should be conditionally added, based on flags field value on library or melange.emit stanzas.

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

rgrinberg · 2022-11-15T14:57:36Z

I am not sure what to do with to_dot_merlin. It seems all FLG directives are commented? I don't understand what's the purpose

No idea. Anyway printing the .merlin is just for debugging so don't bother thinking too hard.

Unsure if -bs-jsx 3 should be conditionally added, based on flags field value on library or melange.emit stanzas.

It should be always added for emit stanzas and for libraries that are "melange only". See the condition we have for object directories already.

rgrinberg · 2022-11-15T14:58:26Z

src/dune_rules/merlin.ml

        make_directive "FLG"
-          (Sexp.List [ Atom (Pp_kind.to_flag flag); Atom args ])
+          (Sexp.List
+             [ Atom (Pp_kind.to_flag Ppx); Atom "melc -as-ppx -bs-jsx 3" ])


Shouldn't the melange ppx come first?

Isn't the merlin syntax something like FLG -ppx melc -as-ppx -bs-jsx 3? The first atom is what renders the -ppx flag next to FLG.

We should make a note to remove the -bs-jsx-3 stuff eventually. Here's my rough plan:

move the React JSX PPX out of the compiler (Add react-jsx-ppx as a separate package melange-re/melange#396)

distribute this new package with melange

nudge users to change to (preprocess (pps reactjs_jsx_ppx)) rather than (flags :standard -bs-jsx 3)

Delete this line in the merlin generated config

rgrinberg · 2022-11-15T14:59:00Z

src/dune_rules/merlin.ml

@@ -414,7 +435,7 @@ module Unprocessed = struct
        Path.Set.union src_dirs
          (Path.Set.of_list_map ~f:Path.source more_src_dirs)
      in
-      { Processed.stdlib_dir; src_dirs; obj_dirs; flags; extensions }
+      { Processed.stdlib_dir; src_dirs; obj_dirs; flags; extensions; mode }


Processed shouldn't include the mode. The flags should already contain what's needed.

Sorry, I spent some time trying to implement this, but I don't understand how it is possible.

flags includes only the compiler flags, and it's dumped in a single FLG directive here:

dune/src/dune_rules/merlin.ml

Lines 92 to 95 in 75d5b40

| flags ->

[ make_directive "FLG"

(Sexp.List (List.map ~f:(fun s -> Sexp.Atom s) flags))

]

We want melange FLG to be a separate directive. I can replace mode field in processed with something like melange_ppx_flag of type string list option or similar. In any case, something else needs to be passed to Processed in order to add this new directive.

Why does it need to be a separate FLG directive? We manage to put all the other flags in one directive.

I don't think so, there were two separate FLG directives already.

One for the OCaml flags:

dune/src/dune_rules/merlin.ml

Lines 93 to 95 in 0280c9e

[ make_directive "FLG"

(Sexp.List (List.map ~f:(fun s -> Sexp.Atom s) flags))

]

Another for the pp (or ppx) flags:

dune/src/dune_rules/merlin.ml

Lines 100 to 102 in 0280c9e

make_directive "FLG"

(Sexp.List [ Atom (Pp_kind.to_flag flag); Atom args ])

:: flags

I don't understand how to inject the melc ppx in either of those. The first one (afaiu) is to tell Merlin which flags are used for the OCaml compiler. The second one is for user-defined ppxs. We need a new directive to tell Merlin "you also need to run this ppx", as it's a separate process: it's not part of the compiler flags, nor a ppx that is user defined.

Do you or @anmonteiro see a way to "blend" it with any of the two other existing directives?

@rgrinberg Any thoughts on the above? Considering there were already two FLG directives, should melc ppx be made part of any of them or could it exist on its own? If anything, it should be part of the second, but I don't think it's possible at the moment as it's not a user defined ppx (so can't be compiled with ppx driver).

This is hacky but it looks good enough for now. A few points:

We shouldn't hard code the path of melc. We should use the absolute path that was resolved by dune

We should make sure that melc is actually built if it's defined in the same workspace.

In any case, if @anmonteiro is fine with this temporary hack then we can merge it.

I agree we should fix 1. & 2. before merging.

What's the temporary hack? the 2 FLG directives?

Yes. I would expect all these melange ppx's to be ported to ppxlib and enabled explicitly.

that's the goal for the ReactJS PPX, as highlighted in #6476 (comment).

I don't yet have a clear plan for the builtin PPX, which currently is very tightly integrated into the compiler.

* main: test(melange): demonstrate error in melange compilation trying to build @ALL (ocaml#6485) chore(nix): make it faster to get melange (ocaml#6347) refactor: status db style tweaks (ocaml#6478) fix: improve error message for status db (ocaml#6479) refactor: remove unused [flags] parameter (ocaml#6480) refactor(ctypes): remove pesky aliases (ocaml#6482) chore: tweak `hacking.rst` following `dune.exe` move to _boot (ocaml#6484) feature(coq): automatic detection of native chore(coq): bump Coq lang to 0.7 test: disable formatting for a single dune file (ocaml#6465) refactor: clean up module compilation (ocaml#6461) doc: add button to copy code blocks in Dune manual (ocaml#6428) refactor: deforest a set conversion (ocaml#6473) refactor: remove temporary map used for sorting (ocaml#6472) fix(melange): handle include_subdirs unqualified (ocaml#6475)

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri · 2022-11-16T09:47:40Z

It should be always added for emit stanzas and for libraries that are "melange only". See the condition we have for object directories already.

I meant something else, rather how to pass compiler flags to melange -ppx directive. I managed to do it in 5de0797 cc @anmonteiro

This reverts commit 5de0797.

jchavarri · 2022-11-16T10:25:15Z

I meant something else, rather how to pass compiler flags to melange -ppx directive. I managed to do it in 5de0797

I tested this change with vscode editor and it seems it doesn't work, I started getting errors of the kind unknown flag -as-ppx. It seems they come from merlin?

I am not sure how to fix this, so I reverted the change in 993c0ff.

anmonteiro · 2022-11-16T17:56:05Z

We don't need to pass all the compiler flags to melc --as-ppx, just the -bs-jsx 3 flag
It's not the end of the world to pass -bs-jsx 3 unconditionally, it just means that the PPX will always be applied
- I highlighted a plan to improve this in fix(melange): run melc ppx with merlin #6476 (comment)

test/blackbox-tests/test-cases/melange/merlin.t

* main: test: formatting of alternative dune files (ocaml#6567) refactor: remove Modules.is_empty (ocaml#6564) refactor: module kinds (ocaml#6562) refactor(coq): resolve lack of coqc properly Cache file contents in action builder by name. (ocaml#6555) fix: re-enable dune on older macos sdk's (ocaml#6515) fix: do not hide lib interface module (ocaml#6549) test: remove pkg-config output for reproducibility (ocaml#6543) melange: add test for ocaml flags (ocaml#6548) fix: improve virtual library error messages test: virtual library and impl locations test: alias module regression (ocaml#6544) refactor(merlin): dump config sub command (ocaml#6547)

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

anmonteiro

Looks good. Do you plan to fix the remaining TODO in Merlin.to_dot_merlin?

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri · 2022-11-24T23:31:15Z

Do you plan to fix the remaining TODO in Merlin.to_dot_merlin?

Fixed the todo and verified the output in test in
c4a6d2b.

test/blackbox-tests/test-cases/melange/merlin.t

anmonteiro · 2022-11-24T23:34:03Z

src/dune_rules/merlin.ml

+    let () =
+      match mode with
+      | `Ocaml -> ()
+      | `Melange -> print "# FLG -ppx melc -as-ppx -bs-jsx 3\n"


I think we can use the same machinery above, with quote_for_dot_merlin. See my comment in the test.

Please check 5c34973.

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

rgrinberg · 2022-11-28T17:09:48Z

Merlin might rely on a few binaries to exist (ocaml toolchain, user ppxs, etc) but I can't find an example that shows how one can define a dependency on binaries. I can only see directives B, S, FLG, STDLIB, EXCLUDE_QUERY_DIR, but idu how any of them could be used to define a dependency on a binary.

Note that user ppx's are provided as absolute paths to merlin for exactly this reason. Merlin doesn't use the ocaml toolchain (it vendors what it needs), so there's no need to worry about that.

Okay, we can skip the dependency on melc for now because we don't handle ppx cleanly either.

rgrinberg · 2022-11-28T17:10:43Z

src/dune_rules/merlin.ml

@@ -68,7 +69,9 @@ module Processed = struct

  let serialize_path = Path.to_absolute_filename

-  let to_sexp ~pp { stdlib_dir; obj_dirs; src_dirs; flags; extensions } =
+  let melc_ppx_flg = "melc -as-ppx -bs-jsx 3"


This is the offending line. It should be using serialize_path to encode melc and using the melc path we resolved to build the actual library

Also, I'm pretty sure you can provide flags as separate atoms. So that we don't have to rely on merlin to parse anything.

It should be using serialize_path to encode melc and using the melc path we resolved to build the actual library

@rgrinberg Could you help me understand this part, please? The path that is resolved in merlin points to the melc stdlib e.g. _opam/lib/melange, assuming you refer to this code

dune/src/dune_rules/merlin.ml

Line 398 in 5a09827

| `Melange -> Melange_binary.where sctx ~dir

But the path here should be to the melc command, which would be in e.g. _opam/bin/melc. How are both related? Or am I misunderstanding?

Sure, keep asking questions if something is unclear. The gist of the issue is: we should not use any relative paths in the .Merlin directive. All paths should be absolute. So, library paths, stdlib paths, preprocessors, etc should all be fully resolved.

The current code violates this with the way it refers to the melc binary.

Thanks for clarifying. I tried a fix in c6e8bd6.

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri · 2022-11-29T08:59:17Z

@rgrinberg in f698bd6 I lifted the calculation of melc flags to the Unprocessed.process function. This allows to share the flags between both to_dot_merlin and to_sexp functions. I think the code is cleaner now, but please let me know your thoughts.

jchavarri · 2022-11-29T09:01:49Z

src/dune_rules/merlin.ml

+            , match acc_melc_flags with
+              | Some _ -> acc_melc_flags
+              | None -> melc_flags ))


I am not sure about this. Should it rather be melc_flags directly? (i.e. no accumulation needed)

I guess the missing part is: what is the relationship between the paths passed as param to print_generic_dot_merlin?

src/dune_rules/merlin.ml

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

src/dune_rules/merlin.ml

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

rgrinberg · 2022-11-29T19:22:41Z

According to the tests, melc is being added even when it's not being used.

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri · 2022-11-30T08:43:36Z

@rgrinberg Fixed in dadc896. I moved the definition of melc_compiler inside the code block where melc_flags is calculated in order to follow the style guidelines about scope.

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri · 2022-11-30T14:11:58Z

I have been testing vscode-ocaml extension using this PR, but unfortunately something regressed with the most recent changes and now it does not work at all.

I see errors in lsp log of the kind:

melc: can not handle multiple files
unknown flag 3
unknown flag -as-ppx

This used to work at the beginning of the PR, when passing whole command in quotes, e.g. (FLG (-ppx "melc -as-ppx -bs-jsx 3")). Is there a way to do something similar with sexp atoms?

rgrinberg · 2022-11-30T14:36:40Z

This used to work at the beginning of the PR, when passing whole command in quotes, e.g. (FLG (-ppx "melc -as-ppx -bs-jsx 3")). Is there a way to do something similar with sexp atoms?

Not that I know. Let's just leave it as a single form for now.

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri · 2022-11-30T15:20:48Z

updated in 8bacbb7 to pass single string. Confirmed it works with editor integration.

Fixes merlin support when using the melange ppx Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

* main: (54 commits) doc: how we write `to_dyn` and `equal` (ocaml#6621) test(cache): test output of man pages test: dune utop for (subdir ..) (ocaml#6629) refactor: improve style of utop rules (ocaml#6628) test: depend on utop (ocaml#6627) refactor(stdune): Add Appendable_list.cons (ocaml#6624) doc: tighten wording in README.md test: add a repro case for ocaml#6607 (ocaml#6612) doc: cleanup status badges in README.md (ocaml#6618) doc(README): rewrap paragraphs and cleanup links coq: more resilient config query fix: link time code gen (ocaml#6606) fix(melange): run melc ppx with merlin (ocaml#6476) feature(melange): add compile_flags (ocaml#6569) refactor: move `modules: Modules.t` from `Dune_package.Lib` to `Lib_info` (ocaml#6605) Set CLICOLOR_FORCE=0 (ocaml#6608) fix: declare deps for ccomp detection (ocaml#6610) refactor: assume Cmdliner.Arg.conv is abstract (ocaml#6609) refactor: gen_rules pattern matching (ocaml#6604) Add CI for MSVC using dkml-workflows (ocaml#6540) ...

fix(melange): run melc ppx with merlin

0d971e5

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri self-assigned this Nov 15, 2022

jchavarri added the melange label Nov 15, 2022

rgrinberg reviewed Nov 15, 2022

View reviewed changes

jchavarri requested review from rgrinberg and removed request for rgrinberg November 16, 2022 09:13

jchavarri added 2 commits November 16, 2022 09:15

pass compiler flags to ppx directive

5de0797

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri requested review from rgrinberg and anmonteiro November 16, 2022 09:47

jchavarri added 2 commits November 16, 2022 10:53

Merge branch 'main' into melange/fix-melc-ppx

b5c979e

Revert "pass compiler flags to ppx directive"

993c0ff

This reverts commit 5de0797.

Merge branch 'main' into melange/fix-melc-ppx

4fd850e

rgrinberg reviewed Nov 24, 2022

View reviewed changes

test/blackbox-tests/test-cases/melange/merlin.t Outdated Show resolved Hide resolved

rgrinberg reviewed Nov 24, 2022

View reviewed changes

test/blackbox-tests/test-cases/melange/merlin.t Outdated Show resolved Hide resolved

jchavarri added 3 commits November 24, 2022 22:21

use dune ocaml merlin dump-config /home/me/code/dune

4acd9e1

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

remove flags from dune file

858dac6

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

anmonteiro mentioned this pull request Nov 24, 2022

React JSX PPX umbrella issue melange-re/melange#459

Closed

anmonteiro approved these changes Nov 24, 2022

View reviewed changes

fix dump-dot-merlin

c4a6d2b

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

anmonteiro reviewed Nov 24, 2022

View reviewed changes

test/blackbox-tests/test-cases/melange/merlin.t Outdated Show resolved Hide resolved

anmonteiro reviewed Nov 24, 2022

View reviewed changes

quote melc ppx flag

5c34973

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

rgrinberg reviewed Nov 28, 2022

View reviewed changes

jchavarri added 5 commits November 28, 2022 19:32

Merge branch 'main' into melange/fix-melc-ppx

c16eec2

merlin: print absolute path to melc

c6e8bd6

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

merlin: lift melc_flags to Processed.config

f698bd6

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

Merge branch 'main' into melange/fix-melc-ppx

9146f1b

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

merlin: fix after breakage upstream

e759821

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri commented Nov 29, 2022

View reviewed changes

rgrinberg reviewed Nov 29, 2022

View reviewed changes

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

_

97c0115

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg force-pushed the melange/fix-melc-ppx branch from a8fcf19 to 97c0115 Compare November 29, 2022 16:54

rgrinberg reviewed Nov 29, 2022

View reviewed changes

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

jchavarri added 2 commits November 29, 2022 17:18

merlin: fix tests

437fabd

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

merlin: remove opt

dfff493

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

merlin: don't add melange flags to ocaml builds

dadc896

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

Merge branch 'main' into melange/fix-melc-ppx

9bc6c7d

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

jchavarri added 2 commits November 30, 2022 15:07

merlin: pass all flags at once

8bacbb7

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

Merge branch 'main' into melange/fix-melc-ppx

2d27c73

Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

Merge branch 'main' into melange/fix-melc-ppx

8ac63ba

rgrinberg merged commit 4cfff5d into ocaml:main Nov 30, 2022

jchavarri deleted the melange/fix-melc-ppx branch November 30, 2022 21:50

jchavarri mentioned this pull request Dec 2, 2022

Create melange-ppx as standalone ppx rewriter melange-re/melange#463

Closed

moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Dec 2, 2022

fix(melange): run melc ppx with merlin (ocaml#6476)

9bfb1bf

Fixes merlin support when using the melange ppx Signed-off-by: Javier Chavarri <javier.chavarri@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(melange): run melc ppx with merlin #6476

fix(melange): run melc ppx with merlin #6476

jchavarri commented Nov 15, 2022

rgrinberg commented Nov 15, 2022

rgrinberg Nov 15, 2022

jchavarri Nov 16, 2022

anmonteiro Nov 16, 2022

rgrinberg Nov 15, 2022

jchavarri Nov 16, 2022 •

edited

Loading

rgrinberg Nov 18, 2022

jchavarri Nov 18, 2022 •

edited

Loading

jchavarri Nov 23, 2022

rgrinberg Nov 24, 2022

anmonteiro Nov 24, 2022

rgrinberg Nov 24, 2022

anmonteiro Nov 24, 2022

jchavarri commented Nov 16, 2022

jchavarri commented Nov 16, 2022

anmonteiro commented Nov 16, 2022

anmonteiro left a comment

jchavarri commented Nov 24, 2022

anmonteiro Nov 24, 2022

jchavarri Nov 24, 2022

rgrinberg commented Nov 28, 2022

rgrinberg Nov 28, 2022

rgrinberg Nov 28, 2022

jchavarri Nov 28, 2022

rgrinberg Nov 28, 2022

jchavarri Nov 29, 2022

jchavarri commented Nov 29, 2022

jchavarri Nov 29, 2022

rgrinberg commented Nov 29, 2022

jchavarri commented Nov 30, 2022

jchavarri commented Nov 30, 2022

rgrinberg commented Nov 30, 2022

jchavarri commented Nov 30, 2022

	\| flags ->
	[ make_directive "FLG"
	(Sexp.List (List.map ~f:(fun s -> Sexp.Atom s) flags))
	]

	make_directive "FLG"
	(Sexp.List [ Atom (Pp_kind.to_flag flag); Atom args ])
	:: flags

fix(melange): run melc ppx with merlin #6476

fix(melange): run melc ppx with merlin #6476

Conversation

jchavarri commented Nov 15, 2022

rgrinberg commented Nov 15, 2022

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

jchavarri Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri Nov 18, 2022 • edited Loading

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

jchavarri commented Nov 16, 2022

jchavarri commented Nov 16, 2022

anmonteiro commented Nov 16, 2022

anmonteiro left a comment

Choose a reason for hiding this comment

jchavarri commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Nov 28, 2022

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

jchavarri commented Nov 29, 2022

Choose a reason for hiding this comment

rgrinberg commented Nov 29, 2022

jchavarri commented Nov 30, 2022

jchavarri commented Nov 30, 2022

rgrinberg commented Nov 30, 2022

jchavarri commented Nov 30, 2022

jchavarri Nov 16, 2022 •

edited

Loading

jchavarri Nov 18, 2022 •

edited

Loading