Add a default for JS_ENGINES #9542
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kripken
reviewed
Sep 30, 2019
tools/gen_struct_info.py
Outdated
| @@ -417,7 +417,7 @@ def inspect_code(headers, cpp_opts, structs, defines): | |||
| # Run the compiled program. | |||
| show('Calling generated program...') | |||
| try: | |||
| info = shared.run_js(js_file[1]).splitlines() | |||
| info = shared.run_js(js_file[1], shared.NODE_JS).splitlines() | |||
There was a problem hiding this comment.
is this needed? the other change in this PR will ensure JS_ENGINES exists with at least node. so it seems like this line would use node anyhow, and we can avoid hard-coding it here?
There was a problem hiding this comment.
But I want to avoid depending on JS_ENGINES. Its less error prone if we run these internal tools always under node, and never under some other engine. We should probably have a helper for this and compiler.js and any other internal js tools.. where we want to always use node regardless of JS_ENGINES.
There was a problem hiding this comment.
But I admit its redundant, I fixed this is two ways.
There was a problem hiding this comment.
I don't feel strongly about this, but I do think it's unnecessary information to say "node" here. This file doesn't care about node, it just needs some JS engine to run in.
Or am I missing something with node's importance here?
There was a problem hiding this comment.
OK I'll leave this out of this change, but I do want to be clear that outside of testing we only support node. For compiler.js etc.. and here.
Since we are guaranteed that `NODE_JS` is part of the config (since all users need this) we can set a sensible default for `JS_ENGINES`. This default was supposed to be part of #9469, but a bug meant the code was dead and it was removed in #9499. `JS_ENGINES` is needed for any users that want to run test code. Fix `gen_struct_info.py` to explicitly use NODE_JS since this is the only engine we expect users to have installed and the only one we officially support outside of test code.
sbc100
force-pushed
the
allow_missing_js_engines
branch
from
9cdeee7 to
71372a5
Compare
kripken
approved these changes
Oct 8, 2019
belraquib
pushed a commit
to belraquib/emscripten
that referenced
this pull request
Dec 23, 2020
Since we are guaranteed that `NODE_JS` is part of the config (since all users need this) we can set a sensible default for `JS_ENGINES`. This default was supposed to be part of emscripten-core#9469, but a bug meant the code was dead and it was removed in emscripten-core#9499. `JS_ENGINES` is needed for any users that want to run test code.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 13, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 13, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 13, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 13, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 13, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 13, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 14, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
sbc100
added a commit
to emscripten-core/emsdk
that referenced
this pull request
Mar 14, 2024
`COMPILER_ENGINE` was completely removed from emscripten in emscripten-core/emscripten#9469. `JS_ENGINES` is only used/needed for testing and is no longer needed as of emscripten-core/emscripten#9542.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we are guaranteed that
NODE_JSis part of the config (sinceall users need this) we can set a sensible default for
JS_ENGINES.This default was supposed to be part of #9469, but a bug meant the code
was dead and it was removed in #9499.
JS_ENGINESis needed for any users that want to run test code.