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

jbuilder utop doesn't load ppx_rewriters #346

Closed
rgrinberg opened this issue Dec 1, 2017 · 14 comments · Fixed by ocaml/opam-repository#16170
Closed

jbuilder utop doesn't load ppx_rewriters #346

rgrinberg opened this issue Dec 1, 2017 · 14 comments · Fixed by ocaml/opam-repository#16170

Comments

@rgrinberg
Copy link
Member

If we have a library with a (ppx_kind rewriter) defined in a directory, then $ jbuilder utop will not not make this ppx extension usable in the toplevel. Would be nice if that would be automatic, or at least possible somehow.

@rgrinberg
Copy link
Member Author

@diml any tips on how to make this work? I would like to get this working if it isn't too hard.

@ghost
Copy link

ghost commented Aug 6, 2018

We could pass the same ppx command we pass to merlin to utop.

@Drup
Copy link

Drup commented Jan 29, 2020

Bump, I got the problem again when I try to debug a PPX I was working on, and wanted to see the syntax tree interactively!

@ghost
Copy link

ghost commented Feb 3, 2020

Yh, let's try to do that soon. That shouldn't be too difficult. @Drup or anyone else, if you'd like to work on it I can provide some pointers.

@ghost
Copy link

ghost commented Mar 23, 2020

Here are a few pointers:

  • src/dune/utop.ml: this file contains the high-level logic for building the custom utop.exe binary that is invoked by dune utop. In particular libs_under_dir collects all the libraries that will be made available in dune utop
  • Lib_info.kind will tell you if a library is a ppx rewriter
  • src/dune/toplevel.ml contains the generator for the code of the custom utop.exe program. The generator is Source.pp_ml

Essentially, what you want to do is:

  • edit Source.pp_ml in toplevel.ml to setup a ppx rewriter in the custom utop binary. You can do that by generating Clflags.all_ppx := ["<preprocessor command>"]
  • to obtain the ppx command, you can do something similar to what the pp_flags function does in src/dune/merlin.ml. (You can ignore the Action and No_preprocessing branch)

Then there will be something more to do for runtime dependencies, I'll describe that later.

/cc @stephanieyou

@rgrinberg
Copy link
Member Author

rgrinberg commented Mar 24, 2020

Does it make sense to add a preprocessing field to the toplevel stanza first? Something like this:

(toplevel
 (name foo)
 (libraries foo b ar)
 (preprocessing (pps x y z))

This would create a toplevel with x, y, z as the preprocessor. It should be quite easy to adopt $ dune utop to have this as well as the two features share a code path to create the toplevel binary.

(The reason this wasn't brought up earlier is that this issue predates the toplevel stanza)

@ghost
Copy link

ghost commented Mar 24, 2020

Indeed, that's a good idea! @rgrinberg could you give a few pointers to @stephanieyou for how to add such a field?

@rgrinberg
Copy link
Member Author

Sure. At first, it's better to start with the advice that follows:

Essentially, what you want to do is:

Then, a pp field needs to be added to the toplevel stanza. It's quite similar to the pps field for executables and libraries so you should reuse code in dune_file.ml to add it. Clflags.all_ppx is only sufficient to set ppx preprocessors If you want to support normal preprocessors as well, Clflags.preprocessor will need to be set. It's OK not to do this for now however, as supporting ppx is much more important.

Then there will be something more to do for runtime dependencies, I'll describe that later.

As far as I understand, nothing special needs to be done here. If you use Lib.DB.resolve_user_written_deps_for_exes and the pps field, this will be automatically handled for you.

Feel free to ask if there's anything unclear.

@stephanieyou
Copy link
Contributor

Thanks for the pointers, @diml and @rgrinberg! I'm currently testing my addition of the pps field to the toplevel stanza, and my test hangs when I attempt to use the [%sexp_of] ppx extension. My dune file has the toplevel stanza:

(toplevel
  (name tt)
  (libraries foo)
  (preprocess (pps ppx_sexp_conv)))

The ppx clflag "/path/to/ppx.exe --as-ppx" is generated, and I checked that the path to ppx.exe does exist.

If I remove [%sexp_of] from foo.ml, the test runs fine. Do you know what could be the cause of [%sexp_of] hanging/what I can do to debug this problem? Thanks.

@rgrinberg
Copy link
Member Author

Do you have a branch somewhere we can try? That should make it easier for us to help you debug.

If I had to blindly guess, then I would say that the toplevel is stuck in the interactive loop waiting for input. Try running the test manually and see if you get more output this way.

@stephanieyou
Copy link
Contributor

I'm currently working on the toplevel-ppx branch on my fork. (https://github.com/stephanieyou/dune/tree/toplevel-ppx)

Sorry if this is a silly question, but how do I run the test manually? I tried to run dune build -p preprocessors tt.exe (where preprocessors is the name of my test directory), but I get this error:

File "dune", line 10, characters 1-11:
10 | (preprocess (pps ppx_sexp_conv)))
      ^^^^^^^^^^
Error: Unknown field preprocess

I don't get this error when running dune build @toplevel-stanza.

@rgrinberg
Copy link
Member Author

rgrinberg commented Mar 27, 2020 via email

@stephanieyou
Copy link
Contributor

So the toplevel is stuck waiting for input because using [%sexp_of] gives this error:
Error: Uninterpreted extension 'sexp_of'

When I run
$ dune exec --root preprocessors ./tt.exe -- -ppx "/Users/stephanieyou/Desktop/work/dune/test/blackbox-tests/test-cases/toplevel-stanza/preprocessors/_build/default/.ppx/e5c62d576feb9cf4645d7d03ed58d831/ppx.exe --as-ppx" -init preprocessors/init.ml
(just added the ppx flag), the test works fine.

Is there a difference between -ppx flag in the above command, and setting Clflags.all_ppx before calling Topmain.main()? I was under the impression that the -ppx flag sets Clflags.all_ppx in utop/uTop_main.ml.

@rgrinberg
Copy link
Member Author

rgrinberg commented Mar 29, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants