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

Allow using PPXes as libraries #171

Merged
merged 5 commits into from
Apr 11, 2021
Merged

Allow using PPXes as libraries #171

merged 5 commits into from
Apr 11, 2021

Conversation

anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Apr 11, 2021

fixes #121

  • generates a ppxlib driver on the fly

TODO:

  • allow passing arguments to the ppx

EduardoRFS and others added 3 commits April 10, 2021 00:05
- move ppx_config to Bsb_global_ninja_vars.global_config
- move root `dune.bsb` file generation from `bsb_main.ml` to
`bsb_ninja_gen.ml`
- build the PPX with `Ppxlib.Driver.standalone` and pass the `--as-ppx`
flag
- Generate the `ppx.ml` file in the `.melange.eobjs` directory instead
of at the toplevel of a project
Copy link
Collaborator

@EduardoRFS EduardoRFS left a comment

Choose a reason for hiding this comment

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

Two small nits

jscomp/bsb/bsb_merlin_gen.ml Outdated Show resolved Hide resolved
jscomp/bsb/bsb_ninja_gen.ml Outdated Show resolved Hide resolved
@anmonteiro anmonteiro merged commit 1921408 into fork Apr 11, 2021
@anmonteiro anmonteiro deleted the ppxlib branch April 11, 2021 21:52
@anmonteiro anmonteiro mentioned this pull request Apr 12, 2021
4 tasks
@TheSpyder
Copy link
Contributor

How do I make use of this? It's not really clear from looking at the changes.

@anmonteiro
Copy link
Member Author

@TheSpyder this will definitely need documented, and we've added it to the list in #105.

A few informal notes:

  1. depend on the PPXes you want via esy (or OPAM or Nix, anything that knows how to install PPXes with the ocamlfind toolchain)
  2. add them to ppx-flags as usual, with just the ppx name (this is the important part -- it should match whatever the findlib name is. Generally it's also the package name)
  3. Profit!

@TheSpyder
Copy link
Contributor

That's the part I'm not clear on, the findlib name. I'm trying to make a branch of my old favourite decco that can integrate with this feature, to measure the speed difference on my fairly large project.

@anmonteiro
Copy link
Member Author

you can run esy ls-libs I believe, which will tell you the name of the library.

@anmonteiro
Copy link
Member Author

You need to depend on decco from source, btw, in your case via esy. I don't know how easy this is, but we could make a PR.

@TheSpyder
Copy link
Contributor

yes, I tried that and it still tried to execute it as a binary instead of loading it as a library. I just need to fiddle a bit more (I'm currently trying to lift the esy project up to the respository root instead of having it in a subfolder, to see if that helps).

@TheSpyder
Copy link
Contributor

I can't get it working, here's the branch I tried, but I also realised that this "PPX as library" feature really only helps if there are multiple PPX. It still builds the ppx as a separate executable, so for single ppx use it won't be any faster than the precompiled binary (just easier to install since precompiled executables are not required).

This feature will likely help me in the future, but it's no longer important for me to try and get it working today :)

@EduardoRFS
Copy link
Collaborator

@TheSpyder yeah that's true for us, but there is always multiple ppx pass, because we do a ppx pass ourselves.

Ideally we should move Melange PPX to ppxlib

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.

RFC: support to ppxlib directly on bsconfig
3 participants