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 support #16

Closed
wants to merge 2 commits into from
Closed

jbuilder support #16

wants to merge 2 commits into from

Conversation

vogler
Copy link

@vogler vogler commented Jul 24, 2017

I wanted to use ppx_import with jbuilder.
I tried something like this PR which makes ppx_deriving work with jbuilder. However, I have no idea about packaging and what I did doesn't seem to suffice.

vogler added 2 commits July 24, 2017 17:02
Error: Uninterpreted extension 'import'.
~~~
(cd _build/default && ./.ppx/ppx_import+ppx_deriving/ppx.exe --dump-ast -o test.pp.ml --impl test.ml)
Sys_error("--impl: No such file or directory")
~~~
@vogler
Copy link
Author

vogler commented Jul 25, 2017

@diml @let-def Maybe you could help? I just mixed things from your ppx_deriving PR and the omp examples.

@ghost
Copy link

ghost commented Jul 25, 2017

Someone else asked about this (ocaml/dune#193). It's currently not possible to use ppx_import with jbuilder.

The reason for that is that jbuilder splits the pre-processing from the rest of the compilation for performance reason. As a result ppx rewriters might be run before the cmi files are generated.

It would be possible to change that, but it's an invasive change in jbuilder and one would have to be careful not to slow-down the build.

What could be done relatively easily, that would work well with jbuilder is a version of ppx_import that would work as a standalone code generator rather than a ppx rewriter. With such a tool you could import type definitions coming from other libraries with jbuilder.

@vogler
Copy link
Author

vogler commented Jul 26, 2017

a version of ppx_import that would work as a standalone code generator rather than a ppx rewriter

Using this then instead? I'll give it a try. Thanks for your help!

@vogler vogler closed this Jul 26, 2017
@ghost
Copy link

ghost commented Jul 26, 2017

I was more thinking of a plain rule.

But actually the current ppx_import should already work with jbuilder to import types from other libraries, by declaring the dependencies on the cmi files the ppx will read. Essentially you'd write:

(library
 (...
  (preprocessor_deps (${lib:foo:bar.cmi}))
  ...))

${lib:...} is not accepted at present in (preprocessor_deps ...) but it should be eventually

@whitequark
Copy link
Contributor

whitequark commented Aug 8, 2017

@diml Can you elaborate on what's needed from me in this case? And what's from jbuilder? Just for triaging.

@ghost
Copy link

ghost commented Aug 8, 2017

I suppose the best would be a way to pass individual cmi on the command line rather than include paths, so that jbuilder knows what files the rewriter will read.

@ghost
Copy link

ghost commented Aug 8, 2017

But maybe it's not worth changing anything right now. I need to change something in jbuilder which might make it easier to support ppx rewriters that use the typer. Maybe all we'll need is a variable in the META of ppx_import to say that it uses the typer.

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.

2 participants