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

globby is b0rked on Windows: .sync nor .async deliver /any/ result. #155

Open
GerHobbelt opened this issue Nov 8, 2020 · 8 comments
Open

Comments

@GerHobbelt
Copy link

Related to #152, but .sync is broken as well: doesn't matter what you try, globby won't produce any results.

Sample code:

var testset = globby.sync(__dirname + '/specs/*.jisonlex');
console.error({testset, dir: path.normalize(__dirname + '/specs/')});

produces this:

{
  testset: [],
  dir: 'W:\\Projects\\sites\\library.visyond.gov\\80\\lib\\tooling\\jison\\packages\\lex-parser\\tests\\specs\\'
}

while that directory is filled with tens of *.jisonlex files.

Ditto for the original code:

  var testset = globby.sync([
    __dirname + '/specs/*.jison',
    __dirname + '/specs/*.lex',
    __dirname + '/specs/*.jisonlex',
    __dirname + '/specs/*.json5',
    '!'+ __dirname + '/specs/*-ref.json5',
    __dirname + '/specs/*.js',
    __dirname + '/lex/*.jisonlex',
  ]);
console.error({testset, dir: __dirname});
@papb
Copy link

papb commented Nov 9, 2020

Hi! What NodeJS version? Did you try other versions?

@GerHobbelt
Copy link
Author

$ node --version
v12.18.4

No, haven't tried other versions, but have seen this issue before. Then I switched tactics as I was in a hurry to get things done, so I don't expect this to be a node version specific thing.

@GerHobbelt
Copy link
Author

Side Note: globby use like this (i.e. without directories in the search argument) works as expected:

globby([ 'ebnf-parser*.js' ])

@GerHobbelt
Copy link
Author

GerHobbelt commented Nov 15, 2020

New info

Worky 😥 (phew!)

This works on Windows:

process.chdir(__dirname);
var testset = globby.sync(['./specs/*.jisonlex', './lex/*.jisonlex']);

