-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 build failures when any parent directory is {"type": "module"} #17451
base: main
Are you sure you want to change the base?
Conversation
This fixes hard-to-debug failures when any parent directory has package.json with {"type": "module"} in it. Before emconfigure / conftest could fail with config.log error: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" The fix forces commonjs mode for generated Node scripts. fixes emscripten-core#13551 fixes emscripten-core#17431
@sbc100 What do you think about the general direction of this fix? I've added some details in the PR description. |
What's the conftest-like usage in option 3? .cjs sounds most explicit, but also like a larger change. |
It looks like Emscripten creates a But I'm not 100% sure. Emscripten is magical :-) |
This is not emscripten, but autoconf which like to make executables with this name and without and extension by default. I think this whole issue only really effects autoconf users since other users. One option is to try to persuade autoconf to use an extension when building executables like this. |
@sbc100 I've implemented your shell wrapper idea from #17431 (comment). It does work. |
with open(script, 'w') as f: | ||
f.write('#!%s\n' % cmd) | ||
logger.debug('writing %s JavaScript file' % cjs_script) | ||
with open(cjs_script, 'w') as f: | ||
f.write(src) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just to shutil.rename(script, cjs_script)
.. this avoid the need for the read_file
above.
# wrapper that calls node with this .cjs script, because autoconf | ||
# does not expect its conftest binary to have an extension. | ||
# See https://github.com/emscripten-core/emscripten/issues/17431 | ||
cjs_script = script + '.cjs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want to do this if the name of the output file does not have an extension already. e.g. conftest
. If the user does -o out.js
it would be a little odd to have out.js
be shell script that runs out.cjs
.
So I think maybe we need both old and new methods. You can do if not shared.suffix(script):
to check this.
The issue can be circumvented by setting the ac_cv_exeext=.cjs emconfigure ... This also configures the Perhaps the |
An alternative is to create a |
Fix hard-to-debug failures when any parent directory has
package.json
with{"type": "module"}
.Before this fix
emconfigure
/conftest
would fail with an error inconfig.log
, and prevent the build from succeeding:To fix the problem, we add .cjs extension to generated Node scripts, forcing
commonjs
mode. In addition, we add a shell wrapper that calls node with this script, to make autoconf happy. Autoconf does not expect its conftest binary to have an extension.Other solutions:
commonjs
mode. See--input-type=commonjs
documentation. Not good, the scripts would not to be able to access stdin anymore.echo '{"type":"commonjs"}' > package.json
in the build directory. Simple, but not good if the build directory has its ownpackage.json
already.--experimental-loader
parameter. It works, but the feature is unstable and depends on Node version.Related Node issues:
"type": "module",
is specified nodejs/node#34049fixes #13551
fixes #17431