Skip to content

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

Closed
sbc100 wants to merge 1 commit intomasterfrom
skip_node_path
Closed

Only add node/bin to user's PATH if one is not already found#714
sbc100 wants to merge 1 commit intomasterfrom
skip_node_path

Conversation

@sbc100
Copy link
Copy Markdown
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 juj and kripken February 15, 2021 22:12
@juj
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

akoeplinger pushed a commit to akoeplinger/emsdk that referenced this pull request Dec 13, 2024
akoeplinger pushed a commit to akoeplinger/emsdk that referenced this pull request Dec 13, 2024
* Only run publish at join point (emscripten-core#714)

(cherry picked from commit 1ed00a9)

* Only sign and publish once

* Set asset manifest file name
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