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

build: add cpp linting to windows build #11856

Closed
wants to merge 4 commits into from

Conversation

liusy182
Copy link

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
@nodejs-github-bot 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 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
vcbuild.bat Outdated
@@ -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


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary whitespace change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I will correct this.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the nit addressed

@liusy182
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools/cpplint.py Outdated
@@ -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('\\', '/')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiniest of nits: can you indent by four spaces?

jasnell pushed a commit that referenced this pull request Mar 17, 2017
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
Copy link
Member

jasnell commented Mar 17, 2017

Landed in 379eec3.

@jasnell jasnell closed this Mar 17, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 17, 2017

@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
Copy link
Author

thanks @gibfahn I will change that in future.

@gibfahn
Copy link
Member

gibfahn commented Mar 18, 2017

@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.

image

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
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>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
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>
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documenting new options

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No documented in line 420 :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #12278

refack added a commit to refack/node that referenced this pull request Apr 8, 2017
* 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
Copy link
Contributor

MylesBorins commented Apr 18, 2017

should we backport to v6.x?

edit: we should land with #11992 if we do

refack added a commit to refack/node that referenced this pull request Apr 25, 2017
* 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)
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
kfarnung pushed a commit to kfarnung/node that referenced this pull request Aug 16, 2017
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>
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants