build: run clang-format on CI

[RaisinTen]

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 [email protected]

[nodejs-github-bot]

Review requested:

• @nodejs/actions

[Trott]

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]

Doesn't it only affect the changed lines?

[RaisinTen]

Doesn't it only affect the changed lines?

Yes, that's the intent.

[Trott]

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]

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]

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]

Is that what you wanted?

That works for me.

[RaisinTen]

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

[tniessen]

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]

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

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

[RaisinTen]

@tniessen here's the fix - #42764.