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

Split js_of_ocaml (WIP) #541

Closed
wants to merge 15 commits into from
Closed

Split js_of_ocaml (WIP) #541

wants to merge 15 commits into from

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Nov 9, 2016

This this a work in progress (feedback welcome)

  • js_of_ocaml-compiler: compiler & runtime (@alainfrisch)
  • js_of_ocaml-camlp4: camlp4 syntaxes (pa_js, pa_deriving)
  • js_of_ocaml-ocamlbuild: ocamlbuild plugin (@dbuenzli)
  • js_of_ocaml-toplevel: jsoo_mkcmis, jsoo_mktop, js_of_ocaml.toplevel

TODO split lib

fix #265
fix #360

@alainfrisch
Copy link
Contributor

alainfrisch commented Nov 9, 2016

This is awesome, and very appreciated!

I was just playing this night to see what it would take to build the compiler with as little external dependencies as possible.

Another easy way to reduce dependencies is to make Base64+Yojson optional. They are only used to support source maps, and this could be packaged in a specific module (that only use Base64+Yojson in its implementation, not its interface, with a stubbed out implementation when Base64+Yojson are not available).

About cppo and the dependency on OCaml:

  • Util.obj_of_const is Symtable.transl_const, but it is unfortunately not exported. I wonder if there is a way to use it through the interface of Symtable (with patch_object and Reloc_literal; and then initial_global_table). But in the long run, it would be best for compiler-libs to expose Symtable.transl_const and have js_of_ocaml use it. This would not solve compatibility for older versions of OCaml (but they could be supported with hard-coded versions of obj_of_const per supported old version of OCaml).
  • capitalize_ascii/uncapitalize_ascii: it would be straightforward to have a local implementation (currently, the semantics would depend on the version of OCaml, I haven't checked whether this is bad or not).
  • find_loc_in_summary (btw, its name argument is unused!): couldn't one use Env_aux to rebuild a real Env.t and use normal lookup functions. This would also be faster in case of repeated lookup (I don't know if this matters).

A temporary solution would of course be to commit a processed version of util.ml for each support version of OCaml (as for files generated by menhir). (And perhaps to split Util into two modules in order to have the version-dependent part smaller.)

@alainfrisch
Copy link
Contributor

alainfrisch commented Nov 9, 2016

Btw, why does js_of_ocaml need to map Lambda's constants to Obj.t? Does it map them finally to its own structured representation (through Parse_bytecode.Constants.parse)?

[EDIT] Ah, the constants can also come from the DATA section of a bytecode executable (as Obj.t values).

@hhugo hhugo added this to the 3.0 milestone Nov 9, 2016
@bobzhang
Copy link
Contributor

bobzhang commented Nov 9, 2016

In case you are interested, I wrote another cppo in a single file (0 deps and no dependencies on compiler-libs and should work against reasonable modern ocaml compilers)
documented here: http://bloomberg.github.io/bucklescript/Manual.html#_conditional_compilation_support_static_if

@hhugo
Copy link
Member Author

hhugo commented Nov 9, 2016

@bobzhang, I don't really see the point of switching from cppo to yet another preprocessor and I don't see how no dependencies on compiler-libs matters at all.

@alainfrisch
Copy link
Contributor

There could be a general discussion about embedding a minimal conditional compilation preprocessor directly in the OCaml compiler (although this would not solve the problem of maintaining compatibility with old versions). @bobzhang Did you mean that your preprocessor is in a shape where it could be upstreamed? @lefessan also proposed to integrate such a preprocessor right in the compiler.

@bobzhang
Copy link
Contributor

bobzhang commented Nov 9, 2016

There are two separate issues.
Yes, eventually we want to upstream our changes to OCaml compiler, it is only 500 Lines of diff to the compiler and super fast.

But I also did some work to extract it into a stand alone single file, so people could grab it and use it on the fly for any ocaml compiler without any dependencies.

As I said, it is an option if you prefer clean deps but I am not trying to convince you to switch..

@hhugo
Copy link
Member Author

hhugo commented Nov 9, 2016

A dependency on cppo vs a dependency on your thing - I don't understand the idea...
Unless you're proposing to include your preprocessor in the jsoo repo - but in that case, how is it different from including cppo sources ?

@bobzhang
Copy link
Contributor

bobzhang commented Nov 9, 2016

My thing is just one file: https://github.com/bloomberg/bucklescript/blob/master/jscomp/bin/bspp.ml
You don't even need build it
I am not sure cppo will have other deps
When we collect enough feedback, maybe we can upstream the patch to the OCaml compiler, I think having a built in conditional compilation is a needy feature

@hhugo
Copy link
Member Author

hhugo commented Nov 10, 2016

The file you're pointing at is 5000 lines, not 500.
Concatenating all cppo files, you get something around 7000 lines [1].

 cat cppo_version.ml cppo_types.ml cppo_parser.mli cppo_parser.ml cppo_lexer.ml cppo_command.ml cppo_eval.ml cppo_main.ml | wc -l
7045

@bobzhang
Copy link
Contributor

It is 500 lines diff and 100% honest to OCaml's lexical convention, and we will maintain both this file and the patch.
As I said, I just made an offer in case you are interested.

@hhugo
Copy link
Member Author

hhugo commented Nov 10, 2016

I understand that you just made an offer, I'm just trying to understand pros/cons.

@hhugo hhugo force-pushed the separate branch 4 times, most recently from 37a39bf to aebe15a Compare November 13, 2016 20:33
@dbuenzli
Copy link
Contributor

@hhugo and @gasche.

Do you know if it's possible/desirable to simply register the plugin as a side effect on -plugin package(js_of_ocaml.ocamlbuild) so that no myocambuild.ml is needed ?

This would actually allow to prevent having to distribute the plugin separately, you simply add the plugin option to the ocamlbuild invocation when js_of_ocaml is installed.

Currently, without the split occuring in this PR, I cannot optionally depend on jsoo because I need to write this in myocamlbuild.ml:

let _ = Ocamlbuild_plugin.dispatch Ocamlbuild_js_of_ocaml.dispatcher

@gasche
Copy link

gasche commented Nov 14, 2016

This is a natural question that people ask, but it was a intended design choice not to allow this when I introduced OCaml plugin. This would only work if the plugin module is linked in by the application. In my experience, expecting compilation units that your code does not visibly depend on to be linked nonetheless is fairly fragile (-linkall has to be used, and sometimes people will forget about it and not link the plugin and not understand the issue). The lesson I took away from linking-based systems (eg. Camlp4 plugins) is that it's sensibly more robust to make sure there is an explicit dependency in the code.

You are the boss of "package configuration". Is there not a way you could make the dependency optional that is not too kludgy and yet generates an explicit dependency when jsoo is present? One thing that could work is to use/generate different myocamlbuild.ml files at configuration time (but how to compose with user code?). Another is to try would be to dynamically link Ocamlbuild_js_of_ocaml from myocamlbuild.ml (depending on a dynamic check of whether the dependency is available), but if I understand correctly dynlink is not flexible enough for this. You could still dynlink a local module that would call Ocamlbuild_js_of_ocaml and then do the dispatch call, but that also sounds kludgy.

In an ideal world, users would be able to just use package(foo_ocamlbuild.plugin) when they want "the obvious thing" to happen, this would be robust, and in the case where they want to take explicit control of the ordering of plugin-provided hooks, they could still do it (instead of being bound to module initialization order). If anyone has a plan to achieve this, I'm all ears. The current design is maximally safe and expressive, but inconvenient by default -- I thought it was a first step on which we would iterate.

@dbuenzli
Copy link
Contributor

One thing that could work is to use/generate different myocamlbuild.ml files at configuration time

I'm in general against generating things (it's basically indirections to work around badly designed systems). Maybe after all having to distribute your ocamlbuild rules separately is the best we can do given the current state of affairs.

@alainfrisch
Copy link
Contributor

Util.obj_of_const is Symtable.transl_const, but it is unfortunately not exported

Now exported (see ocaml/ocaml#904 ). I'd suggest to adapt the code to use this the function when compiling for OCaml >= 4.05.

@hhugo hhugo force-pushed the separate branch 6 times, most recently from 0a83de5 to 5f61aad Compare December 1, 2016 15:38
@rgrinberg
Copy link
Contributor

What's the status of this? Anything we can do to help this out?

@Drup Drup mentioned this pull request Mar 10, 2017
9 tasks
@hhugo
Copy link
Member Author

hhugo commented Mar 29, 2017

#565 was merged

@hhugo hhugo closed this Mar 29, 2017
@hhugo hhugo deleted the separate branch March 29, 2017 01:17
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.

Distribute the ocamlbuild plugin separately split js_of_ocaml in sub libraries
6 participants