Only add node/bin to user's PATH if one is not already found #1189
Conversation
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.
|
After much back and forth in my thinking on this I'm re-opening this PR with the hope of landing it this time. The status quo which is to shadow any existing version of node in the user's PATH has causes many folks to complain, and I think its not enough to add a way to opt out of it. I think the shadowing needs to not happen in the default case to avoid yet more issues being filed on this. A recent issue relating to this problem is #1183, which is an attempt to update the version of linux we use, so that we can then update the version of node that we ship. That PR was triggered simply because we were injecting our version of node, overriding the already installed one. Once we fix this issue, such changes won't be necessary and we de-couple our internal use of node from the external use of node by our users. Another attempt at fixing this is #1189 which add an explicit opt out and hard-codes the logic into emsdk.py itself. Another issue complaining out our old node: #1173. Again, most likely not an issue once we fix this. In order to rationalize why I think landing this change is the right thing to do I broke down our users into different types to consider who this change will effect them.
The current behaviour of emsdk favors group 3, and my change favors group 2. I can't imagine any users in group 3 at all.. but we have evidence that there are many users in group 2 based on the bug reports we have been getting. So are there any users in group 3? Can we imagine having a working version of node, but then preferring to use the old/pinned version that comes with emsdk, but only when using the emsdk environment? The risk of landing this change is that if we do have users in group 3 they would end up with their own version of node being first in the PATH at all times. If I'm wrong and there are folks in group 4 who want to explicitly call the emsdk-internal version of node could still use it via |
Perhaps on a machine where the system node is very old, perhaps even too old to work with the emsdk? |
Yes, if such a user does exists they would be (as far as I can tell) the only negatively effected users.. and they would still have the option of using |
|
Makes sense. I think I agree this is the best path, even if no option is perfect. What kind of errors would that affected group get? Do we point them to |
|
I think I had once version of this that checked the node version overrode it only when it was older than the system version.. but that seems unnecessarily complex. Its also not true that it would make those users happy.. it could be that they want to use the old/system version of node. I think never shadowing is simpler and more straight forward and the right thing to do for 99.XX% of users. |
Such as user would be somebody trying to run something one the command line right after building it, and they would have to be running a version of node that was below our I'm not sure we actually enforce or check |
|
Sounds good about the runtime error. Do we also need a check in the node code that we run during compilation? Or no I guess because that does always use the emsdk node? |
Yes, we already use the internal/emsdk version that in all cases. |
(But we already do have a version check there.. since it is also configurable) |
|
I'm going to wait for @juj here because he was the one who objected to this PR when I originally posted it. |
|
@juj can we get your (reluctant?) approval for this? |
|
I recall we had a conversation somewhere from before the consensus was to change emsdk_env to take in as parameter the list of tools to enter into PATH, e.g. Anyhow, LGTM for this, you make good points here that I am convinced. |
We could certainly do that (in addition to this change) if it would be useful to folks.
|
…ten-core#1189) 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#705.
…1209) This should have been part of emscripten-core#1189. See emscripten-core#1183.
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.
This change was originally proposed in #714 and we decided against it because there was some danger that it might confuse some users. However since we closes #714 more and more users have been frustrated buy the status quo. I also looked into several options for allowing users to opt out explicitly, but they all seems to have the same problem which is that the users don't want their version of node shadowed in the default case.
Part of fix for #705.