As long as you

  • DO NOT pass Windows path separators ('\') as delivered by path.normalize() et al 💢
  • DO NOT pass Windows drive / network share identifiers (e.g. 'W:' in 'W:\' which would be a absolute path in Windows) into globby anywhere, you'll be fine.

Not Worky 1

Ergo: doing something like

let basepath = path.normalize(path.join(__dirname, ......)).replace(/\\\\/g, '/'); // UNIXify path
// ^^^ INSUFFICIENT, as the Windows drive letter plus colon, e.g. 'W:/' will make it through
// and then globby will barf a hairball, delivering NIL results!
globby(basepath + '/specs/*.jisonlex', ......);

will FAIL.

Not Worky 2

Ditto for any inattentive coding like this, where you employ path.join just to shut up eslint et al -- as would happen with the case above:

(note the trouble-causing path.join in the globby statement; compare to previous example)

let basepath = path.normalize(path.join(__dirname, ......)).replace(/\\/g, '/'); // UNIXify path
// ^^^ AS PREVIOUS EXAMPLE ABOVE.
globby(path.join(basepath, 'specs/*.jisonlex', ......));
// ^^^ BAD JUJU: path.join screws you over as it will inject a '\' and you will be TOAST, once again.

Conclusion

Only work with paths coding and globby when you're totally hopped up on caffeine and bright and shiny yourself. 👼

"Using relative UNIXy paths only" won't save your bacon because:

  • your globbing MAY span multiple drives, e.g. ['D:/projectBla/', 'Z:/bit-of-network/projectBlaOverlord/'] -- no way out but to split those searches into one glob per drive and join up the results by hand. 😠 Buggerit Millenium Hand & Shrimp.
  • you MAY have used a Node path API a little late in the game (e.g. second example above: the second path.join in there) and your paths won't be relative and utterly UNIX any more. A non-exhaustive list of devils:
    • path.join
    • path.normalize
    • path.resolve
    • path.format

Be vewwy vewwy careful and on the look-out for hidden backslash-in-a-path delivery boys, such as:

  • __dirname
  • process.cwd()
  • anything else that heralds producing an absolute path

GerHobbelt added a commit to GerHobbelt/jison that referenced this issue Nov 16, 2020
…lled `braceArrowActionCode()`

- micro step towards merging this branch with the (currently still faulty!) master branch and make that one the main dev line once again.
  + this will take some significant migratory work on bnf.l + bnf.y before we can merge branches, though! master has a significantly advanced, yet buggy, new grammar spec, which never got finished as the plugs got pulled due to RL developments which are nearing the end after a year now   |:-(
- regenerated library files
- updated test reference files
  - as usual done brute-force via `make clean-dumpfiles ; make`
- removing testset file lists as globby is back in action -- see also sindresorhus/globby#155 (comment)
- added another test example grammar for #51 ; also added the required input file and minimal lexer for producing a Minimum Viable Test Product running the generated grammars (if there's any success there)
@phawxby
Copy link

phawxby commented Nov 22, 2020

@GerHobbelt yes, adding this test to test.js fails on Windows but passes on Mac/Unix.

test('glob - async - absolute', async t => {
	const temporaryAbsolute = path.resolve(temporary);

	const result = (await globby(path.join(temporaryAbsolute, '*.tmp'))).sort();

	t.true(result.length === 5);
	for (const [i, element] of fixture.entries()) {
		t.true(result[i].endsWith(element));
	}
});

However, as a workaround you can do .replace(/\\/g, '//'). This test will pass.

test('glob - async - absolute', async t => {
	const temporaryAbsolute = path.resolve(temporary);

	const result = (await globby(path.join(temporaryAbsolute, '*.tmp').replace(/\\/g, '//'))).sort();

	t.true(result.length === 5);
	for (const [i, element] of fixture.entries()) {
		t.true(result[i].endsWith(element));
	}
});

Definitely some kind of bug in handle of full system paths combined with path.sep.
cc. @sindresorhus

@GerHobbelt
Copy link
Author

@phawxby: yup, did another test and the multiple-drive search scenario seems to pass as well.

🤔 What did go wrong before over here then as when I did the replace thingy it was not enough? 🤔

A-ha! I did it too early: before the path.normalize() calls like in

globby([
   path.normalize(p1.replace(/\\/g, '/')),
   path.normalize(p2.replace(/\\/g, '/')),
  ...
], ...

which, 🤦 of course 🤦 , would happily convert those UNIX slashes back to Windows' \ backslashes again and thus throw me a curveball. 😢 (I did not re-check the replace-only work-around after I found out path.normalize was having me on when I wrote the previous message and results therein. 😓 )

So the chdir+useRelativePathsOnly approach is not required, while my list of culprits to watch out for stands:

Them Nasty Buggers

  • path.join
  • path.normalize
  • path.resolve
  • path.format
  • __dirname
  • process.cwd()
  • 😇 ... quite probably missed a couple backslash-stabbing others here ... 😇

Back on the topic of globby: what really fazes me now is the question how I got away with that __dirname-based stuff passing into globby. 🤔 Apparently I had some magic mix that I cannot bring back, as one thing's for sure: this was in a unit test rig which has run many times before (at least in 2018/2019).

What changed since that stuff got run last time?

Over time...

  • nvm-for-windows 👍 got installed and the node installs got ditched.
  • dev/test boxes got upgraded using nvm all the way from node v6 + v8 + v10 to node v12.
  • ncu (https://www.npmjs.com/package/npm-check-updates) was run on the projects everywhere.
  • Plus some bash scripting around npm install && npm prune && npm audit fix too, for good measure.

And after all these updates, said test rig went t*ts up. (Not a big surprise. What was a surprise was that globby was doing all the b0rking and once I got fed up and hardcoded the search result set using UNIX find and some shell scripting + fs.readfileSync().split('\n') instead of globby all tests behaved as before, no sweat.

Anyway, that's where I chose to file an issue this time around. And here we are. 😅

Anyway, I hope some poor bugger will find some useful help in all this.


Which changes this to maybe a choice:

a. make globby cope with backslash paths (or at least warn about them entering the premises, maybe?) despite \ being a (ahem) legal filename character on UNIX. (Note the K&P quote in https://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names for extra fun.)
b. add a bit to the documentation about Windows folks being extra special careful with them path babies.

(Incidentally, I bet the other globber libs out there suffer the same issue. I seem to recall some similar struggle with node-glob or what-was-it-I-used-back-in-the-day?)

@AndrewLeedham
Copy link

@GerHobbelt @phawxby I believe this was deliberate and was flagged as a breaking change in the v10.0.0 release, as fast-glob changed this behaviour in their v3.0.0 release.

Fast-globs proposed workaround is .replace(/\\/g, '//') https://github.com/micromatch/micromatch#backslashes. So I don't see this being fixed in globby as it will break special character escaping.

@SrBrahma
Copy link

SrBrahma commented Jun 6, 2021

I am only using globby to get all the deep filenames from a given cwd (so the results won't contain the parents dirs). The glob pattern is just ''. Do I need to care about Windows/Posix fs? Because it says the cwd defaults to process.cwd(), and it on Windows, will contain backslashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants