Dunify #598

chrismamo1 · 2019-04-12T09:26:04Z

Eliom now compiles in ~17s on my machine, whereas the master branch takes up to 100s.

This also factors the eliom PPX into a separate package, although I suspect it would be possible to avoid doing that.

It also completely removes the syntax package, which I believe is necessary as dune actively discourages camlp4 use. This will break ocsigen-start, since that still relies on Macaque's camlp4 extension, so it cannot use PPX.

Drup

I see you managed to go forward quite a bit, impressive!

First off, You have lot's of code changes that are due to two things: warnings fixes due to dune's settings, and OMPification of the ppxs. I suggest you pull those out and submit them in separate PRs, so that we can merge them (they are much simpler in nature) and clean up the current PR.

Now, about the core of the PR: Your solution is to segregate shared, client, server and eliom files each in their own folder/library, and enforce some dependency order between them. The goal of eliom is precisely not to organize libraries like that, but to organize them by topic, regardless of where the modules are located. If we want to truly be able to build eliom libraries correctly, we need to have libraries where all kind of files live, and have proper handling of dependencies. Of course, this doesn't prevent to merge an initial dunification which we can refine later (when dune has proper support, for instance).

More concretely:

I do not see calls to the type ppx, how does that work ?
you renamed *.eliom[i] files in eliom/*.ml[i]. This doesn't seem necessary.
You currently suppose that we have two building scheme, one for client and one for server. This is not the case with my new implementation. It would be nicer to only suppose you have one build scheme that output 2 things (and which happens to call 2 ppxs currently). This would provide a better way forward.

Drup · 2019-04-12T09:45:57Z

src/lib/client/eliom_bus.ml

@@ -46,6 +46,7 @@ let consume (t,u) s =
        | Lwt.Sleep -> Lwt.wakeup_exn u e;
        | _ -> ());
      [%lwt raise ( e)]
+      [@ocaml.warning "-22"]


I'm pretty sure you can just turn those into Lwt.raise, and get rid of the extension node.

This change was made to satisfy Dune's default settings, which are very strict on compiler warnings. As per your suggestion, I've pulled it out of the PR.

Drup · 2019-04-12T09:46:56Z

src/ppx/.merlin

+S /home/chrismamo1/.opam/default/lib/result
+S .
+FLG -ppx '/home/chrismamo1/Documents/work/BeSport/pgocaml-ppx-porting/eliom/_build/default/.ppx/1f6b9d3f3f2366a895b212642c81f26e/ppx.exe --as-ppx --cookie '\''library-name="ppx"'\'''
+FLG -w @a-4-29-40-41-42-44-45-48-58-59-60-40 -strict-sequence -strict-formats -short-paths -keep-locs


You should just drop the merlin file, since they are dune-generated.

Drup · 2019-04-12T09:47:48Z

src/ppx/ppx_eliom_utils.ml

@@ -315,7 +312,6 @@ module type Pass = sig
  val client_sig: signature_item -> signature_item list
  val server_sig: signature_item -> signature_item list

-  (** How to handle "[%client ...]" and "[%shared ...]" expr. *)


Why are you removing comments?!

Drup · 2019-04-12T09:47:57Z

src/ppx/ppx_eliom_utils.ml

-       ... x ...
-     ]
-   ]
-*)


Drup · 2019-04-12T09:49:23Z

src/ppx/ppx_eliom_utils.ml

+      | "", false -> ()
+      | _, true -> Location.raise_errorf ~loc:Location.(in_file !input_name)
+            "Wrong arguments:@ %s" (String.concat " " (Array.to_list Sys.argv))
+      | x, _ -> Mli.type_file := Some x


Since you are already using Arg, you should use the ~args argument of Migrate_parsetree_driver.register.

Drup · 2019-04-12T10:56:17Z

src/lib/eliom/eliom_service_base.ml

    k
  | _ ->
-    failwith "non_attached_info"
+    .


Beware, this restrict to fairly recent OCaml versions.

This change was made to satisfy Dune's default settings, which are very strict on compiler warnings. As per your suggestion, I've pulled it out of the PR.

move *.client.ml* and *.server.ml* lib files into their respective directories move *.shared.ml* files into shared directory and add symlinks refactor the ppx to support standalone and factor .eliom files back out actually compile things fix building multiple tools delete ppx use external ppx tools fix a dependency issue and update some possibly-obsolete package names create eliomopt alias

chrismamo1 · 2019-04-16T13:19:04Z

@Drup thank you very much for the input.

I've pulled all the compiler warning fixes out, and will submit a new PR once this one has been dealt with.

For simplicity reasons, I think it would be best to pull the PPX and Camlp4 rewriters into separate opam packages, with eliom itself depending on the PPX package.

As for your specific concerns:

I do not see calls to the type ppx, how does that work ?

I'm also unsure about this. I'm able to build ocsigen-toolkit and ocsigen-start with the library as-is, which I honestly didn't expect to work precisely because I've done nothing to handle the generation of .type_mli files within eliom, and I'm a bit unsure of what I could concretely do with those.
you renamed .eliom[i] files in eliom/.ml[i]. This doesn't seem necessary.

I've undone this.
You currently suppose that we have two building scheme, one for client and one for server. This is not the case with my new implementation. It would be nicer to only suppose you have one build scheme that output 2 things (and which happens to call 2 ppxs currently). This would provide a better way forward.

I'm not sure what you're talking about when you mention your new implementation. Are you referring to DO NOT MERGE: Compiling Eliom with the new compiler #459 ?

Drup · 2019-04-16T13:31:26Z

For simplicity reasons, I think it would be best to pull the PPX and Camlp4 rewriters into separate opam packages, with eliom itself depending on the PPX package.

Agreed, but keep everything in this repository for now. Dune works particularly well for this, and opam support it just fine. And please don't push things to the opam repository before it's discussed and we have reached a stable solution. In particular it's completely unacceptable to have some eliom packages pointing to github repositories that are not under the ocsigen umbrella.

I'm also unsure about this. I'm able to build ocsigen-toolkit and ocsigen-start with the library as-is, which I honestly didn't expect to work precisely because I've done nothing to handle the generation of .type_mli files within eliom, and I'm a bit unsure of what I could concretely do with those.

If the typing files are not available, the typing is basically ignored. This is wrong as it means injections and fragments are unchecked and makes eliom's typing completely useless. Arguably, it should lead to an error, but unfortunately it currently doesn't. The fact that eliom code compiles is not sufficient to assert that eliom works. You also need wrong code to fail. :)

I'm not sure what you're talking about when you mention your new implementation. Are you referring to #459 ?

Yes.

rgrinberg · 2020-08-13T18:51:21Z

@chrismamo1 are you still working on this?

chrismamo1 · 2020-08-14T15:01:57Z

@chrismamo1 are you still working on this?

Not actively, no.

rgrinberg · 2020-08-15T07:06:31Z

I see. I was wondering if you were interested in upstreaming eliom support in dune itself. It would be nice if people didn't have to manually write eliom/eliomi rules :)

paurkedal · 2020-08-16T10:00:27Z

I think dune support would be a tremendous help. Judging from my own experience, using manual rules is infeasible. Just getting it to compile is straight forward, but getting the dependencies right for server-client type-checking is tricky. That also means adding dune support may be less straight forward than one would think. I hope that doesn't sound too scary. Someone correct my if I'm wrong, but the rough approach to compiling an Eliom applications is:

Build dependencies separately for server and client side, these need to be included in the ruleset somehow.
Build the server side as a library using eliom.ppx.server, both native and bytecode are supported by the OCSIGEN server.
Build once again in the server side environment using eliom.ppx.type and -infer the result to produce mli files for parts shared with the client.
Build the client side bytecode using the eliom.ppx.client, which for each compilation unit will use the corresponding inferred mli file from the server side for the additional type-checking.
The bytecode can then be compiled to JavaScript with the built-in js_of_ocaml rule.

We should take advantage of the dependencies in the first step to parallelize across the succeeding steps. Note that a client side compilation unit will depend on the corresponding server side inferred type in addition to the regular dependencies, and the inferred type may have it's own dependencies on the server side.

One thing we need to consider is that client-side must have a different build directory from the server side since the module names of the compilation units overlap.

rgrinberg · 2020-08-17T07:32:44Z

@paurkedal Indeed that sounds involved. It seems like you know quite a bit of what's required. If you're interested, I can help you understand the require parts of dune needed to implement this feature.

One thing we need to consider is that client-side must have a different build directory from the server side since the module names of the compilation units overlap.

That would be a good first step to look into in dune.

paurkedal · 2020-08-20T19:42:25Z

@rgrinberg I would like to take you up on that offer in a couple of months when I have more time to spare, but just to get the thoughts going:

We could try to generalize dune to support eliom-style flows or we could make a specialised rule. I think the latter is most realistic given how I understand the dune design as an end-user. Keeping all sources in a single directory for server and client is encouraged, using .client.ml, .server.ml, and .shared.ml for non-eliom files, so the application is best declared in a single dune file. Since it makes no sense to only build one side of an eliom application and since the parts have non-trivial inter-dependencies, I think the best option is to also use a single stanza. Not to get into details about syntax, a flattered stanza might that look like

(eliom_application
 (name ...) (public_name ...)
 (eliom_modules ...) (shared_modules ...) (client_modules ...) (server_modules ...)
 (shared_libraries ...) (client_libraries ...) (server_libraries ...)
 (shared_flags ...) (client_flags ...) (server_flags ...)
 (shared_preprocess ...) (client_preprocess ...) (server_proprocess ...)
 ...)

but we could also make server library and client executable nested with shared properties factored up.

Next, it would also be good to support building Eliom libraries, which would consist of two libraries, a server and client, where the latter is bytecode only. Otherwise the same logic regarding inferred mli files, PPXes, and dependencies applies.

rgrinberg · 2020-08-20T20:43:44Z

What about eliom libraries? Isn't it possible to define a library with client & server side components?

paurkedal · 2020-08-20T20:54:51Z

Yes, I think an (eliom_library ...) stanza would be quite similar. The public names would by convention be foo.server and foo.client. (Also for the application I assumed above that the library and executable names would be inferred from the declared names.)

rgrinberg · 2020-08-20T20:57:03Z

I see. I wonder if it's simpler to just an eliom field to existing stanzas. I'd prefer to introduce new stanza only if some existing fields of library or executable don't apply to eliom.

paurkedal · 2020-08-20T20:59:47Z

But note that there is a tight relation between the client and server side, both since declarations of modules and library dependencies will overlap (i.e. the (shared_* ...) and (eliom_* ...) will be the common case) and due to tight dependencies from servers side to client side.

clembu · 2020-08-21T14:11:59Z

I imagine support for an (eliom (server_libraries ...) (client_libraries ...) (server_flags ...) .....) stanza for both executables and library stanzas would be better than brand new top-level stanzas.

Drup reviewed Apr 12, 2019

View reviewed changes

John Christopher McAlpine added 7 commits April 15, 2019 11:33

change camlp4 references to the new eliom_camlp4

7fb0bb3

fix ppx_eliom issue

882921d

update opam file

1be1b3b

add eliomcp installation stanza

8a32e5e

add eliom-distillery executable installation

4376b77

purge unnecessary dune libs and unnecessary warning corrections

2601a53

chrismamo1 force-pushed the dunify-new-new branch from 48d2d23 to 2601a53 Compare April 16, 2019 12:37

John Christopher McAlpine added 3 commits April 16, 2019 14:50

purge unnecessary dune libs and unnecessary warning corrections

f6aef02

purge unnecessary dune libs and unnecessary warning corrections

0df6a81

purge pkg folder

b719739

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dunify #598

Dunify #598

chrismamo1 commented Apr 12, 2019 •

edited

Loading

Drup left a comment •

edited

Loading

Drup Apr 12, 2019

chrismamo1 Apr 16, 2019

Drup Apr 12, 2019

Drup Apr 12, 2019

Drup Apr 12, 2019

Drup Apr 12, 2019

Drup Apr 12, 2019

chrismamo1 Apr 16, 2019

chrismamo1 commented Apr 16, 2019

Drup commented Apr 16, 2019

rgrinberg commented Aug 13, 2020

chrismamo1 commented Aug 14, 2020

rgrinberg commented Aug 15, 2020

paurkedal commented Aug 16, 2020

rgrinberg commented Aug 17, 2020

paurkedal commented Aug 20, 2020

rgrinberg commented Aug 20, 2020

paurkedal commented Aug 20, 2020

rgrinberg commented Aug 20, 2020

paurkedal commented Aug 20, 2020 •

edited

Loading

clembu commented Aug 21, 2020

Dunify #598

Are you sure you want to change the base?

Dunify #598

Conversation

chrismamo1 commented Apr 12, 2019 • edited Loading

Drup left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismamo1 commented Apr 16, 2019

Drup commented Apr 16, 2019

rgrinberg commented Aug 13, 2020

chrismamo1 commented Aug 14, 2020

rgrinberg commented Aug 15, 2020

paurkedal commented Aug 16, 2020

rgrinberg commented Aug 17, 2020

paurkedal commented Aug 20, 2020

rgrinberg commented Aug 20, 2020

paurkedal commented Aug 20, 2020

rgrinberg commented Aug 20, 2020

paurkedal commented Aug 20, 2020 • edited Loading

clembu commented Aug 21, 2020

chrismamo1 commented Apr 12, 2019 •

edited

Loading

Drup left a comment •

edited

Loading

paurkedal commented Aug 20, 2020 •

edited

Loading