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: run clang-format on CI #42681

Merged

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Apr 10, 2022

We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Demo

The diff in acfed40:

diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
index 739b559c3b..4faff290cf 100644
--- a/src/crypto/crypto_context.cc
+++ b/src/crypto/crypto_context.cc
@@ -55,7 +55,7 @@ static bool extra_root_certs_loaded = false;
 BIOPointer LoadBIO(Environment* env, Local<Value> v) {
   HandleScope scope(env->isolate());

-  if (v->IsString()) {
+  if (v-> IsString()) {
     Utf8Value s(env->isolate(), v);
     return NodeBIO::NewFixed(*s, s.length());
   }

triggers the clang-format linter and it fails the CI run with this error: https://github.com/nodejs/node/runs/5960445927?check_suite_focus=true

Formatting C++ diff from 6d4b01774bfaa319e3c7a9f1a3165a4d3641ec84..
changed files:
    src/crypto/crypto_context.cc
diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
index 4faff290cf..739b559c3b 100644
--- a/src/crypto/crypto_context.cc
+++ b/src/crypto/crypto_context.cc
@@ -55,7 +55,7 @@ static bool extra_root_certs_loaded = false;
 BIOPointer LoadBIO(Environment* env, Local<Value> v) {
   HandleScope scope(env->isolate());
 
-  if (v-> IsString()) {
+  if (v->IsString()) {
     Utf8Value s(env->isolate(), v);
     return NodeBIO::NewFixed(*s, s.length());
   }

ERROR: Please run:

  CLANG_FORMAT_START="$(git merge-base HEAD <target-branch-name>)" make format-cpp

to format the commits in your branch.

. Accepting the suggested change in abfa103 produces a green CI - https://github.com/nodejs/node/runs/5960458481?check_suite_focus=true.

Signed-off-by: Darshan Sen raisinten@gmail.com

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Apr 10, 2022
.github/workflows/linters.yml Outdated Show resolved Hide resolved
.github/workflows/linters.yml Outdated Show resolved Hide resolved
.github/workflows/linters.yml Outdated Show resolved Hide resolved
@RaisinTen RaisinTen added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 10, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the build/run-clang-format-on-ci branch from 6c3f55c to acfed40 Compare April 10, 2022 09:50
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@Trott
Copy link
Member

Trott commented Apr 10, 2022

Not a blocking comment, but for reviewers who may not be aware:

  • This will result in failures in many cases where a file is edited because we have a lot of files that have not been run through clang-format. This is probably OK, but is extra friction to be aware of.
  • As of now at least, there are a small number of files where running clang-format results in output that is incompatible with lint-cpp. Also likely manageable, but also extra friction to be aware of.

Refs: #42665 (comment)

@targos
Copy link
Member

targos commented Apr 10, 2022

Doesn't it only affect the changed lines?

@RaisinTen
Copy link
Contributor Author

Doesn't it only affect the changed lines?

Yes, that's the intent.

@Trott
Copy link
Member

Trott commented Apr 10, 2022

Doesn't it only affect the changed lines?

Yes, that's the intent.

I was under the impression that it only affects changed files, but is it actually only the changed lines in those files? If so, I didn't realize it was that granular. That's very helpful in this situation.

@Trott
Copy link
Member

Trott commented Apr 10, 2022

I think the answer is "no" but just to make sure: Will we want to start checking in the node_modules directory for the clang-format tool like we do for ESLint and lint-md so that people without an internet connection can still run the formatter etc.?

@RaisinTen
Copy link
Contributor Author

but is it actually only the changed lines in those files?

Yes, that's right. It was one of the key points that made #21997 a better approach than #16122.

and make clang-format run on C++ diffs instead of the whole code base (this is also what chromium's git-cl does). This allows us to gradually format the code base while we update the C++ files in normal PRs (although it may require support from external tooling and build to make sure people run it before landing PRs).

I think the answer is "no" but just to make sure:

Did you get a chance to go through the demo I edited into the PR description?

Will we want to start checking in the node_modules directory for the clang-format tool like we do for ESLint and lint-md so that people without an internet connection can still run the formatter etc.?

Running make format-cpp when the module is not present in the node_modules directory, already prints this message:

node/Makefile

Lines 1443 to 1444 in 0c57a37

$(info clang-format is not installed.)
$(info To install (requires internet access) run: $$ make format-cpp-build)
. Is that what you wanted?

@Trott
Copy link
Member

Trott commented Apr 11, 2022

Is that what you wanted?

That works for me.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 11, 2022
@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2022
@RaisinTen
Copy link
Contributor Author

Needed another approval because I committed #42681 (comment) after the last review.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2022
@RaisinTen RaisinTen deleted the build/run-clang-format-on-ci branch April 13, 2022 17:33
vmoroz pushed a commit to vmoroz/node that referenced this pull request Apr 13, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@tniessen
Copy link
Member

tniessen commented Apr 17, 2022

Is the failure in #42701 related to this change? The output is not particularly helpful:

Formatting C++ diff from c3a581c360ca1a2a11e49e2e99ca9ea83bbbaaf9..
changed files:
    src/api/environment.cc
    src/env.h
    src/node_wasm_web_api.cc
    src/node_wasm_web_api.h
make: *** [Makefile:1437: format-cpp] Error 1
Error: Process completed with exit code 2.

@RaisinTen
Copy link
Contributor Author

@tniessen You mean in #42701? Hmm, the error looks different from what I got in this PR (as you can see in the description, it is supposed to print the diff and also print the command you can run to fix it locally). 👀
You should be able to fix it by running CLANG_FORMAT_START="$(git merge-base HEAD master)" make format-cpp for now.

@tniessen
Copy link
Member

Oops, yes, that's what I mean :) I did that, seems to pass now.

RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 17, 2022
According to the logs in
nodejs#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: nodejs#42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen
Copy link
Contributor Author

@tniessen here's the fix - #42764.

RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 17, 2022
Since we run the formatter only on PRs that are targetting the master
branch, let's specify that.

Refs: nodejs#42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 17, 2022
Since we run the formatter only on PRs that are targeting the master
branch, let's specify that.

Refs: nodejs#42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 17, 2022
Since we run the formatter only on PRs that are targeting the master
branch, let's specify that.

Refs: nodejs#42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Apr 20, 2022
According to the logs in
#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: #42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
According to the logs in
nodejs#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: nodejs#42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2022
According to the logs in
#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: #42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
According to the logs in
#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: #42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
According to the logs in
#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: #42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
According to the logs in
#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: #42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
According to the logs in
#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: #42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: #42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
We already include the tool inside tools/clang-format, so we should
start using it on CI. This attempts to run the linter only on the
commits present in an opened PR.

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs/node#42681
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
According to the logs in
nodejs/node#42681 (comment),
`make format-cpp` exits with an NZEC. This change intentionally ignores
the error code because it is irrelevant. We already check if the
formatter produced a diff in the next line.

Refs: nodejs/node#42681 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs/node#42764
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants