build: add cpp linting to windows build #11856

liusy182 · 2017-03-15T03:18:38Z

This PR adds cpp linting to windows build script. After this change,
running command vcbuild lint will run both cpp linting and javascript
linting on a windows machine.

Fixes: #11816

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. Fixes: nodejs#11816

mscdex · 2017-03-15T03:28:03Z

vcbuild.bat

@@ -362,6 +407,7 @@ echo running jslint-ci
 %config%\node tools\jslint.js -J -f tap -o test-eslint.tap benchmark lib test tools
 goto exit

+


Unnecessary whitespace change

Thanks I will correct this.

jasnell

LGTM with the nit addressed

liusy182 · 2017-03-15T05:20:31Z

I am actually seeing linting failures on my machine when following steps in CONTRIBUTING.md.

More specifically, the failure is regarding header_guard where the full file path is used for comparison. For example, for file "C:\sample\node\src\async-wrap.h", linter is expecting #ifndef SAMPLE_NODE_SRC_ASYNC_WRAP_H_ instead of #ifndef SRC_ASYNC_WRAP_INL_H_. This is because _root is empty on my machine so this statement is not executed.

More of a question, I am wondering if specifying a --root when calling cpplint.py is required. I do not have a Mac at hand so I am not sure how linting could pass on Mac.

bnoordhuis · 2017-03-15T08:03:20Z

@liusy182 We float a patch (fadf66a) that computes the root from the absolute path to tools/cpplint.py. It probably needs some tweaks for Windows paths.

liusy182 · 2017-03-16T07:37:02Z

thanks @bnoordhuis 👍 I changed file path comparison to use forward slash and it is now running fine.

bnoordhuis

Thanks, LGTM with a nit. CI: https://ci.nodejs.org/job/node-test-pull-request/6884/

bnoordhuis · 2017-03-16T08:23:14Z

tools/cpplint.py

@@ -1074,7 +1074,8 @@ 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__), '..'))
+    toplevel = os.path.abspath(
+      os.path.join(os.path.dirname(__file__), '..')).replace('\\', '/')


Tiniest of nits: can you indent by four spaces?

This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. PR-URL: #11856 Fixes: #11816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell · 2017-03-17T16:26:42Z

Landed in 379eec3.

gibfahn · 2017-03-17T19:49:04Z

@liusy182 So at the moment your Git author name and email address are set to:

liusi <siyuan.liu@autodesk.com>

People usually choose to use their full names for commits. To set your name globally (if you want to) you can do:

git config --global user.name "Siyuan Liu"

Just FYI.

liusy182 · 2017-03-18T00:02:46Z

thanks @gibfahn I will change that in future.

gibfahn · 2017-03-18T10:24:24Z

@liusy182 also your email address isn't associated with your GitHub account, so you probably want to either change your email address (see below), or add it as an alternative in your GitHub email settings.

git config --global user.email "ss_161091@163.com"

Otherwise you get "Unrecognised author" in the commit log.

This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. PR-URL: nodejs#11856 Fixes: nodejs#11816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

refack

Missing documenting new options

refack · 2017-03-21T17:10:52Z

vcbuild.bat

 if /i "%1"=="test-known-issues" set test_args=%test_args% known_issues&goto arg-ok
 if /i "%1"=="test-node-inspect" set test_node_inspect=1&goto arg-ok
 if /i "%1"=="jslint"        set jslint=1&goto arg-ok
 if /i "%1"=="jslint-ci"     set jslint_ci=1&goto arg-ok
+if /i "%1"=="lint"          set cpplint=1&set jslint=1&goto arg-ok
+if /i "%1"=="lint-ci"       set cpplint=1&set jslint_ci=1&goto arg-ok
 if /i "%1"=="package"       set package=1&goto arg-ok


No documented in line 420 :(

Addressed in #12278

* enable eslint to run even in a "clean" workspace * small improvement in performance by reducing number of calls to `findstr` * Document [jslint/jslint-ci] nodejs#11856 (comment)

MylesBorins · 2017-04-18T17:18:04Z

should we backport to v6.x?

edit: we should land with #11992 if we do

* enable eslint to run even in a "clean" workspace * small improvement in performance by reducing number of calls to `findstr` * Document [jslint/jslint-ci] nodejs#11856 (comment)

This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. PR-URL: nodejs#11856 Fixes: nodejs#11816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. Backport-PR-URL: #14879 PR-URL: #11856 Fixes: #11816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

build: add cpp linting to windows build

3f6c5a9

This PR adds cpp linting to windows build script. After this change, running command `vcbuild lint` will run both cpp linting and javascript linting on a windows machine. Fixes: nodejs#11816

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Mar 15, 2017

mscdex added windows Issues and PRs related to the Windows platform. and removed doc Issues and PRs related to the documentations. labels Mar 15, 2017

mscdex reviewed Mar 15, 2017

View reviewed changes

jasnell approved these changes Mar 15, 2017

View reviewed changes

remove accidentally added new line

15ae7c0

make file path comparison work for windows machine as well.

3822b82

bnoordhuis approved these changes Mar 16, 2017

View reviewed changes

indent by 4 spaces instead.

1743f76

gibfahn mentioned this pull request Mar 16, 2017

test/arm reported as failure when it passed for a PR nodejs/build#572

Closed

jasnell closed this Mar 17, 2017

italoacasas mentioned this pull request Mar 20, 2017

v7.7.4 Release Proposal #11941

Merged

refack reviewed Mar 21, 2017

View reviewed changes

refack mentioned this pull request Mar 21, 2017

new lint option on windows is missing documentation #11971

Closed

refack mentioned this pull request Apr 7, 2017

build,windows: clean lint paths in vcbuild.bat #12278

Closed

2 tasks

MylesBorins added the lts-watch-v6.x label Apr 18, 2017

refack mentioned this pull request May 15, 2017

doc: adds options for cpp linting to help section of Windows build script #11992

Closed

3 tasks

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 17, 2017

kfarnung mentioned this pull request Aug 16, 2017

[v6.x backport] build: enable cpplint on windows #14879

Closed

2 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Sep 19, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add cpp linting to windows build #11856

build: add cpp linting to windows build #11856

liusy182 commented Mar 15, 2017

mscdex Mar 15, 2017

liusy182 Mar 15, 2017

jasnell left a comment

liusy182 commented Mar 15, 2017

bnoordhuis commented Mar 15, 2017

liusy182 commented Mar 16, 2017

bnoordhuis left a comment

bnoordhuis Mar 16, 2017

jasnell commented Mar 17, 2017

gibfahn commented Mar 17, 2017 •

edited

liusy182 commented Mar 18, 2017

gibfahn commented Mar 18, 2017

refack left a comment

refack Mar 21, 2017

refack Apr 7, 2017

MylesBorins commented Apr 18, 2017 •

edited

		@@ -362,6 +407,7 @@ echo running jslint-ci
		%config%\node tools\jslint.js -J -f tap -o test-eslint.tap benchmark lib test tools
		goto exit

build: add cpp linting to windows build #11856

build: add cpp linting to windows build #11856

Conversation

liusy182 commented Mar 15, 2017

Checklist

Affected core subsystem(s)

mscdex Mar 15, 2017

Choose a reason for hiding this comment

liusy182 Mar 15, 2017

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

liusy182 commented Mar 15, 2017

bnoordhuis commented Mar 15, 2017

liusy182 commented Mar 16, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Mar 16, 2017

Choose a reason for hiding this comment

jasnell commented Mar 17, 2017

gibfahn commented Mar 17, 2017 • edited

liusy182 commented Mar 18, 2017

gibfahn commented Mar 18, 2017

refack left a comment

Choose a reason for hiding this comment

refack Mar 21, 2017

Choose a reason for hiding this comment

refack Apr 7, 2017

Choose a reason for hiding this comment

MylesBorins commented Apr 18, 2017 • edited

gibfahn commented Mar 17, 2017 •

edited

MylesBorins commented Apr 18, 2017 •

edited