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

Only add node/bin to user's PATH if one is not already found #714

Closed
wants to merge 1 commit into from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Feb 15, 2021

If the user already has a version of node in their PATH don't clobber
it. This doesn't effect emscripten since the version of node we use
there is controlled via the config file, not via PATH.

Part of fix for emscripten-core/emscripten#705.

If the user already has a version of node in their PATH don't clobber
it.  This doesn't effect emscripten since the version of node we use
there is controlled via the config file, not via PATH.

Part of fix for #705.
@sbc100 sbc100 requested review from kripken and juj February 15, 2021 22:12
@juj
Copy link
Collaborator

juj commented Feb 16, 2021

I don't think this kind of conditional behavior is a good idea, it can lead to cryptic corner cases.

Also when people enter the emsdk environment with emsdk_env, I don't think it is a problem to activate the emsdk tools in it. The user did in fact enter the emsdk environment! (With python the root issue was that our bundled python has broken pip, not that we activated python in PATH)

If users don't want to get node, they could emsdk activate the individual tools that make up the sdk, and leave out node from it. Then emsdk won't add node in it?

@sbc100
Copy link
Collaborator Author

sbc100 commented Feb 16, 2021

I don't think this kind of conditional behavior is a good idea, it can lead to cryptic corner cases.

Sure, I agree with this point.

Also when people enter the emsdk environment with emsdk_env, I don't think it is a problem to activate the emsdk tools in it. The user did in fact enter the emsdk environment! (With python the root issue was that our bundled python has broken pip, not that we activated python in PATH)

IMHO, its not just that pip3 didn't work, its that by doing that we clobber any python installation the user might have on their machine. We completely switch up the python installation. Even if our installation works fine that is still rather rude.

If users don't want to get node, they could emsdk activate the individual tools that make up the sdk, and leave out node from it. Then emsdk won't add node in it?

I'm not sure how to do that (how could I install latest and leave out node today?), but I don't care to much because I actually do want to use emsdk's node.

I guess I will abandon this until we get users who really care that we clobber the node in their existing PATH.

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 30, 2021

It seems that some folks really don't want our version of node clobber the one in their PATH: #1142

Perhaps we could at least put our version at the end of the PATH?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 4, 2022

Another user on #1142 surprised/annoyed by having node PATH clobbered by emsdk.

I'm not sure how easy it is to install our tools without node... and what is more we do want to use our own version of node internally even if the user has a more recent one in their path.

In short there are two versions of node in play here:

  1. The node we use internall to run js compiler (this should be invisible to the users and does not need to be in the PATH)
  2. The node version users will run the output in.

We think we should find some way for users to easily overide (2)... my person opinion is that emsdk should not even provide (2) at all.. at least it shouldn't put it on the PATH. We don't provide a browser runtime.. why should we provide a node runtime? We expect folks have their own runtimes if they want to execute the output.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 4, 2022

BTW our bundled python no longer has a broken pip.. but I still think its important/good that we don't overide the version of python in the user's PATH.

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