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.

Sign up for GitHub

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

Jump to bottom

module: fix stat with long paths on windows #2013

Closed
wants to merge 1 commit into from
Closed

module: fix stat with long paths on windows #2013

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 18, 2015

Fix from #2011 with is an updated regression test that fails without the patch.

v2.2.1

Error: Cannot find module 'D:\Documents\GitHub\io.js\test\tmp\xxxxxxxxxxxxxxxxxx
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\index.js'

master

Error: Cannot find module './other'

master with fix

No error !

@mscdex mscdex added module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform. labels Jun 18, 2015
@yosuke-furukawa
Copy link
Member

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/50/

yosuke-furukawa
yosuke-furukawa reviewed Jun 19, 2015
View reviewed changes
test/parallel/test-require-long-path.js
var fileNameLen = Math.max(261 - common.tmpDir.length - 1, 1);
var fileName = path.join(common.tmpDir, new Array(fileNameLen + 1).join('x'));
var fullPath = path.resolve(fileName);
const dirNameLen = Math.max(261 - common.tmpDir.length - 1, 1);
Copy link
Member

Choose a reason for hiding this comment

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

IMO, 260 - common.tmpDir.length is simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I changed it

@silverwind silverwind mentioned this pull request Jun 19, 2015
Closed
@targos targos force-pushed the fix-module-long-path-windows branch from 9a4c26a to f448125 Compare June 19, 2015 06:10
@targos
Copy link
Member Author

targos commented Jun 19, 2015

addressed @yosuke-furukawa comments.
I removed the assert part so it is clearer that we are checking the require call itself, not what it returns.

@targos targos force-pushed the fix-module-long-path-windows branch from f448125 to 61e66e0 Compare June 19, 2015 06:15
@yosuke-furukawa
Copy link
Member

LGTM

@bnoordhuis
Copy link
Member

LGTM but what's the reason for modifying an existing test instead of adding a new one? Two regression tests are better than one.

@targos
Copy link
Member Author

targos commented Jun 19, 2015

Sorry, I thought it would be redundant.
require(indexFile) is like the existing test and the nested require('./other') is testing the new fix (it needs a parent directory with long path).
So the test for the new fix needs the other one to pass.

@Fishrock123 Fishrock123 mentioned this pull request Jun 19, 2015
Closed
rvagg pushed a commit that referenced this pull request Jun 22, 2015
@targos @rvagg
module: fix stat with long paths on Windows
a4f4909 
PR-URL: #2013
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg
Copy link
Member

rvagg commented Jun 22, 2015

landed @ a4f4909

@rvagg rvagg closed this Jun 22, 2015
@targos targos deleted the fix-module-long-path-windows branch June 22, 2015 12:31
@gr00nd gr00nd mentioned this pull request Feb 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@targos @yosuke-furukawa @bnoordhuis @rvagg @mscdex