tools: this relates to #14336, fixes the 'make lint' portion with non-ascii build file path #16047

sharkfisher · 2017-10-06T20:04:49Z

Checklist

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

Affected core subsystem(s)

deps

… make

…ject path contains non-ascii characters.

BridgeAR · 2017-10-06T20:56:21Z

deps/v8/third_party/inspector_protocol/CodeGenerator.py

        if not output_base:
            raise Exception("Base output directory must be specified")
        config_file = arg_options.config
        if not config_file:
            raise Exception("Config file name must be specified")
-        config_base = os.path.dirname(config_file)
+        config_base = os.path.dirname(config_file).decode('utf-8')


No changes should be made to the deps. Those changes should only happen upstream.

Changes to this effect were already made upstream-upstream, btw: https://chromium-review.googlesource.com/c/deps/inspector_protocol/+/574858 … ping @ak239

refack

tools/cpplint.py change LGTM

refack · 2017-10-07T17:03:08Z

Welcome @sharkfisher and thank for this contribution 🥇
The fix to cpplint.py LGTM.
As others have commented, the files in the /deps/ directory are usually "vendored in" from upstream projects. In this case from V8, and as @addaleax commented, a CL was submitted to address this issue, so it should get here after it lands.

refack · 2017-10-07T17:05:14Z

Quick CI check: https://ci.nodejs.org/job/node-test-linter/12296/ ✔️

addaleax · 2017-10-07T17:22:44Z

@refack That CL already landed, it just waits to be rolled into V8

bnoordhuis

Since the upstream change was merged I suppose it makes sense to follow suit but decoding as UTF-8 won't work when the path contains e.g. Latin-1 characters.

bnoordhuis · 2017-10-07T17:21:47Z

tools/cpplint.py

@@ -1075,7 +1075,7 @@ def RepositoryName(self):
    fullname = self.FullName()
    # XXX(bnoordhuis) Expects that cpplint.py lives in the tools/ directory.
    toplevel = os.path.abspath(
-        os.path.join(os.path.dirname(__file__), '..')).replace('\\', '/')
+        os.path.join(os.path.dirname(__file__), '..')).replace('\\', '/').decode('utf-8')


Can you wrap this at 80 columns?

gibfahn · 2017-10-07T18:47:49Z

Since the upstream change was merged I suppose it makes sense to follow suit but decoding as UTF-8 won't work when the path contains e.g. Latin-1 characters.

Were these supported before? This seems like a net positive.

bnoordhuis · 2017-10-07T19:25:39Z

cpplint.py currently is file system encoding-agnostic. The bug in #14336 seems to be primarily CodeGenerator.py mixing byte strings and unicode strings.

stay within 80 cols

sharkfisher · 2017-10-10T22:04:00Z

so I removed changes to CodeGenerator.py since it will be fixed by upstream, and reformatted cpplint.py to stay within 80 cols. So now the only change is to let "make lint" pass when build path has non-ascii chars, it's not directly addressing bug #14336, but is related to it once "make" passes.

BridgeAR · 2017-10-18T14:20:23Z

@bnoordhuis PTAL

bnoordhuis

With the CodeGenerator.py changes gone, I don't think there is much reason to make changes to cpplint.py - it shouldn't need them.

sharkfisher · 2017-10-18T22:11:26Z

CodeGenerator.py change only fixes "make" for me.
"make lint" was still failing until cpplint.py change is in.

bnoordhuis · 2017-10-19T10:41:36Z

What error do you get? As I mentioned earlier, cpplint.py is (or should be) encoding-agnostic.

sharkfisher · 2017-10-19T13:37:11Z

$ pwd
/Users/mzhang/Documents/node/弄得/node

$ make lint
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
	  benchmark doc lib test tools
Running C++ linter...
Traceback (most recent call last):
  File "tools/cpplint.py", line 6094, in <module>
    main()
  File "tools/cpplint.py", line 6087, in main
    ProcessFile(filename, _cpplint_state.verbose_level)
  File "tools/cpplint.py", line 5950, in ProcessFile
    extra_check_functions)
  File "tools/cpplint.py", line 5798, in ProcessFileData
    CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error)
  File "tools/cpplint.py", line 5439, in CheckForIncludeWhatYouUse
    (same_module, common_path) = FilesBelongToSameModule(abs_filename, header)
  File "tools/cpplint.py", line 5333, in FilesBelongToSameModule
    files_belong_to_same_module = filename_cc.endswith(filename_h)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe5 in position 29: ordinal not in range(128)
make[1]: *** [lint-cpp] Error 1
make: *** [lint] Error 2

bnoordhuis

Huh, interesting. It's a bit of a shame that it swaps one encoding issue for another (Latin-1 stops working, UTF-8 starts working) but UTF-8 is probably the most prevalent of the two.

bnoordhuis · 2017-10-19T19:51:45Z

tools/cpplint.py

-    toplevel = os.path.abspath(
-        os.path.join(os.path.dirname(__file__), '..')).replace('\\', '/')
+    toplevel = os.path.abspath(os.path.join(os.path.dirname(__file__), '..')) \
+    .replace('\\', '/').decode('utf-8')


This might be a little easier to read as:

