Issue with UNC paths on Windows

[gotdibbs]

When a UNC path is provided as part of the pattern to match, it is incorrectly transformed to an invalid path on the local computer.

e.x. \\nas\node\ incorrectly becomes /nas/node which is treated as C:\nas\node\.

[treet]

Yes, I've run into this problem too. The readme states to only use forward slashes on Windows, but I'm wondering what the reasoning behind that restriction is.

Is there any fundamental problem that prevents implementing support for UNC paths?

[gotdibbs]

@treet To be honest, I haven't had time to ramp up on node-globs code base to take a look at how support for this would work just yet. However, FYI this came up for me because Azure seems to use only UNC paths.

[tuxracer]

As a workaround I've used:

path = require('path');
glob.sync(filename).map(function (item) {
  return path.resolve(item);
});

Example: https://github.com/tuxracer/stylus-import-tree/blob/master/src/index.coffee

[s-trooper]

i have the same problem.
i use plain js and the workaround don't help me.

[staticshock]

Have a look at this pull request, which purports to implement decent UNC path handling. \\host\dirname is basically treated as if they were a drive letter, like C:, so wildcards don't work on that portion of the path, just like *: wouldn't have worked for a drive letter.

That can probably be improved, but, alas, I couldn't figure out a quick way to do it, because path.resolve("\\\\host") gets treated as a relative path and resolves to $PWD\host, so \\host can't be used as a valid root.

[staticshock]

I looked into it a bit more, and found another reason why \\host\dirname should probably remain as the root: fs.readdirSync("\\\\host") doesn't work anyway:

> fs.readdirSync("\\\\vboxsvr")
Error: ENOENT, no such file or directory 'C:\vboxsvr'
    at Object.fs.readdirSync (evalmachine.<anonymous>:654:18)
    at repl:1:5
    at REPLServer.self.eval (repl.js:110:21)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.EventEmitter.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)
    at ReadStream.EventEmitter.emit (events.js:98:17)
> fs.readdirSync("\\\\vboxsvr\\test")
[ 'child1',
  'child2' ]

[staticshock]

Updated the PR to work with v4.2. I can change it further if you throw Feedback() at me.

[isaacs]

Sorry for the delay, I've been meaning to get to this, and hoped that I would over the thanksgiving holiday, but it slipped.

I'll try to land this week. Thanks for your patience.

[isaacs]

Don't worry about rebasing on master, I think the patch should land pretty cleanly, and it's relatively simple, so it shouldn't be too hard for me to pull forward if necessary.

[staticshock]

No problem. I was thinking a bit about #123, though, and I realized that they have roughly the same solution, so here's a modified pull request that should fix the other issue as well (basically I just replaced isUnc with isAbsolute.)

[tomwayson]

This is keeping me from being able to use grunt-contrib-config to copy files from a network file share w/o first having to map the drive in Windows.

[stefanpenner]

Ah, just ran into this one as well. I can confirm that @staticshock's fix appears to work.

@isaacs just a friendly reminder in-case this one has falling off your radar.

[isaacs]

@stefanpenner et al: See #146. I need a test that passes with the patch, and doesn't pass without the patch.

Nothing can be landed in this library with a valid test. I'm sorry for being so strict about this, but it's the only thing that makes maintenance of this module even remotely possible.

[stefanpenner]

Nothing can be landed in this library with a valid test. I'm sorry for being so strict about this, but it's the only thing that makes maintenance of this module even remotely possible.

I absolutely agree, good coverage is paramount.

I was sending the ping, not to merge, but to make sure you had not forgotten the issue in the first place. As there may have been a small chance you have time to address your own concerns. I suspect based on the comment you do not.

Although I have a work-around, I would prefer to not have to play wack-a-mole going forward. So let me see if I can nail down that test case for you this afternoon.

[staticshock]

I've already submitted a test case that works on Windows, and haven't really had any time to look at it since then. So if you tackle this, make sure your approach meets @isaacs's "must run on linux" criterion.

[stefanpenner]

@isaacs ah, it seems like this library should actually be run on a windows CI as well. What are your thoughts on this? I will gladly do the work to make it run on appveyor.

But, If it must provide "full coverage" on *nix, I can also dig in. Although doing it elegantly might require some more refactoring.

