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

[RFC] Allow to pass more information to ppx rewriters #2020

Closed
ghost opened this issue Apr 4, 2019 · 5 comments
Closed

[RFC] Allow to pass more information to ppx rewriters #2020

ghost opened this issue Apr 4, 2019 · 5 comments

Comments

@ghost
Copy link

ghost commented Apr 4, 2019

This proposal describes a way to pass more information from dune to ppx rewriters.

Description

We extend the ppx_rewriter and ppx_deriver library kinds to take a cookies parameter. This parameters describe a list of cookies that dune should pass to the ppx rewriter. The argument of cookies is a list of cookie definition of the form (<name> <string-with-variables>), such as (profile %{profile}).

For instance:

(library
 (name ppx_inline_test)
 (kind (ppx_rewriter (cookies (profile "\"%{profile\""})))))

When invoking the final ppx driver that links all the ppx rewriter, dune will collect all the cookies requested by the various ppx rewriters and pass them via the command line as follow:

$ ppx.exe --cookie profile="release" ...

When two ppx rewriters request the same cookie, the associated value must be equal. Otherwise dune will error out.

To match the current behavior, dune will also add the following cookie to this list: (library-name \"%{library-name}\").

@Khady
Copy link
Contributor

Khady commented Apr 5, 2019

I feel that using the profile to encode information is a bad pattern. We have for example multiple profiles that are equivalent to release. The ppx has no way to guess that those other profiles should be treated like release.

@ghost
Copy link
Author

ghost commented Apr 8, 2019

That makes sense. I suppose we could have a new parameter drop-inline-tests. It would default to (= %{profile} release), but you could manually set it for other profiles.

In that case, we don't even need to implement the full generic proposal right now, we can only pass drop-inline-tests manually as we do for library-name and leave the general solution for later since it is a bit more work.

@rgrinberg
Copy link
Member

I thought about this a bit more, and I'm wondering if this is at all necessary with link time code gen. We can very easily create a little dune library that dune can generate on every build with information such as profile, context etc. That seems like a useful feature for other tools invoked at build time as well.

@ghost
Copy link
Author

ghost commented Mar 25, 2020

That's a good point.

The feature described in this issues has been added to dune BTW, so we can safely close it.

@ghost ghost closed this as completed Mar 25, 2020
@rgrinberg
Copy link
Member

In retrospect, using link time code gen is a bad idea here. We don't want to rebuild the ppx binary just because context/profile changed. I'd much prefer if there was a simple way to pass these values at runtime.

This issue was closed.
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

No branches or pull requests

2 participants