toplevel = os.path.abspath(...) toplevel = toplevel.decode('utf-8')

Trott · 2017-10-27T16:45:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11024/

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

refack · 2017-10-27T21:32:35Z

Landed in 22882d4

@sharkfisher congrats on being promoted from

to

🍾

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

PR-URL: nodejs/node#16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

PR-URL: nodejs/node#16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

deps: this fixes nodejs#14336, non-ascii build file path when running…

1436e10

… make

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 6, 2017

tools: additional fixes for nodejs#14336 to pass ‘make lint’ when pro…

511286e

…ject path contains non-ascii characters.

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

BridgeAR reviewed Oct 6, 2017

View reviewed changes

refack approved these changes Oct 7, 2017

View reviewed changes

refack self-assigned this Oct 7, 2017

bnoordhuis reviewed Oct 7, 2017

View reviewed changes

sharkfisher added 2 commits October 10, 2017 16:48

Update cpplint.py

9a68c5a

stay within 80 cols

Update CodeGenerator.py

20f538e

sharkfisher changed the title ~~deps: this fixes #14336, non-ascii build file path when running make~~ tools: this relates to #14336, fixes the 'make lint' portion with non-ascii build file path Oct 10, 2017

bnoordhuis reviewed Oct 18, 2017

View reviewed changes

bnoordhuis approved these changes Oct 19, 2017

View reviewed changes

refack pushed a commit that referenced this pull request Oct 27, 2017

tools: fix cpplint.py when path contains non-ascii

22882d4

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

refack closed this Oct 27, 2017

jasnell mentioned this pull request Oct 30, 2017

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

Merged

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

tools: fix cpplint.py when path contains non-ascii

6a20849

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

tools: fix cpplint.py when path contains non-ascii

a59aacf

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

gibfahn pushed a commit that referenced this pull request Oct 31, 2017

tools: fix cpplint.py when path contains non-ascii

b1e2a38

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017

tools: fix cpplint.py when path contains non-ascii

f96c83c

PR-URL: nodejs/node#16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017

tools: fix cpplint.py when path contains non-ascii

30cf80c

PR-URL: nodejs/node#16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis mentioned this pull request Nov 4, 2017

docs: warn users about non-ASCII paths on build #16735

Closed

2 tasks

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins pushed a commit that referenced this pull request Nov 16, 2017

tools: fix cpplint.py when path contains non-ascii

bdfc9cd

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

MylesBorins pushed a commit that referenced this pull request Nov 21, 2017

tools: fix cpplint.py when path contains non-ascii

c01adff

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins pushed a commit that referenced this pull request Nov 28, 2017

tools: fix cpplint.py when path contains non-ascii

a8d7f5f

PR-URL: #16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

tools: fix cpplint.py when path contains non-ascii

4a22700

PR-URL: nodejs/node#16047 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

refack removed their assignment Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: this relates to #14336, fixes the 'make lint' portion with non-ascii build file path #16047

tools: this relates to #14336, fixes the 'make lint' portion with non-ascii build file path #16047

sharkfisher commented Oct 6, 2017

BridgeAR Oct 6, 2017

mscdex Oct 6, 2017

addaleax Oct 7, 2017

refack left a comment

refack commented Oct 7, 2017 •

edited

Loading

refack commented Oct 7, 2017 •

edited

Loading

addaleax commented Oct 7, 2017

bnoordhuis left a comment

bnoordhuis Oct 7, 2017

gibfahn commented Oct 7, 2017 •

edited

Loading

bnoordhuis commented Oct 7, 2017

sharkfisher commented Oct 10, 2017

BridgeAR commented Oct 18, 2017

bnoordhuis left a comment

sharkfisher commented Oct 18, 2017

bnoordhuis commented Oct 19, 2017

sharkfisher commented Oct 19, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Oct 19, 2017

Trott commented Oct 27, 2017

refack commented Oct 27, 2017

tools: this relates to #14336, fixes the 'make lint' portion with non-ascii build file path #16047

tools: this relates to #14336, fixes the 'make lint' portion with non-ascii build file path #16047

Conversation

sharkfisher commented Oct 6, 2017

Checklist

Affected core subsystem(s)

BridgeAR Oct 6, 2017

Choose a reason for hiding this comment

mscdex Oct 6, 2017

Choose a reason for hiding this comment

addaleax Oct 7, 2017

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack commented Oct 7, 2017 • edited Loading

refack commented Oct 7, 2017 • edited Loading

addaleax commented Oct 7, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 7, 2017

Choose a reason for hiding this comment

gibfahn commented Oct 7, 2017 • edited Loading

bnoordhuis commented Oct 7, 2017

sharkfisher commented Oct 10, 2017

BridgeAR commented Oct 18, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

sharkfisher commented Oct 18, 2017

bnoordhuis commented Oct 19, 2017

sharkfisher commented Oct 19, 2017 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 19, 2017

Choose a reason for hiding this comment

Trott commented Oct 27, 2017

refack commented Oct 27, 2017

refack commented Oct 7, 2017 •

edited

Loading

refack commented Oct 7, 2017 •

edited

Loading

gibfahn commented Oct 7, 2017 •

edited

Loading

sharkfisher commented Oct 19, 2017 •

edited

Loading