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

Initial js_of_ocaml support (requires the unreleased jsoo.3.0) #60

Merged
1 commit merged into from
May 2, 2017

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented Apr 24, 2017

Documentation is still missing

@hhugo hhugo changed the title Initial js_of_ocaml support (requires the unreleased jsoo.3.0) Initial js_of_ocaml support (requires the unreleased jsoo.3.0) WIP Apr 24, 2017
src/gen_rules.ml Outdated
@@ -985,6 +985,128 @@ module Gen(P : Params) = struct
| t :: ts -> dot_merlin ~dir (List.fold_left ts ~init:t ~f:merge_two)
end

module Js_of_ocaml_rule = struct

let build_dir = List.fold_left ~init:(Path.relative ctx.build_dir ".js") ~f:Path.relative
Copy link

Choose a reason for hiding this comment

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

Currently the auto-generated ppx rewriters are generated in _build/.ppx/<context>. The idea was to avoid reserving the .ppx name. On second though maybe that's a bit extra, but at least we should be consistent and use _build/.js/<context>.

BTW, shouldn't this be called in_build_dir?

src/gen_rules.ml Outdated
let build_dir = List.fold_left ~init:(Path.relative ctx.build_dir ".js") ~f:Path.relative

let runtime_file ~dir fname =
let _lib, file = Artifacts.file_of_lib ~dir ~use_provides:true (sprintf "js_of_ocaml-compiler:%s" fname) in
Copy link

Choose a reason for hiding this comment

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

This should be use_provides:false. true is only for ${findlib:...} with (jbuild_version jane_street)

src/gen_rules.ml Outdated
Utils.library_not_found ~context:ctx.name ~hint:"opam install js_of_ocaml-compiler" "js_of_ocaml-compiler")
| Ok f -> Arg_spec.Dep f

let binary bin =
Copy link

Choose a reason for hiding this comment

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

The same code is used for "refmt", we should factorize it, maybe as Artifacts.prog_spec

src/gen_rules.ml Outdated
; spec
]

let get_runtimes (libs,_) =
Copy link

Choose a reason for hiding this comment

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

This function should go in lib.ml

src/gen_rules.ml Outdated
@@ -1701,7 +1845,7 @@ module Gen(P : Params) = struct
|> Merlin.gen ~dir:ctx_dir

let () = List.iter P.stanzas ~f:rules

let () = List.iter (Js_of_ocaml_rule.setup_findlib ()) ~f:add_rule
Copy link

Choose a reason for hiding this comment

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

This needs to be done only if we separate compilation is enabled, otherwise we are going to scan the whole findlib database for nothing on every start

@ghost
Copy link

ghost commented Apr 24, 2017

The manual also need to be updated

@ghost
Copy link

ghost commented Apr 25, 2017

Ok, code looks good. BTW, since we don't use the fact that (js_of_ocaml ...) is present or not, we should change the field to not be an option, and use a default value if it's absent

@ghost
Copy link

ghost commented Apr 28, 2017

I started splitting gen_rules.ml to make it easier to add new systems. You should now be able to move the js_of_ocaml rules in their own module. Global variables should go in Super_context

@hhugo hhugo force-pushed the js_of_ocaml-support branch 3 times, most recently from d7df3f1 to ccaeb49 Compare April 30, 2017 11:20
@hhugo hhugo changed the title Initial js_of_ocaml support (requires the unreleased jsoo.3.0) WIP Initial js_of_ocaml support (requires the unreleased jsoo.3.0) Apr 30, 2017
@ghost ghost merged commit 86a4f03 into ocaml:master May 2, 2017
@ghost
Copy link

ghost commented May 2, 2017

Thanks!

@hhugo hhugo deleted the js_of_ocaml-support branch November 5, 2021 22:37
This pull request 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

Successfully merging this pull request may close these issues.

1 participant