-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Conversation
I could not test it yet as I am not on a Windows machine right now. |
LGTM, but it needs a test |
Maybe it would be better to wrap |
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 commit should target the |
@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).
But I guess it would do no harm to make the test path a big longer. |
The test should clean up after itself and delete the temp file. |
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
@ChALkeR I am now on my Windows machine and can confirm it fails with 260 chars so I'm leaving it like that. |
I still can't confirm the fix is actually working (installing VS right now) so a CI could be useful |
@nodejs/build the win2008r2 build slave seems to be stuck on a failed |
#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