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.

Sign up for GitHub

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

Jump to bottom

build: run cpplint even if jslint failed #12276

Closed
wants to merge 1 commit into from
Closed

build: run cpplint even if jslint failed #12276

wants to merge 1 commit into from

Conversation

krydos
Copy link
Contributor

@krydos krydos commented Apr 7, 2017

As it mentioned in #12082 issue, it would be good to execute cpplint even if jslint failed. As far as I can see it should help to get both lint errors from both checks at once, which is good.

as @mscdex mentioned in the issue, there is vcbuild.bat that should be updated also with something similar, but I have huuuuge troubles with it. Could somebody help me please with this file or point me to the correct direction. I see that I can check the %errorlevel%variable but currently I do not see the way how can I pass it from cpplint to jslint "target".

Thank you

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@krydos
build: run cpplint even if jslint failed
383cc08 
from now lint target should run both linters
even if one of them failed.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 7, 2017
@mscdex mscdex added the tools Issues and PRs related to the tools directory. label Apr 7, 2017
@vsemozhetbyt vsemozhetbyt added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 7, 2017
@vsemozhetbyt
Copy link
Contributor

cc @nodejs/platform-windows

@refack
Copy link
Contributor

refack commented Apr 7, 2017

@krydos yes, batch files are a special kind of hell. but vcbuild.bat does the fall through already, so you're clear.

@refack
Copy link
Contributor

refack commented Apr 7, 2017

but vcbuild.bat does the fall through already, so you're clear.

Actually I'm wrong. I'll PR your PR.

@refack
Copy link
Contributor

refack commented Apr 7, 2017

Actually I'm wrong.

I was right vcbuild does the fall through but it does not exit with ERRORLEVEL > 0 so it won't fail the CI

@aqrln
Copy link
Contributor

aqrln commented Apr 7, 2017 •
edited
Loading

@refack I believe this is intentional, isn't this? The lint target in Unix Makefile isn't run on CI at all from what I see, and we've got linting covered by a separate job (node-test-linter) that node-test-commit invokes. This way we see only compilation and test status for each platform, and lint status separated from those.

/cc @nodejs/build though, as I may be wrong.

@refack
Copy link
Contributor

refack commented Apr 7, 2017

linting covered by a separate job (node-test-linter)

Yea I was thinking that it is the same code, and if it fails on unix then the CI's red.
Cool, so @krydos is exempt of dealing with vcbuild.bat

@krydos
Copy link
Contributor Author

krydos commented Apr 7, 2017

@refack thank you so much for the efforts to check and test it 👍

@refack refack mentioned this pull request Apr 7, 2017
Closed
2 tasks
aqrln
aqrln reviewed Apr 8, 2017
View reviewed changes
Makefile
EXIT_STATUS=0 ; \
$(MAKE) jslint || EXIT_STATUS=$$? ; \
$(MAKE) cpplint || EXIT_STATUS=$$? ; \
exit $$EXIT_STATUS
CONFLICT_RE=^>>>>>>> [0-9A-Fa-f]+|^<<<<<<< [A-Za-z]+
lint-ci: jslint-ci cpplint
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be updated too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aqrln I've updated lint with idea to get all the lint error when I run lint or test locally.
On CI it is enough to know the fact that something (and what exactly) failed. That's why I didn't touch lint-ci. Do you think it should be updated also?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can probably be left alone since CI doesn't show the lint output anyway and as @krydos said, it's enough to let everyone know linting of any kind failed.

@aqrln
Copy link
Contributor

aqrln commented Apr 8, 2017

According to @refack, there's no need to change vcbuild.bat since it already works the way this PR modifies Makefile, so I'll go ahead and remove the "help wanted" label.

@aqrln aqrln removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 8, 2017
aqrln
aqrln approved these changes Apr 8, 2017
View reviewed changes
Copy link
Contributor

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Apr 8, 2017

CI failure are unrelated.

@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

@mscdex does this LGTY?

@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

FWIW, fresh CI: https://ci.nodejs.org/job/node-test-pull-request/7357/

@benjamingr
Copy link
Member

CI green, changes LGTM, landed in 47c255e .

Thanks for your contribution!

@benjamingr benjamingr closed this Apr 14, 2017
benjamingr pushed a commit that referenced this pull request Apr 14, 2017
@krydos
build: run cpplint even if jslint failed
47c255e 
lint target now runs both linters
even if one of them failed.

PR-URL: #12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@krydos krydos deleted the continues-lint branch April 14, 2017 18:54
@mscdex
Copy link
Contributor

mscdex commented Apr 14, 2017

It wasn't mentioned in the commit metadata, but this fixes #12082.

evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@krydos @evanlucas
build: run cpplint even if jslint failed
43e0fd4 
lint target now runs both linters
even if one of them failed.

PR-URL: #12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@krydos @evanlucas
build: run cpplint even if jslint failed
09ee5e5 
lint target now runs both linters
even if one of them failed.

PR-URL: #12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@krydos @evanlucas
build: run cpplint even if jslint failed
4d9e671 
lint target now runs both linters
even if one of them failed.

PR-URL: #12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 15, 2017
@krydos @MylesBorins
build: run cpplint even if jslint failed
1646c6a 
lint target now runs both linters
even if one of them failed.

PR-URL: #12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@krydos @MylesBorins
build: run cpplint even if jslint failed
7c335f6 
lint target now runs both linters
even if one of them failed.

PR-URL: #12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@krydos @andrew749
build: run cpplint even if jslint failed
e81715d 
lint target now runs both linters
even if one of them failed.

PR-URL: nodejs/node#12276
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@aqrln aqrln aqrln approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@krydos @vsemozhetbyt @refack @aqrln @Trott @benjamingr @mscdex @MylesBorins @nodejs-github-bot