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

Using PPX #8586

Open
rgrinberg opened this issue Sep 4, 2023 · 9 comments · May be fixed by #9223
Open

Using PPX #8586

rgrinberg opened this issue Sep 4, 2023 · 9 comments · May be fixed by #9223
Assignees
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected

Comments

@rgrinberg
Copy link
Member

Background

Dune contains a large amount of manually written code mundane tasks. In particular, we currently write the following by hand:

  • Comparison functions.
  • Hashing functions
  • Digest version generators
  • Dyn converters

This is all rather time consuming and error prone. The natural solution is to use ppx to get rid of all this.

Using PPX

Unfortunately, using ppx comes with its own set of problems. In particular, it requires us to commit the generated code so that bootstrapping without ppx is still available. I'm going to describe how we can make this process a little more painless

The obvious thing to do would be to use to deriving_inline like base. Unfortunately, this scheme requires a bunch of tooling to be usable. For example, we need an editor extension to hide generated code. Something similar for hiding the generated code in diffs is required as well.

The following workflow allows us to avoid working with generated code without these downsides:

  • The generated code for $source is going to live in ppx/$source.
  • We're going to tell git to ignore the ppx directory for diffing purposes.
  • We're going to tell grep (rg in practice) to ignore this directory as well.
  • In development, we will require installing the ppx stack. In particular, every commit will require a promote step to update the generated sources.
  • The bootstrapping process will be update to compile $source as follows. First, check for the existence of $source in ppx/$source. If it exists, then use it. Otherwise use $source.

Potential challenges

  • To maintain our use of dyn, we will need to write a ppx for it
  • We'll need to tweak the ppx rules to produce corrected files in our ppx directory
  • We will need to vendor a few ppx runtime libraries. These are likely going to include base. Therefore we need to evaluate if base is going to be usable for us.

cc @snowleopard

@rgrinberg rgrinberg added the proposal RFC's that are awaiting discussion to be accepted or rejected label Sep 4, 2023
@NathanReb
Copy link
Collaborator

I'd be up to give this a spin!

After a quick offline discussion with @rgrinberg we agreed a good place to start would be to allow use of ppx inside dune via the separate ppx/ directory for preprocessed sources and corresponding bootstrapping and promotion rules
and to inaugurate it with a deriver for dyn converters.
That would already benefit dune without having to decide whether we want to vendor base.

@hhugo
Copy link
Collaborator

hhugo commented Sep 5, 2023

I'm surprised you're considering depending on base given the recent announcement https://discuss.ocaml.org/t/ann-v0-16-release-of-jane-street-packages/12398 about reducing support for older ocaml versions.

@rgrinberg
Copy link
Member Author

I'm surprised you're considering depending on base given the recent announcement https://discuss.ocaml.org/t/ann-v0-16-release-of-jane-street-packages/12398 about reducing support for older ocaml versions.

Yes, using base "as-is" is a non starter. The idea was to strip it to a 4.08+ compatible subset that is enough to run bin_prot, compare, hash or to port the mentioned ppx's to work without base.

@NathanReb
Copy link
Collaborator

I started looking into this by getting familiar with the bootstrapping of dune and managed to modify duneboot.ml so it picks sources from ppx/ instead of the originals if such files exists.

I then proceeded to write a dummy ppx_deriving_to_dyn to try and setup the rest of the chain, i.e. build dune in dev mode using the regular ppx toolchain (so far so good, especially since ppxlib already is a test deps), actually generate the content of ppx/ and keep it in sync with the sources.

I tried a first prototype using simple rules in dune files but quickly realized that it can't easily be done this way. We'd have to write one for each preprocessed file and we don't easily have access to the ppx driver that dune as already built to preprocess the files as part of a regular dev build. We also can't easily have them generated in a ppx/ folder at the root —or in any kind of subdir for that matters— using rules.

Ideally we'd want to handle this from within dune itself so we can easily access dune files and the relevant preprocess fields from libraries and executable stanzas.

I was thinking about adding an alias for this so that running dune build @saidalias would generate the preprocessed source and diff them with the content of the ppx/ folder so we can promote the changes. We could imagine adding a dependency from @check or @runtest to this alias at the root of dune's codebase. You mentioned enforcing this per-commit. I definitely agree but I'm unsure how to proceed with this, I assume this would rather be handled on the CI side but having such an alias should make it easy as we'd just have to run the above command in the CI for each individual commit.

I'm not sure if we could easily disable this alias outside of dune codebase so that it doesn't leak unintentionally. Maybe simply not documenting it, in association with the lack of support for building using preprocessed sources outside of dune is enough of a dissuasion to use this in the wild to build projects.
That being said, I could imagine the alias being used as a quick and convenient way to have a look at what one's ppx are generating.

If making this alias strictly private is too much of a hassle, I imagined we could go with a seperate private executable that would do the exact same steps i.e. read dune files, generate preprocessed source files, diff with ppx/ content.

Let me know what you think and if you have any better suggestions on how to tackle this specific bit of the work.

@Alizter
Copy link
Collaborator

Alizter commented Sep 8, 2023

Such a ppx preview could also be related to #6897. However we probably don't want to implement anything user facing just yet and get something working internally first. I don't think making it private will be a hassle. We already have INSIDE_DUNE which might be able to be hooked up.

@rgrinberg
Copy link
Member Author

We also can't easily have them generated in a ppx/ folder at the root —or in any kind of subdir for that matters— using rules.

Isn't it just a matter of creating a dune file in ppx/$lib-path/dune and putting the various rules there? Although I agree that it would be incredibly tedious.

I was thinking about adding an alias for this so that running dune build @saidalias would generate the preprocessed source and diff them with the content of the ppx/ folder so we can promote the changes

Do we really need the intermediate diff step? The code is going to be hardly readable and generated. We could just set (mode promote) on the rules and skip the explicit promotion step.

As for choosing what alias we're going to use to run this promotion step, I'm not sure that it matters. After all, I imagine that we'll need some sort of a project option to enable this feature and the alias would otherwise be disabled. The entire discussion could be side stepped though. I imagine all generated sources could be built with $ dune build ppx/ which will just build the @all alias in the ppx/ directory which should include all the promoted rules automatically.

I imagined we could go with a seperate private executable that would do the exact same steps i.e. read dune files, generate preprocessed source files, diff with ppx/ content.

I might be missing something. Why do we need a private executable?

@NathanReb
Copy link
Collaborator

NathanReb commented Sep 12, 2023

I might be missing something. Why do we need a private executable?

We don't, I was just considering this as an alternative if we didn't want to include that feature directly into dune but it seems we're fine with that.

Do we really need the intermediate diff step? The code is going to be hardly readable and generated. We could just set (mode promote) on the rules and skip the explicit promotion step.

It's a good point, I guess we don't want the intermediate diff step indeed. As long as the CI properly checks everything is kept in sync we should be good.

I imagine all generated sources could be built with $ dune build ppx/ which will just build the @all alias in the ppx/ directory which should include all the promoted rules automatically.

Do you see this as being hardcoded in dune somehow or rather enabled via some kind of (include_preprocessed_sources <dir>) stanza written somewhere?
You also mention promoted rules and I'm not quite sure what you mean here, could you expand on that?

@rgrinberg
Copy link
Member Author

Do you see this as being hardcoded in dune somehow or rather enabled via some kind of (include_preprocessed_sources

) stanza written somewhere?

I imagine this would work automatically if the rules are setup. The way the all alias is defined in a directory is by building Predicate_lang.true_ in that directory. If you setup the rules, the glob would pick up the targets.

You also mention promoted rules and I'm not quite sure what you mean here, could you expand on that?

Sorry, that's just sloppy wording on my part. Rules are never promoted, but can be defined with a mode. So here I just meant the rules with (mode promote).

@rgrinberg
Copy link
Member Author

rgrinberg commented Sep 12, 2023

We don't, I was just considering this as an alternative if we didn't want to include that feature directly into dune but it seems we're fine with that.

I forgot to mention that indeed we're fine with it. Mostly because it's going to be the simplest thing to do and because we've already done it before with other dune only features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants