Tweak esignore #16372

refack · 2017-10-21T23:48:43Z

Fixes issue with unicode paths not being ignored by eslint
b485bc4 solves what #16010 tried to acomplish
The second commit solves it at the root

Refs: #16010
Refs: #16278

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tools,test

refack · 2017-10-21T23:49:46Z

/cc @Trott @chrisbudy @nodejs/testing

Trott · 2017-10-22T01:11:20Z

First commit seems fine. Not clear on how/why the second commit. Can you elaborate a bit?

refack · 2017-10-22T01:49:28Z

The original bug is caused by

node/tools/eslint/node_modules/ignore/ignore.js

Line 416 in 8e268c7

return (/^\\\\\?\\/.test(str) || /[^\x00-\x80]+/.test(str) ? str : str.replace(/\\/g, '/')

in ignore. This RegExp fails when there are non ASCII characters in the path. so it fails to "posix"ise the path
ignore slices the path by / then test east slice for a match exclusion with the exclusion rules
eslint has a default ignore rule to exclude any path starting with . so if the temp directories we create start with dot we don't need an extra rule, and it's not slice dependant.

Trott · 2017-10-22T01:59:10Z

@refack Thanks for the clear explanation. Does the bug only manifest on Windows? I've not seen it anywhere else... EDIT: Never mind, I see that the buggy code is indeed Windows-specific.

Trott

LGTM if CI is green. I dislike using .tmp instead of tmp but that's an aesthetic decision, and working around bugs is more important than accommodating my aesthetic preferences.

refack · 2017-10-22T02:11:05Z

The bug manifests only on Windows because the step that is sensitive to unicode (or UTF-8) is the conversion from \ to /.

Trott · 2017-10-22T11:24:46Z

I guess it makes sense to leave test/tmp* in .gitignore because those directories will still be hanging around after this commit lands in someone's clone.

Maybe it makes sense to add test/tmp* (and test/.tmp* because why not?) to the clean target in Makefile and vcbuild.bat so that there's a chance of removal of test/tmp* from .gitignore eventually? Could be a separate PR, especially if we think a fix might be coming to ignore.js and people might actively dislike the change to .tmp for whatever reason.

refack · 2017-10-22T14:16:51Z

Maybe it makes sense to add test/tmp* (and test/.tmp* because why not?) to the clean target in Makefile and vcbuild.bat so that there's a chance of removal of test/tmp* from .gitignore eventually? Could be a separate PR, especially if we think a fix might be coming to ignore.js and people might actively dislike the change to .tmp for whatever reason.

I'm a big believer in git clean -Xfd (Capital X to remove ignored files), but for those who are used to make targets will do...

refack · 2017-10-22T14:17:27Z

I guess it makes sense to leave test/tmp* in .gitignore because those directories will still be hanging around after this commit lands in someone's clone.

P.S. a .gitignore cleanup/refactor in the works...

refack · 2017-10-22T14:47:41Z

RE the third commit (25ff3343d326c4295f580db32e7d0c7b5a1a8127) I'm doing it semi blind, so would appreciate some extra manual validation.
Decided to un-document the vcbuild.bat command since it seems vestigial (does very little actual cleaning), and only relevant for release build scenarios.

Trott · 2017-10-22T15:13:22Z

Makefile

@@ -102,11 +102,14 @@ uninstall:

 clean:
 	$(RM) -r out/Makefile $(NODE_EXE) $(NODE_G_EXE) out/$(BUILDTYPE)/$(NODE_EXE) \
-                out/$(BUILDTYPE)/node.exp
+	out/$(BUILDTYPE)/node.exp


I think the indentation in the current Makefile makes more sense because this is a continuation of the previous line and not a new command.

gibfahn · 2017-10-22T17:44:47Z

test/common/index.js

@@ -38,7 +38,7 @@ const noop = () => {};

 exports.fixturesDir = fixturesDir;

-exports.tmpDirName = 'tmp';
+exports.tmpDirName = '.tmp';


Could you add a comment? Maybe something like:

// Eslint ignores directories beginning with '.' by default.

gibfahn · 2017-10-22T17:48:19Z

Makefile

 	@if [ -d out ]; then find out/ -name '*.o' -o -name '*.a' -o -name '*.d' | xargs $(RM) -r; fi
 	$(RM) -r node_modules
 	@if [ -d deps/icu ]; then echo deleting deps/icu; $(RM) -r deps/icu; fi
 	$(RM) test.tap
+	$(RM) -r test/tmp*


I'm not sure we need this, if we never had it before, then why not just stick with the new one? (partly a question for @Trott )

If we are adding it (and it's already legacy) then could you add a comment saying # Remove this at some point

gibfahn · 2017-10-22T17:56:51Z

vcbuild.bat

@@ -541,7 +541,7 @@ echo Failed to create vc project files.
 goto exit

 :help
-echo vcbuild.bat [debug/release] [msi] [test/test-ci/test-all/test-uv/test-internet/test-pummel/test-simple/test-message/test-async-hooks/test-v8/test-v8-intl/test-v8-benchmarks/test-v8-all] [clean] [noprojgen] [small-icu/full-icu/without-intl] [nobuild] [sign] [x86/x64] [vs2015/vs2017] [download-all] [enable-vtune] [lint/lint-ci] [no-NODE-OPTIONS] [link-module path-to-module]
+echo vcbuild.bat [debug/release] [msi] [test/test-ci/test-all/test-uv/test-internet/test-pummel/test-simple/test-message/test-async-hooks/test-v8/test-v8-intl/test-v8-benchmarks/test-v8-all] [noprojgen] [small-icu/full-icu/without-intl] [nobuild] [sign] [x86/x64] [vs2015/vs2017] [download-all] [enable-vtune] [lint/lint-ci] [no-NODE-OPTIONS] [link-module path-to-module]


So this is only used in a few places right?

node/vcbuild.bat

Lines 171 to 177 in 7a171fd

if "%target%"=="Clean" echo deleting %~dp0deps\icu

if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu

:no-depsicu

call :getnodeversion || exit /b 1

if "%target%"=="Clean" rmdir /Q /S "%~dp0%config%\node-v%FULLVERSION%-win-%target_arch%" > nul 2> nul

Agreed that it doesn't look like a user-facing thing.

Might have been once... Now it seems more like clean-release-artifacts. So, confusing.

gibfahn · 2017-10-22T17:58:23Z

.eslintignore

@@ -1,10 +1,9 @@
 lib/internal/v8_prof_polyfill.js
 lib/punycode.js
-test/addons/??_*/
+test/addons/??_*


So how exactly does this make the ignoring more correct?

It's an ignore thing, it builds the exclusion regex based on this line (requiring a terminal /), then the directory itself is matched as test/addons/12_XFD (with no trailing /) so it passes and all it's children are enumerated (only they fail).
So the line pre PR is equivalent to test/addons/??_*/* and test/addons/??_* make it exclude the directories and not even enumerate their content.

gibfahn · 2017-10-22T22:15:47Z

test/common/index.js

@@ -38,7 +38,9 @@ const noop = () => {};

 exports.fixturesDir = fixturesDir;

-exports.tmpDirName = 'tmp';
+// Using a `.` prefixed name, which is the convention for "hidden" on POSIX,
+// get tools to ignore it by default, especially git and eslint.


Git doesn't ignore it by default, so maybe remove git and

Makes eslint exclude directories without enumerating their content PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs#16372 Refs: nodejs#16010 Refs: nodejs#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Makes eslint exclude directories without enumerating their content PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: #16372 Refs: #16010 Refs: #16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-11-16T21:13:29Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

This should likely be part of a batch update to eslint

Makes eslint exclude directories without enumerating their content PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

`.tmp` prefix allows easier exclusion PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

also undocument the `vcbuild.bat` command since it's broken and seems to only be relevant to release builds PR-URL: nodejs/node#16372 Refs: nodejs/node#16010 Refs: nodejs/node#16278 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 21, 2017

refack added test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Oct 21, 2017

Trott approved these changes Oct 22, 2017

View reviewed changes

joyeecheung approved these changes Oct 22, 2017

View reviewed changes

lpinca approved these changes Oct 22, 2017

View reviewed changes

gibfahn approved these changes Oct 22, 2017

View reviewed changes

Trott reviewed Oct 22, 2017

View reviewed changes

gibfahn reviewed Oct 22, 2017

View reviewed changes

jasnell approved these changes Oct 23, 2017

View reviewed changes

refack added 3 commits October 26, 2017 18:38

refack force-pushed the tweak-esignore branch from d86197f to 9ab6481 Compare October 26, 2017 22:39

refack merged commit 9ab6481 into nodejs:master Oct 26, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

Trott mentioned this pull request Nov 14, 2017

tools: make lint-js should not lint test/tmp* #17029

Closed

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

refack deleted the tweak-esignore branch April 26, 2018 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak esignore #16372

Tweak esignore #16372

refack commented Oct 21, 2017 •

edited

Loading

refack commented Oct 21, 2017

Trott commented Oct 22, 2017

refack commented Oct 22, 2017

Trott commented Oct 22, 2017 •

edited

Loading

Trott left a comment

refack commented Oct 22, 2017

Trott commented Oct 22, 2017

refack commented Oct 22, 2017 •

edited

Loading

refack commented Oct 22, 2017

refack commented Oct 22, 2017

Trott Oct 22, 2017

gibfahn Oct 22, 2017

gibfahn Oct 22, 2017

gibfahn Oct 22, 2017

refack Oct 22, 2017

gibfahn Oct 22, 2017

refack Oct 22, 2017

gibfahn Oct 22, 2017

MylesBorins commented Nov 16, 2017

	if "%target%"=="Clean" echo deleting %~dp0deps\icu
	if "%target%"=="Clean" rmdir /S /Q %~dp0deps\icu
	:no-depsicu

	call :getnodeversion \|\| exit /b 1

	if "%target%"=="Clean" rmdir /Q /S "%~dp0%config%\node-v%FULLVERSION%-win-%target_arch%" > nul 2> nul

Tweak esignore #16372

Tweak esignore #16372

Conversation

refack commented Oct 21, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Oct 21, 2017

Trott commented Oct 22, 2017

refack commented Oct 22, 2017

Trott commented Oct 22, 2017 • edited Loading

Trott left a comment

Choose a reason for hiding this comment

refack commented Oct 22, 2017

Trott commented Oct 22, 2017

refack commented Oct 22, 2017 • edited Loading

refack commented Oct 22, 2017

refack commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Nov 16, 2017

refack commented Oct 21, 2017 •

edited

Loading

Trott commented Oct 22, 2017 •

edited

Loading

refack commented Oct 22, 2017 •

edited

Loading