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 playground build infra w/ ReScript 11 #6201

Merged
merged 16 commits into from
May 12, 2023

Conversation

ryyppy
Copy link
Member

@ryyppy ryyppy commented Apr 25, 2023

Fixes #6200

Key changes:

  • Updated the generate_cmij.js script to also build a compiler-builtins/cmij.js file from the ReScript lib/ocaml modules (otherwise the playground can't compile any ReScript programs due to missing core modules like Pervasives)
  • Added @rescript/core as a pre-vendored package
  • Playground bundle and pre-built third party cmijs default to uncurried mode
  • Streamlined development by adding relevant Makefile targets
  • Also updated the CONTRIBUTING file to reflect the latest changes
  • Remove the outdated jsoo_main.ml / jsoo_common infra (and use js_of_ocaml as a library instead)

There's quite some potential making the playground e2e tests more rigorous by re-using existing compiler tests and passing them to the JSOOified compiler APIs as strings.

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks for this! 🎉

CONTRIBUTING.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@cristianoc
Copy link
Collaborator

I get errors with:

make playground-cmijs

<<<<<<
>>>>>> running command: find /Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/node_modules/rescript/lib/ocaml -name "*.cmi" -or -name "*.cmj" | xargs -n1 basename | xargs js_of_ocaml build-fs -o /Users/cristianocalcagno/GitHub/rescript-compiler/playground/packages/compilerCmij.js -I /Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/node_modules/rescript/lib/ocaml
<<<<<<
>>>>>> running command: find /Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/node_modules/@rescript/react/lib/es6 -name '*.js' -exec cp {} /Users/cristianocalcagno/GitHub/rescript-compiler/playground/packages/@rescript/react \;
find: /Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/node_modules/@rescript/react/lib/es6: No such file or directory
node:child_process:960
    throw err;
    ^

Error: Command failed: find /Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/node_modules/@rescript/react/lib/es6 -name '*.js' -exec cp {} /Users/cristianocalcagno/GitHub/rescript-compiler/playground/packages/@rescript/react \;
    at checkExecSyncError (node:child_process:885:11)
    at Object.execSync (node:child_process:957:15)
    at e (/Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/scripts/generate_cmijs.js:45:17)
    at installLib (/Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/scripts/generate_cmijs.js:100:5)
    at Array.forEach (<anonymous>)
    at buildThirdPartyCmijs (/Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/scripts/generate_cmijs.js:73:12)
    at Object.<anonymous> (/Users/cristianocalcagno/GitHub/rescript-compiler/packages/playground-bundling/scripts/generate_cmijs.js:108:1)
    at Module._compile (node:internal/modules/cjs/loader:1205:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1259:10) {
  status: 1,
  signal: null,
  output: [ null, null, null ],
  pid: 67434,
  stdout: null,
  stderr: null
}

@ryyppy
Copy link
Member Author

ryyppy commented Apr 26, 2023

@cristianoc Found the issue. Could you try again and see if it works on your machine?

@cristianoc
Copy link
Collaborator

@cristianoc Found the issue. Could you try again and see if it works on your machine?

Works.

There are a few warnings.

These jump to the eye:

Warning: integer overflow: integer 0x100000000 (4294967296) truncated to 0x0 (0); the generated code might be incorrect.
Warning: integer overflow: integer 0x100000000 (4294967296) truncated to 0x0 (0); the generated code might be incorrect.

I guess they might be jsoo warnings. And probably does not matter.

@cristianoc
Copy link
Collaborator

ocsigen/js_of_ocaml#285

The location information does not exactly help.
I'd say let's ignore it unless something strange is observed in the playground.

@cristianoc
Copy link
Collaborator

To please CI:

npm run format

@cknitt
Copy link
Member

cknitt commented Apr 26, 2023

I guess they might be jsoo warnings. And probably does not matter.

These warnings only occur when building with jsoo, and they have been there "forever" (before moving to the dune-based build setup).

@ryyppy
Copy link
Member Author

ryyppy commented Apr 26, 2023

These warnings only occur when building with jsoo, and they have been there "forever" (before moving to the dune-based build setup).

Yes, in fact I actually find these errors useful to know that jsoo actually did its work.

I will still need to adapt the compiled files to uncurried mode.

@ryyppy ryyppy force-pushed the ryyppy/playground-dune-integration branch from 3792080 to 889fa49 Compare April 28, 2023 21:49
@ryyppy ryyppy force-pushed the ryyppy/playground-dune-integration branch from 337c12d to f262f1f Compare May 3, 2023 20:01
@ryyppy
Copy link
Member Author

ryyppy commented May 3, 2023

Force-pushed to latest master. There's still some issue when trying to compile useState (which causes a Stack Overflow error), so will investigate a little more before marking this as ready.

@ryyppy ryyppy marked this pull request as draft May 3, 2023 20:07
@mununki
Copy link
Member

mununki commented May 8, 2023

Force-pushed to latest master. There's still some issue when trying to compile useState (which causes a Stack Overflow error), so will investigate a little more before marking this as ready.

I stumbled upon this while testing, and it seems to cause a stack overflow error if there is an unused value. If I replace it with this, I don't get the error.

let _ = React.useState(_ => 0)

@ryyppy ryyppy marked this pull request as ready for review May 10, 2023 07:48
@ryyppy ryyppy requested a review from cknitt May 10, 2023 07:48
@ryyppy
Copy link
Member Author

ryyppy commented May 10, 2023

The issue was caused by the overloaded warning printer logic. Fixed in f75fbf0.

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Great work! Thanks a lot! ❤️

@cristianoc
Copy link
Collaborator

Go for it.

@ryyppy ryyppy force-pushed the ryyppy/playground-dune-integration branch from 1cb750c to ed711f8 Compare May 12, 2023 09:14
@ryyppy
Copy link
Member Author

ryyppy commented May 12, 2023

Re-based onto master again and adjusted the code to use the adapted error reporter. Ready to be merged now.

@ryyppy ryyppy merged commit 0aff3a6 into master May 12, 2023
@ryyppy ryyppy deleted the ryyppy/playground-dune-integration branch May 12, 2023 09:42
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.

Fix playground infra for ReScript / dune setup
4 participants