Skip to content

Add support for getentropy in d8 #24185

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

Merged
merged 6 commits into from
Apr 25, 2025
Merged

Conversation

hoodmane
Copy link
Collaborator

Use os.system to read from /dev/urandom and then base 64 encode the result. Then use base64Decode to read the result. Only works when --enable-os-system is passed to d8.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm, are you using d8 in production somewhere? i.e. what is the motivation for this change?

@hoodmane
Copy link
Collaborator Author

hoodmane commented Apr 24, 2025

I asked @erikcorry if he could look into ways to improve codegen for Python ceval.c and he said it would be much easier if he could run Pyodide in d8 as opposed to having to build node or such against a branch of v8.

@hoodmane
Copy link
Collaborator Author

But it was surprisingly not too hard to get it to work: https://github.com/pyodide/pyodide/pull/5607/files
This is the only change to Emscripten needed.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 24, 2025

I asked @erikcorry if he could look into ways to improve codegen for Python ceval.c and he said it would be much easier if he could run Pyodide in d8 as opposed to having to build node or such against a branch of v8.

So you are patching / working on v8? Indeed that does sounds like a good use case.

If you just want recent builds of v8 in node you can also use node canary which I think its basically a nightly build: https://nodejs.org/download/v8-canary/

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Looks like you need to add os.system to src/closure-externs/node-externs.js maybe?

@hoodmane
Copy link
Collaborator Author

The same set of tests failed twice in CI but they pass for me locally.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 25, 2025

The closure compilr failure looks real:

building:ERROR: Closure compiler run failed:

building:ERROR: /tmp/emtest_qmeo9dvm/emscripten_temp_3t3ik5j9/hello_world.js:1078:15: ERROR - [JSC_UNDEFINED_VARIABLE] variable os is undeclared
  1078|           if (!os.system) {
                       ^^

1 error(s), 0 warning(s)

emcc: error: closure compiler failed (rc: 1): /root/emsdk/node/20.18.0_64bit/bin/node --max_old_space_size=8192 /root/project/node_modules/.bin/google-closure-compiler --compilation_level ADVANCED_OPTIMIZATIONS --language_in UNSTABLE --language_out NO_TRANSPILE --emit_use_strict=false --externs /root/project/src/closure-externs/closure-externs.js --externs /root/project/src/closure-externs/webgpu-externs.js --externs /root/project/src/closure-externs/v8-externs.js --externs /root/project/src/closure-externs/spidermonkey-externs.js --js /tmp/emtest_qmeo9dvm/emscripten_temp_3t3ik5j9/hello_world.js --js_output_file tmpj2841mhf.cc.js --formatting PRETTY_PRINT
None
None
test_closure_webgpu_wasm64 (test_other.other) ... FAIL

@sbc100 sbc100 enabled auto-merge (squash) April 25, 2025 17:42
@hoodmane
Copy link
Collaborator Author

Hmm:

ERROR - [JSC_VAR_MULTIPLY_DECLARED_ERROR] Variable os declared more than once. First occurrence: /home/rchatham/Documents/programming/emscripten/third_party/closure-compiler/node-externs/os.js:30:4

@sbc100 sbc100 merged commit 5cdadd6 into emscripten-core:main Apr 25, 2025
28 checks passed
@hoodmane hoodmane deleted the shell-getentropy branch April 26, 2025 01:27
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.

2 participants