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

Improved support for js_of_ocaml + OCaml 4.03+ #1372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improved support for js_of_ocaml + OCaml 4.03+ #1372

wants to merge 1 commit into from

Conversation

SanderSpies
Copy link
Contributor

@SanderSpies SanderSpies commented Aug 25, 2017

I did this so Reason can build be with JSOO on OCaml 4.04.2.

  • Remove dependency on utop as this should be optional
  • Rename vendored easy_format.ml to easy_format_vendored.ml to avoid warning 31, which became the default from OCaml 4.03+ but can't be disabled with JSOO.
  • Fix reason_without_utop.mllib which included duplicate stuff from reasonparser.mllib

@chenglou
Copy link
Member

Cool. So what happens if folks install Reason and expect rtop? And what about reason-cli

The bspack thing is likely broken. I can fix it later.

@jordwalke
Copy link
Member

jordwalke commented Aug 28, 2017

Great! As far as optional dependency on utop, I believe that opam is deprecating that feature (optional dependencies).

The test is failing because it can't find utop. I think the right step is to completely factor out rtop into its own opam package (it wouldn't need to be updated frequently IIRC).

(reason-cli can still aggregate all the tools we want but there's no reason why the core Reason tool for syntax should include a command line.)

@bsansouci
Copy link
Contributor

I want this PR because the profilers I'd like to use require 4.03. (https://github.com/LexiFi/landmarks)

@chenglou chenglou mentioned this pull request Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants