-
Notifications
You must be signed in to change notification settings - Fork 43
Fix treatment of long file paths in Windows #42
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// This test creates a file with a path that's over 300 characters | ||
// long, which is longer than the Windows limit unless you use the | ||
// '\\?\' prefix. | ||
// https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx | ||
// | ||
// Then it passes that directory into and out of fstream, to see if | ||
// that file comes out the other side. This tests | ||
// https://github.com/npm/fstream/issues/30 | ||
|
||
var tap = require('tap') | ||
var temp = require('temp').track() | ||
var fs = require('fs') | ||
var path = require('path') | ||
var mkdirp = require('mkdirp') | ||
var fstream = require('../fstream.js') | ||
|
||
tap.test('long file paths', function (t) { | ||
var inputDir = temp.mkdirSync('fstream-test-input') | ||
var outputDir = temp.mkdirSync('fstream-test-output') | ||
|
||
var longDir = inputDir | ||
while (longDir.length < 300) { | ||
longDir = path.join(longDir, 'subdirectory') | ||
} | ||
|
||
var STAMP = 'stamp' | ||
|
||
mkdirp.sync(longDir) | ||
var inputStampedFile = path.join(longDir, 'file') | ||
fs.writeFileSync(inputStampedFile, STAMP) | ||
|
||
var onPipeComplete = function () { | ||
var outputStampedFile = inputStampedFile.replace(inputDir, outputDir) | ||
t.equal(fs.readFileSync(outputStampedFile, 'utf-8'), STAMP) | ||
t.end() | ||
} | ||
|
||
var reader = fstream.Reader(inputDir) | ||
reader.on('end', onPipeComplete) | ||
reader.pipe(fstream.Writer(outputDir)) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,28 @@ | ||
var fstream = require('../fstream.js') | ||
var notOpen = false | ||
|
||
fstream | ||
.Writer({ | ||
path: 'path/to/symlink', | ||
linkpath: './file', | ||
isSymbolicLink: true, | ||
mode: '0755' // octal strings supported | ||
}) | ||
.on('close', function () { | ||
notOpen = true | ||
var fs = require('fs') | ||
var s = fs.lstatSync('path/to/symlink') | ||
var isSym = s.isSymbolicLink() | ||
console.log((isSym ? '' : 'not ') + 'ok 1 should be symlink') | ||
var t = fs.readlinkSync('path/to/symlink') | ||
var isTarget = t === './file' | ||
console.log((isTarget ? '' : 'not ') + 'ok 2 should link to ./file') | ||
}) | ||
.end() | ||
// No symlinks on Windows | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
if (process.platform !== 'win32') { | ||
fstream | ||
.Writer({ | ||
path: 'path/to/symlink', | ||
linkpath: './file', | ||
isSymbolicLink: true, | ||
mode: '0755' // octal strings supported | ||
}) | ||
.on('close', function () { | ||
notOpen = true | ||
var fs = require('fs') | ||
var s = fs.lstatSync('path/to/symlink') | ||
var isSym = s.isSymbolicLink() | ||
console.log((isSym ? '' : 'not ') + 'ok 1 should be symlink') | ||
var t = fs.readlinkSync('path/to/symlink') | ||
var isTarget = t === './file' | ||
console.log((isTarget ? '' : 'not ') + 'ok 2 should link to ./file') | ||
}) | ||
.end() | ||
|
||
process.on('exit', function () { | ||
console.log((notOpen ? '' : 'not ') + 'ok 3 should be closed') | ||
}) | ||
process.on('exit', function () { | ||
console.log((notOpen ? '' : 'not ') + 'ok 3 should be closed') | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,15 +97,8 @@ function Reader (props, currentStat) { | |
|
||
self._path = self.path = path.resolve(props.path) | ||
if (process.platform === 'win32') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If UNC path support was added to Node core after this code was put together, then it's probably OK to nuke this whole block, except maybe a call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Node has been doing the long-path transformation internally since version 0.5.something. Unless you guys still support node v0.4, you should indeed nuke this bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're masochists, but we're not that masochistic. This can go, then. Thanks, Bert! |
||
// XXX What is this code for? | ||
self.path = self._path = self.path.replace(/\?/g, '_') | ||
if (self._path.length >= 260) { | ||
// how DOES one create files on the moon? | ||
// if the path has spaces in it, then UNC will fail. | ||
self._swallowErrors = true | ||
// if (self._path.indexOf(" ") === -1) { | ||
self._path = '\\\\?\\' + self.path.replace(/\//g, '\\') | ||
// } | ||
} | ||
} | ||
self.basename = props.basename = path.basename(self.path) | ||
self.dirname = props.dirname = path.dirname(self.path) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together, and for including the useful and informative comment.