Rewrite paths for globby to use forward slash #5386

tbowmo · 2021-07-07T15:50:19Z

Fixes windows paths for globby, by replacing backward slash with forward slashes

No, it's self-explanatory.

woutervanvliet · 2021-07-08T08:08:52Z

lib/standalone.js

 		if (fs.existsSync(absolutePath)) {
 			// This path points to a file. Return an escaped path to avoid globbing
 			return fastGlob.escapePath(entry);
 		}

+		// Flip backward slash to forward slash (windows compatability)


Runaway comment

removed it..

woutervanvliet · 2021-07-08T08:09:18Z

lib/standalone.js

@@ -175,11 +175,14 @@ module.exports = function (options) {
 		const cwd = _.get(globbyOptions, 'cwd', process.cwd());
 		const absolutePath = !path.isAbsolute(entry) ? path.join(cwd, entry) : path.normalize(entry);

+		entry = entry.replace(/\\(?![[\]()])/g, '/');


Perhaps you can add a unit test on this behaviour, to make sure it doesn't break in the future

Added unit test for windows style backslash in paths

woutervanvliet

LGTM

ybiquitous

@tbowmo Thanks for creating this PR. Could you consider the following suggestions, please?

Can you change the base branch to v14?
We have pushed many commits for the next version 14.0.0. (see Release 14.0.0 #5205)
According to the FAQ on fast-glob, which is a base package of globby, it seems better to me to use normalize-path because it is well-tested. What do you think?

📖 Use the normalize-path or the unixify package to convert Windows-style path to a Unix-style path.

tbowmo · 2021-07-12T12:48:54Z

@ybiquitous I'll move the changes to the v14 branch instead.. And also have a look at normalize-path.. It was a quick solution that was made in the first place (that is working..)

tbowmo · 2021-07-12T13:19:42Z

@ybiquitous I tried to use normalize-path, as suggested. (just replaced the regex that I added in standalone.js, with the following:

---		entry = entry = entry.replace(/\\(?![[\]()])/g, '/');
+++		entry = normalize(entry);

And added the required dependencies.. However, this breaks one of the tests:
● standalone globbing › mixed globs and paths with special chars › manual escaping
No files matching the pattern "/home/thomas/gits/stylelint/lib/tests/fixtures/globs/got/[braces/] and /(spaces/)/*.+(s|c)ss" were found.

tbowmo · 2021-07-12T13:22:50Z

btw. what is the timeframe for releasing v14? (I think @woutervanvliet is eager to get this fix into "production" as the current behavior is breaking a couple of developers machines, as they insist running windows only)

ybiquitous · 2021-07-12T14:53:08Z

@tbowmo Sorry for the late reply. My answers are as below.

#5386 (comment)

I think we should convert back-slashes (\) to forward-slashes (/) only when actual file paths are specified (not globs) because:

fast-glob doesn’t convert back-slashes
the back-slashes problem on globs is difficult and complex, according to the micromatch notes
(fast-glob uses internally micromatch)

So hers is my suggestion code, fixing also the test case:

diff --git a/lib/__tests__/standalone-globs.test.js b/lib/__tests__/standalone-globs.test.js
index 8254794d..ed28cd79 100644
--- a/lib/__tests__/standalone-globs.test.js
+++ b/lib/__tests__/standalone-globs.test.js
@@ -113,8 +113,11 @@ describe('standalone globbing', () => {
 	});
 
 	describe('windows path style with backslash', () => {
+		// The tests fail on non-windows environments.
+		if (process.platform !== 'win32') { return }
+
 		it('should handle normal backslash in glob pattern', async () => {
-			const cssGlob = `${fixturesPath}\\with spaces\\*.+(s|c)ss`;
+			const cssGlob = path.win32.join(fixturesPath, "globs", "with spaces", "styles.css");
 
 			const { results } = await standalone({
 				files: cssGlob,
diff --git a/lib/standalone.js b/lib/standalone.js
index 385d7b33..87b672b1 100644
--- a/lib/standalone.js
+++ b/lib/standalone.js
@@ -19,6 +19,7 @@ const { default: ignore } = require('ignore');
 const DEFAULT_IGNORE_FILENAME = '.stylelintignore';
 const FILE_NOT_FOUND_ERROR_CODE = 'ENOENT';
 const ALWAYS_IGNORED_GLOBS = ['**/node_modules/**'];
+const normalizePath = require('normalize-path');
 const writeFileAtomic = require('write-file-atomic');
 
 /** @typedef {import('stylelint').StylelintStandaloneOptions} StylelintStandaloneOptions */
@@ -174,11 +175,9 @@ module.exports = function (options) {
 		const cwd = (globbyOptions && globbyOptions.cwd) || process.cwd();
 		const absolutePath = !path.isAbsolute(entry) ? path.join(cwd, entry) : path.normalize(entry);
 
-		entry = entry.replace(/\\(?![[\]()])/g, '/');
-
 		if (fs.existsSync(absolutePath)) {
 			// This path points to a file. Return an escaped path to avoid globbing
-			return fastGlob.escapePath(entry);
+			return fastGlob.escapePath(normalizePath(entry));
 		}
 
 		return entry;

Run normalizePath() only when specified paths exists on a file system
Run the test only for a windows environment

#5386 (comment)

As you see the v14.0.0 issue, we have a lot of tasks to release v14.0.0. So I cannot promise the release date, unfortunately. 😞

Perhaps, I think there are the following workarounds for this problem:

Use your forked version based on this PR’s change, such as "stylelint": "your-github-orb/stylelint#fix-win-path-problem"
Specify unix-like paths, such as c:/dev/myfile.scss instead of c:\dev\myfile.scss

tbowmo · 2021-07-12T15:07:12Z

I'm not that big a fan of making conditions for when tests are run.. If the majority of devs are using unix like systems, then we never will run the tests for windows platform, unless you run the tests in the CI pipeline somehow, and then we can break the functionality for windows (like it is right now with the latest 13.x release)

I'll re-think testing for the windows paths when I get a bit more headroom (@woutervanvliet you usually have a couple of hints as well ;))

tbowmo · 2021-07-12T15:15:44Z

The above changes needs to be tested on a windows only system (no wsl), which I do not have access to at the moment (work pc is shutdown for 3 weeks of summer holidays)

tbowmo · 2021-07-12T21:13:20Z

I fired up a win10 in virtualbox, for a quick spin of the tests, which unfortunately fails (on the windows path tests).. So I'll look into making a more permanent test setup on my tinkering machine, while debugging / testing this windows issue..

ybiquitous · 2021-07-12T23:06:49Z

Yes, I also agree that we should avoid environment-specific tests if possible.

tbowmo · 2021-07-13T18:06:15Z

Looked into it, I did the changes requested by @ybiquitous but that also fails when running the tests on windows! The extra test that I wrote, fails as the following bit of code:

	fileList = fileList.map((entry) => {
		const cwd = (globbyOptions && globbyOptions.cwd) || process.cwd();
		const absolutePath = !path.isAbsolute(entry) ? path.join(cwd, entry) : path.normalize(entry);

		if (fs.existsSync(absolutePath)) {
			// This path points to a file. Return an escaped path to avoid globbing
			return fastGlob.escapePath(normalizePath(entry));
		}

		return entry;
	});

will not enter the if clause, as f.existsSync(absolutePath) returns false.

Thereby it doesn't rewrite the entry through normalizePath.

I then rewrote it, so I always throw entry through normalize path, like:

	fileList = fileList.map((entry) => {
		const cwd = (globbyOptions && globbyOptions.cwd) || process.cwd();
		const absolutePath = !path.isAbsolute(entry) ? path.join(cwd, entry) : path.normalize(entry);

		entry = normalizePath(entry);

		if (fs.existsSync(absolutePath)) {
			// This path points to a file. Return an escaped path to avoid globbing
			return fastGlob.escapePath(entry);
		}

		return entry;
	});

but that also fails tests on windows ?!?!?

I then reverted to just use the regular expression as in my initial PR, and then it passes.. That test is passing on both windows and linux hosts.

ybiquitous · 2021-07-14T03:01:59Z

@tbowmo Sorry, my suggestion code (test file path) is bad. globs is needless. 🙇🏼

-const cssGlob = path.win32.join(fixturesPath, 'globs', 'with spaces', 'styles.css');
+const cssGlob = path.win32.join(fixturesPath, 'with spaces', 'styles.css');

EDITED: I think fs.existsSync(absolutePath) should return true.

ybiquitous

Thank you for fixing the test failure (a65f668)!

lib/__tests__/standalone-globs.test.js

ybiquitous

Thanks! LGTM 👍🏼

jeddy3

Thank you!

jeddy3 · 2021-07-14T12:51:34Z

v14 changelog entry added:

Fixed: "No files matching the pattern" when using backslash paths on Windows (#5386).

VikasAgarwal1984 · 2021-11-12T19:56:25Z

lib/standalone.js

 		if (fs.existsSync(absolutePath)) {
 			// This path points to a file. Return an escaped path to avoid globbing
-			return fastGlob.escapePath(entry);
+			return fastGlob.escapePath(normalizePath(entry));


This only works when path is absolute, if applying a pattern like "**/*.scss" that is not absolute, it still fails.
I tried to run with
entry = entry.replace(/\(?![[]()])/g, '/');

it works, I have not read all comments what was the reason to remove it.

entry = normalizePath(entry) needs to be before if (fs.existsSync(absolutePath)) {

@VikasAgarwal1984 this is an old (merged) pr, so if you think something needs changes, you should create a new pr for the changes.

woutervanvliet suggested changes Jul 8, 2021

View reviewed changes

woutervanvliet approved these changes Jul 10, 2021

View reviewed changes

ybiquitous reviewed Jul 12, 2021

View reviewed changes

Thomas Bowman Mørch added 3 commits July 12, 2021 15:00

Rewrite paths for globby to use forward slash

e31f00c

Remove stray comment

9494e17

Extend tests to verify 'windows paths'

bb8868b

tbowmo changed the base branch from master to v14 July 12, 2021 13:03

Use normalize-path for path normalization instead of regex

e9785e5

Amend tests as suggested in PR comments

a65f668

ybiquitous reviewed Jul 14, 2021

View reviewed changes

lib/__tests__/standalone-globs.test.js Outdated Show resolved Hide resolved

Updated test description

07b18e4

ybiquitous approved these changes Jul 14, 2021

View reviewed changes

jeddy3 approved these changes Jul 14, 2021

View reviewed changes

jeddy3 merged commit 336cf15 into stylelint:v14 Jul 14, 2021

jeddy3 mentioned this pull request Jul 14, 2021

"No files matching the pattern" when using Windows #5122

Closed

sync-by-unito bot mentioned this pull request Nov 1, 2021

build(deps-dev): bump stylelint and stylelint-config-standard numbersprotocol/capture-lite#1031

Closed

VikasAgarwal1984 reviewed Nov 12, 2021

View reviewed changes

tbowmo deleted the fixPathsForGlobby branch November 13, 2021 09:24

sync-by-unito bot mentioned this pull request Dec 1, 2021

build(deps-dev): bump stylelint and stylelint-config-standard numbersprotocol/capture-lite#1078

Closed

sync-by-unito bot mentioned this pull request Jan 1, 2022

build(deps-dev): bump stylelint and stylelint-config-standard numbersprotocol/capture-lite#1143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite paths for globby to use forward slash #5386

Rewrite paths for globby to use forward slash #5386

tbowmo commented Jul 7, 2021

woutervanvliet Jul 8, 2021

tbowmo Jul 8, 2021

woutervanvliet Jul 8, 2021

tbowmo Jul 8, 2021

woutervanvliet left a comment

ybiquitous left a comment •

edited

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

ybiquitous commented Jul 12, 2021

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

ybiquitous commented Jul 12, 2021

tbowmo commented Jul 13, 2021

ybiquitous commented Jul 14, 2021 •

edited

ybiquitous left a comment

ybiquitous left a comment

jeddy3 left a comment

jeddy3 commented Jul 14, 2021

VikasAgarwal1984 Nov 12, 2021

VikasAgarwal1984 Nov 12, 2021

tbowmo Nov 13, 2021

Rewrite paths for globby to use forward slash #5386

Rewrite paths for globby to use forward slash #5386

Conversation

tbowmo commented Jul 7, 2021

woutervanvliet Jul 8, 2021

Choose a reason for hiding this comment

tbowmo Jul 8, 2021

Choose a reason for hiding this comment

woutervanvliet Jul 8, 2021

Choose a reason for hiding this comment

tbowmo Jul 8, 2021

Choose a reason for hiding this comment

woutervanvliet left a comment

Choose a reason for hiding this comment

ybiquitous left a comment • edited

Choose a reason for hiding this comment

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

ybiquitous commented Jul 12, 2021

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

tbowmo commented Jul 12, 2021

ybiquitous commented Jul 12, 2021

tbowmo commented Jul 13, 2021

ybiquitous commented Jul 14, 2021 • edited

ybiquitous left a comment

Choose a reason for hiding this comment

ybiquitous left a comment

Choose a reason for hiding this comment

jeddy3 left a comment

Choose a reason for hiding this comment

jeddy3 commented Jul 14, 2021

VikasAgarwal1984 Nov 12, 2021

Choose a reason for hiding this comment

VikasAgarwal1984 Nov 12, 2021

Choose a reason for hiding this comment

tbowmo Nov 13, 2021

Choose a reason for hiding this comment

ybiquitous left a comment •

edited

ybiquitous commented Jul 14, 2021 •

edited