Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Oct 17, 2025

Fix tools/install.py script to use Emsdk Node.js and NPM. Fixes other.test_install on macOS if system macOS Node.js version is out of date.

tools/install.py Outdated


def get_npm():
node = os.getenv('EMSDK_NODE')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use shared.NODE_JS instead of EMSDK_NODE here? I'd rather not use these emsdk-specific things here if we can avoid it.

shared.NODE_JS is guaranteed to exist.

Maybe install.py isn't setup to import shared.py yet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm.. I guess bootstrap.py has the same issue? Is this script also not working your CI machine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use config.NODE_JS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bootstrap.py does work. It is only the test other.test_install that is failing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems odd because bootstrap.py also runs npm from the PATH.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 17, 2025

macOS if system macOS Node.js version is out of date.

What does the failure look like in this case?

@juj
Copy link
Collaborator Author

juj commented Oct 17, 2025

The failure looks like

image

@juj
Copy link
Collaborator Author

juj commented Oct 17, 2025

Hmm, the changes are resulting in these errors: https://github.com/emscripten-core/emscripten/actions/runs/18601976754/job/53042367932

I am a bit stumped as to what could cause this.. it is as if .emscripten no longer exists, but the changes here shouldn't affect that?

@juj
Copy link
Collaborator Author

juj commented Oct 17, 2025

The version that relied on EMSDK_NODE env. var. above did pass CI tests: https://github.com/emscripten-core/emscripten/actions/runs/18589062792/job/52999639477

@sbc100
Copy link
Collaborator

sbc100 commented Oct 17, 2025

I think the archive step is assuming the make install (and by extension) install.py don't require a fulling configured version of emscripten to work.

i.e we are assuming the install.py is completely standalone... but it seems like maybe it cannot actually be standalone.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 17, 2025

Could you maybe update the test_install test to use env_with_node_in_path when it runs install.py?

@juj juj force-pushed the fix_tools_install_script branch from 79c7557 to 29a72c8 Compare October 17, 2025 19:54
@juj
Copy link
Collaborator Author

juj commented Oct 17, 2025

Oh right, it is the mere act of attempting to import shared that requires .emscripten config to be present.

Updated to change the test instead.

@juj juj enabled auto-merge (squash) October 17, 2025 19:56
@juj juj merged commit ea6a722 into emscripten-core:main Oct 17, 2025
34 checks passed
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