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

Fix for recent version of OCaml, js_of_ocaml, lwt, core_kernel #8362

Closed
wants to merge 1 commit into from

Conversation

hhugo
Copy link
Contributor

@hhugo hhugo commented May 3, 2020

Make the code compile with recent versions of core_kernel (v0.13.1), lwt (5.3.0), js_of_ocaml (3.6), ocaml (4.10)

Makefile Outdated
@@ -224,9 +224,18 @@ COPIED_FLOWLIB=\
COPIED_PRELUDE=\
$(foreach lib,$(wildcard prelude/*.js),_build/$(lib))

# $ ocamlfind query sedlex lwt lwt_log lwt.unix lwt_ppx unix str bigarray ppx_let core_kernel base wtf8 dtoa -predicates javascript -o-format -r
# Manualy fix path for +base/base_internalhash_types/runtime.js and +ppx_expect/collector/runtime.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mroch
Copy link
Contributor

mroch commented May 12, 2020

thanks for this! we have a lot of different environments to get this to build in so it might take a bit of time to merge this, or I might have to land it in a couple pieces.

@hhugo
Copy link
Contributor Author

hhugo commented May 12, 2020

Not pressing for me. Im doing this as I might use the Js parser in js_of_ocaml at some point in the future.

Note that I would expect the changes I made to be backward compatible with old version of ocaml, core_kernel.

facebook-github-bot pushed a commit that referenced this pull request May 15, 2020
Summary:
adding `true: thread` caused `Thread` to be compiled into the JS build, and that executes stuff at init time that isn't supported in JS:

https://github.com/ocaml/ocaml/blob/12ef11225a5242f41bac8b47e5e1f6b578fd6f33/otherlibs/systhreads/thread.ml#L62

instead, we pass `-tag thread` to `ocamlbuild` everywhere except the JS build

thanks to hhugo for the fix in #8362, landing just that piece of it separately

Reviewed By: samwgoldman

Differential Revision: D21600577

fbshipit-source-id: 8df638b854aa644b700eb26622f0aa0c8262200a
facebook-github-bot pushed a commit that referenced this pull request May 18, 2020
Summary: splits off some of hhugo's changes from #8362 plus some additional changes to build without warnings.

Reviewed By: dsainati1

Differential Revision: D21620532

fbshipit-source-id: c99a677ec4f693cfb04a6e5e58894d873ec2c4df
facebook-github-bot pushed a commit that referenced this pull request Oct 28, 2020
Summary:
2 fixes here:

- splits `FINDLIB_OPTS` into `NATIVE_FINDLIB_OPTS` and `JS_FINDLIB_OPTS`, so that we can pass `JS_FINDLIB_OPTS` to the jsoo build. the difference is that the JS one does not include OS-related packages like `unix` that the .js version doesn't need nor support.
- uses `ocamlfind` to locate all of the JS stubs for the packages we're using, so that we just have to update one list (`FINDLIB_PACKAGES`)

Special thanks to hhugo for the related fix in #8362 (we're landing these incrementally to get our internal builds updated)

Reviewed By: vsiles

Differential Revision: D24505008

fbshipit-source-id: 9fadb3b0819ab369728e9802e1354a512152a1c9
@hhugo
Copy link
Contributor Author

hhugo commented Nov 20, 2020

How much of that PR has been integrated already ? Should I leave it open ? Or close it ?

@mroch
Copy link
Contributor

mroch commented Mar 30, 2021

thanks again for this and sorry for the horrible delay. internally, we were stuck on ocaml 4.07, which meant we couldn't upgrade base or js_of_ocaml to versions that supported 4.10. that's why this is taking so long, and why it's happening in slightly different ways (like js_of_ocaml 3.5.2). I just bumped again to js_of_ocaml 3.7.1.

I've been using this PR as a reference, so I appreciate that you left it open and rebased it. I'll take care of rebasing again and merging what's left... hopefully without any more internal blockers.

@facebook-github-bot
Copy link
Contributor

@mroch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mroch merged this pull request in 3bb8ad7.

@hhugo hhugo deleted the upgrade-410 branch April 6, 2021 21:55
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.

3 participants