[isaacs]

Yes, the test needs to run on Unix (mostly so I don't inadvertently destroy it some day, which I probably will otherwise).

Windows CI would be awesome, and I'd greatly appreciate getting this module running somewhere. But it's not a replacement for a test that runs on Unix.

[stefanpenner]

Windows CI would be awesome, and I'd greatly appreciate getting this module running somewhere. But it's not a replacement for a test that runs on Unix.

proposed resolution:

• get this project working running / testing on appveyor
• ensure @staticshock's scenario is also testable in some way on nix

c/d ?

[isaacs]

Yes, plus one, would merge. I suggest hijacking or mocking whatever is necessary to make the patch in #146 testable on unix. That's actually all that's missing from getting to UNC support; appveyor is optional.

[stefanpenner]

If memory serves a mixture of UNC and cwd was the pandoras box I noticed. Basically if cwd was a UNC path, fun things would happen.

My PR (although some more massaging needed) even without the cwd support, would be a really good step in the correct direction. Fixing all the scenarios I could come across, except if a user changed the cwd to a UNC path. I suspect the cwd stuff could also be tackled, but maybe at a later time.

@isaacs would you be alright with improved UNC support being merged, but leaving some scenarios related to cwd also being on a UNC path for a future PR ?

Although not ideal, it would likely fix most users problems, without introducing any known regressions.

[cmawhorter]

has anyone explored require('path').normalize? from looking at the path.js source it looks like a lot of cross-platform work has been done and it supports UNC. i'm doing glob() { if (IS_WIN) path.normalize } in my workaround to fix my specific issue.

and there is cygwin paths which might be worth exploring?

[riteshatsencha]

Any updates on this issue? Facing the same issue. Thx.

[designerx2]

Another user stung by the UNC issue. Is there a temporary work around for this issue? Any assistance is greatly appreciated. Thank you.

[michaeldewayneharris]

Any updates on this issue? Facing the same issue. Thx.

[Nepoxx]

Is this "simply" a case of passing the right path to the OS or is it more complicated in that you need some NFS specific code for node's fs?

[mpaland]

Late to this party... Having this issue on a Win10 node trying to access/scan a NAS like '\\host\my_media\**\*'.
@isaacs @stefanpenner Is there a mergable PR to fix this long standing UNC issue on Windows?
Would be very appreciated!

[pste]

@mpaland The party's not over :)
No needed to fix, use the cwd parameter:
glob("**/*.pdf", {cwd:"//host/my_media"}, (err, files) => { if (err) throw err; console.log(files) });

[bernardoadc]

@pste solution works with Windows drive letters as well ( {cwd:"C:"} )

[ray007]

I'm not using glob directly but through gulp.src(), and setting the cwd option there does not seem to help.

Not really familiar with the code, but after a quick look, could the problem be in the minimatch module?

[ray007]

Not sure what I did wrong last week, but the cwd option did just make things work.
Back to the original issue, because gulp.src() still fails.

[sangimed]

@mpaland The party's not over :)
No needed to fix, use the cwd parameter:
glob("**/*.pdf", {cwd:"//host/my_media"}, (err, files) => { if (err) throw err; console.log(files) });

EDIT: My bad :) I misplaced the sharedFolder so it's glob.sync("**/*.txt", {cwd:"//remoteComputerName/sharedFolder"}) instead of glob.sync("sharedFolder/**/*.txt", {cwd:"//remoteComputerName"})

Tried glob.sync("sharedFolder/**/*.txt", {cwd:"//remoteComputerName"}) in Windows Server 2012 R2 but didn't work (\\remoteComputerName works when accessed from the Explorer windows app).

PS: Only one folder is shared in the remoteComputerName (the very folder I need to run glob in) so I can't access to any drive of the remote.

[aza547]

@mpaland The party's not over :) No needed to fix, use the cwd parameter: glob("**/*.pdf", {cwd:"//host/my_media"}, (err, files) => { if (err) throw err; console.log(files) });

This works for me, but seems to be far less performant than the initial method (so much so that it's not viable in my application sadly).

[isaacs]

UNC paths are first class citizens now in v9. Only took 10 years to fix this bug 😬