build: run cpplint even if jslint failed #12276
Conversation
from now lint target should run both linters even if one of them failed.
nodejs-github-bot
added
the
build
vsemozhetbyt
added
the
help wanted
|
cc @nodejs/platform-windows |
|
@krydos yes, batch files are a special kind of hell. but |
Actually I'm wrong. I'll PR your PR. |
I was right |
|
@refack I believe this is intentional, isn't this? The /cc @nodejs/build though, as I may be wrong. |
Yea I was thinking that it is the same code, and if it fails on unix then the CI's red. |
|
@refack thank you so much for the efforts to check and test it 👍 |
| 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 |
There was a problem hiding this comment.
Shouldn't this be updated too?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
|
According to @refack, there's no need to change |
aqrln
removed
the
help wanted
|
CI failure are unrelated. |
|
@mscdex does this LGTY? |
|
FWIW, fresh CI: https://ci.nodejs.org/job/node-test-pull-request/7357/ |
|
CI green, changes LGTM, landed in 47c255e . Thanks for your contribution! |
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>
|
It wasn't mentioned in the commit metadata, but this fixes #12082. |
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>
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>
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>
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>
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>
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>
As it mentioned in #12082 issue, it would be good to execute
cpplinteven ifjslintfailed. 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.batthat 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 fromcpplinttojslint"target".Thank you
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build