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

lib: allow long paths for require on Windows #1991

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 16, 2015

#1801 introduced internal fs methods to speed up require.
The methods do not call path._makeLong like their counterpart
from the fs module. This brings back the old behaviour.

Fixes: #1990
Fixes: #1980

@targos
Copy link
Member Author

targos commented Jun 16, 2015

I could not test it yet as I am not on a Windows machine right now.
I'll also add a test.

@vkurchatkin
Copy link
Contributor

LGTM, but it needs a test

@ChALkeR
Copy link
Member

ChALkeR commented Jun 16, 2015

Maybe it would be better to wrap internalModuleStat and internalModuleReadFile?
Just asking.

@targos
Copy link
Member Author

targos commented Jun 16, 2015

test added. I copied the filename generation from https://github.com/nodejs/io.js/blob/master/test/parallel/test-fs-long-path.js#L9-L12
The result is a path length of exactly 260 characters. Shouldn't it be at least 261 ?
If I read this correctly, 260 is still allowed.

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Jun 16, 2015
@mscdex
Copy link
Contributor

mscdex commented Jun 16, 2015

The commit should target the module subsystem instead of lib.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 16, 2015

@targos https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#maxpath states that those are 260 chars including the terminating NULL, so max safe path length is 259 significant chars (including the drive letter).

For example, the maximum path on drive D is "D:\some 256-character path string" where "" represents the invisible terminating null character for the current system codepage. (The characters < > are used here for visual clarity and cannot be part of a valid path string.)

But I guess it would do no harm to make the test path a big longer.

@piscisaureus
Copy link
Contributor

The test should clean up after itself and delete the temp file.
Other than that, LGTM.

@othiym23
Copy link
Contributor

Given that #1849, #1980, and #1990 affect npm (see npm/npm#8419 for details) on newer versions of io.js, I'm a strong +1 on getting this landed.

nodejs#1801 introduced internal fs methods to speed up require.
The methods do not call path._makeLong like their counterpart
from the fs module. This brings back the old behaviour.

Fixes: nodejs#1990
Fixes: nodejs#1980
Fixes: nodejs#1849
@targos
Copy link
Member Author

targos commented Jun 16, 2015

@ChALkeR I am now on my Windows machine and can confirm it fails with 260 chars so I'm leaving it like that.
@mscdex @piscisaureus followed your suggestions

@targos
Copy link
Member Author

targos commented Jun 16, 2015

I still can't confirm the fix is actually working (installing VS right now) so a CI could be useful

@piscisaureus
Copy link
Contributor

@piscisaureus
Copy link
Contributor

@nodejs/build the win2008r2 build slave seems to be stuck on a failed git clean -fdx.

@piscisaureus
Copy link
Contributor

Tested locally and landed (with a minor modification to the test) in 671e64a.
@targos Thanks for your patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants