Fix treatment of long file paths in Windows #42
Conversation
This test reads and write a directory with a path that is above 300 characters long, and checks that it comes out the other side. Currently, this test fails on Windows due to https://www.npmjs.com/package/standard. A subsequent commit will fix this.
3e5d171 (from November 2011) introduced this code to fix treatment of long file paths in Windows. Then, one week later, this commit fixed long file paths directly in Node's `fs` library: nodejs/node-v0.x-archive@1f16a7b So, the code currently in fstream actually /breaks/ long file paths in Windows. This fixes it, but keeps an additional mysterious line introduced by that original commit that I don't understand. Fixes npm#30. Thanks to @sdarnell for identifying the problem.
The fix is actually in npm/fstream#42, but now we also remove our explicit path length check that used to throw an error instead of silently losing files. This commit also adds a self-test to test the entire flow through `files.createTarball` and `files.extractTarGz`.
| console.log((isTarget ? '' : 'not ') + 'ok 2 should link to ./file') | ||
| }) | ||
| .end() | ||
| // No symlinks on Windows |
There was a problem hiding this comment.
Why make this change? Windows has had symbolic links basically forever, and since Windows 7 (I believe), it hasn't even always required Administrator access to create them. One of the things we'd really like to do as maintainers is add a proper test suite to this package, and having parity coverage on both Windows and Unix is important.
There was a problem hiding this comment.
Whilst Windows does have a selection of things that are like symlinks, none of them are quite the same, or node doesn't handle them properly, or they need permissions that are not available to regular users with the default installation options (and rarely in enterprise setups). They only work on NTFS, and often confuse the heck out of many tools which should know better. Apologies if my war wounds are flaring up, but I've concluded that they are more trouble than they are worth. However, if the security policy is changed, and node were fixed (possibly done already), true NTFS symbolic links are pretty close to unix symlinks.
There was a problem hiding this comment.
I partially agree - unix-like symlinks that have been available since Vista probably aren't worth the effort, since they require special permissions and/or elevation. Additionally windows requires you to specify the type of the target (file of directory) when the symlink is created, which is weird.
Junctions are more limited, but they're universally available and don't require special permissions. For most intents and purposes (e.g. NPM) they're good to use, and I would advise to do so. There are some caveats though:
- Junctions can only point at directories.
- Junctions always have to contain absolute paths.
- Node internally resolves the target path when a junction is created with a relative path, however in most node versions (up to v0.10, and even in v0.12 I believe) there's a bug in it. See Relative junction should behave like dangling symlink nodejs/node-v0.x-archive#8813
- Very old & unsupported versions of windows (XP prior to SP1 I believe) had some serious issues where a junction would be traversed when recursively deleting a directory. But nobody uses that any more, not even in China.
- Node treats junctions like symlinks for most intents and purposes;
stat,lstat,readlink,realpathetc. all deal with them properly.
|
I have some comments on this, and I'd like to get the eyes of somebody who has a better grasp of Node's history with UNC paths to take a look at this (@piscisaureus, is this something you could take a look at, or do you know who we could ask to take a look at it?). This is a more or less invisible piece of npm's infrastructure, so touching it is a little nervewracking for me, but if there's code in here that complicates filesystem handling on Windows, it would be good to get rid of it. |
3e5d171 (from November 2011) introduced code to fix treatment
of long file paths in Windows. Then, one week later, this commit
fixed long file paths directly in Node's
fslibrary:nodejs/node-v0.x-archive@1f16a7b
So, the code currently in fstream actually /breaks/ long file
paths in Windows.
This fixes it, but keeps an additional mysterious line introduced
by that original commit that I don't understand.
I also added an automated test for this, and also guarded the
existing symlink test to not run on Windows... It would be nice
if the test suite were set up to automatically run on Windows as
well.
Fixes #30. Thanks to @sdarnell for identifying the problem.