upgrade npm to 3.7.1 by iarna · Pull Request #5097 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation24 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Issues and PRs related to the npm client dependency or the npm registry.
label
I'm getting some weirdness over here
npm ERR! code MODULE_NOT_FOUND
npm ERR! Cannot find module 'internal/util'
Eeek!
It seems like the tests on v5.x work as expected as do the tests at 76cb81b when it was last upgraded.
The previous commit with v3.6.0 is experiencing the same weirdness, so I think this is on our end.
Bisecting
Looks like it was 1124de2 which landed about 4 hours ago and it is semver major
¯_(ツ)_/¯
PR for reference #4525
/cc @thefourtheye @jasnell @ChALkeR @bnoordhuis
It looks like there was a search against the ecosystem, but perhaps not done against npm itself
edit:
I just have to note how crazy it is the a deprecation that was supposed to happen in 2010 finally lands an hour before a release that it breaks... I'm without words
@thealphanerd, it should not fail, the API change probably had nothing to do with it.
Most probably something is re-executing source code of the fs module, monkey-patching it. Will take a look later today.
@ChALkeR my thoughts exactly. Seemed very odd that this would break it. npm is effectively broken on master right now
@thealphanerd 1124de2 introduced require('internal/util'), which is not requirable from the external code (i.e. not from the Node.js core), so anything that wraps fs module code, monkey-patches it and re-executes it (as graceful-fs was doing a while ago) is broken by that.
@ChALkeR should we be doing a try catch on that require?
edit: perhaps we can move this discussion to #4525
@thealphanerd Why? I think we decided against supporting monkey-patching core modules in that way. Some module required by npm should be fixed.
Could you give me a trace, please?
I was unaware of not supporting monkey-patching, but that is reasonable.
That being said This version of npm has some pretty important performance improvements. Do you think it makes sense to temporarily revert the offending change until the next release of npm which can perhaps fix this?
Okay. Actual problem is one of the npm's dependencies are still using older version of cmd-shim, which uses older, monkey-patched version of graceful-fs
Excerpt from npm-debug.log file generated when I ran make test-npm locally
258 verbose stack Error: Cannot find module 'internal/util'
258 verbose stack at Function.Module._resolveFilename (module.js:339:15)
258 verbose stack at Function.Module._load (module.js:290:25)
258 verbose stack at Module.require (module.js:367:17)
258 verbose stack at require (internal/module.js:16:19)
258 verbose stack at evalmachine.<anonymous>:38:26
258 verbose stack at Object.<anonymous> (/home/thefourtheye/git/io.js/test-npm/node_modules/cmd-shim/node_modules/graceful-fs/fs.js:11:1)
258 verbose stack at Module._compile (module.js:417:34)
258 verbose stack at Object.Module._extensions..js (module.js:426:10)
258 verbose stack at Module.load (module.js:357:32)
258 verbose stack at Function.Module._load (module.js:314:12)
Update:
On further investigation, npm itself is using cmd-shim@~2.0.1 and since there was no new release after merging npm/cmd-shim#14, npm still uses the monkey-patched graceful-fs (indirectly) and that is why this is failing. Because we don't allow userland modules to access modules in lib/internal and the older monkey-patched version of graceful-fs is trying to require internal/util. Probable fix is to have cmd-shim release a new version and then to have npm use the latest version of it.
/cc @nodejs/ctc
It seems we have #1898 again. Some historic discussion in #2026.
edit:
removed as I was totally off base, thanks @ChALkeR
@nodejs/ctc My opinion here is that we should in fact fix 1124de2 on the Node.js side for now, but add an explicit warning there, for at least one major version (v6.x), so that everyone who monkey-patches fs in that way (e.g uses an old version of the graceful-fs module) will be notified of the problem and will get off it, because sudden breakage with a bogus error message could be confusing.
This does not cancel the fact that the problem should be fixed on cmd-shim/npm side, of course.
I will make a PR for that when I'll get home.
Can we just remove lower versions of graceful-fs from the dep tree?
This was fixed ages ago in graceful-fs and I am not particularly sympathetic to it blatantly abusing the runtime code.
@Fishrock123 That has to be done, of course, I'm not suggesting leaving an old version of graceful-fs hanging there.
As soon as the updated cmd-shim is published I'll add a commit to this PR that updates it. (It'll be two weeks before it shows up in the version of npm being upstreamed directly.)
@Fishrock123 We can't just remove the deeper one's from the dep tree, w/o patching cmd-shim or we'd be shipping a version of npm that's marked as invalid when you do an npm ls -g.
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request
This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module.
To be reverted in Node.js 7.0.
Fixes nodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525.
jasnell pushed a commit that referenced this pull request
This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module.
To be reverted in Node.js 7.0
Fixes: #5097, see also #1898, #2026, and #4525. PR-URL: #5102 Reviewed-By: Rod Vagg rod@vagg.org Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
stefanmb pushed a commit to stefanmb/node that referenced this pull request
This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module.
To be reverted in Node.js 7.0
Fixes: nodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525. PR-URL: nodejs#5102 Reviewed-By: Rod Vagg rod@vagg.org Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl
Labels
Issues and PRs related to the npm client dependency or the npm registry.