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: upgrade clang-format to v18 #53957

Merged
merged 2 commits into from
Sep 14, 2024
Merged

build: upgrade clang-format to v18 #53957

merged 2 commits into from
Sep 14, 2024

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Jul 19, 2024 •
edited
Loading

Fixes #50157

This PR updates the Clang formatter from the unmaintained clang-format to @magic-akari's amazing @wasm-fmt/clang-format library.

The old formatter was unmaintained and vastly out of date.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 19, 2024
@RedYetiDev RedYetiDev added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. review wanted PRs that need reviews. labels Jul 19, 2024
magic-akari
magic-akari reviewed Jul 20, 2024
View reviewed changes
Copy link

@magic-akari magic-akari left a comment

Choose a reason for hiding this comment

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

It is my honor that you are using @wasm-fmt/clang-format.

I believe that some stylistic differences may arise from the version of LLVM in use.
The current version of @wasm-fmt/clang-format is based on LLVM 16.0.6.

Should you have specific requirements regarding the LLVM version, I am prepared to release the version that meets your needs.

tools/clang-format/clang-format.mjs Outdated Show resolved Hide resolved
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 20, 2024 •
edited
Loading

Oh! I thought it was v19. Nevertheless, it's newer than angular and actively maintained.

No need to prepare anything special, this is only a draft, and I'm not a core collaborator...

I'm curious to see the feedback on this, as I'm hoping that an update of the formatter (this) (along with the linter in another PR) will help keep the CPP code clean and organized for the future.

The stylistic issues are a result of both the linter and LLVM, so I'm hoping that if the linter is updated, at least one of them will be resolved. I'm going to do more testing to see if they are compatible with each other out of the box, but if not I can always configure the linter to work with the formatter.

@RedYetiDev RedYetiDev changed the title build: upgrade clang-format to v19. build: upgrade clang-format to use WASM Jul 20, 2024
@targos
Copy link
Member

targos commented Jul 20, 2024

The second commit is not acceptable. We've been using a clang-format option that only formats the modified lines to avoid a huge change like this one.

@magic-akari
Copy link

The second commit is not acceptable. We've been using a clang-format option that only formats the modified lines to avoid a huge change like this one.

So, we need to support range formats, right?

@RedYetiDev
Copy link
Member Author

I've undone the file changes to make review easier, but that doesn't mean this is ready for review.

@RedYetiDev
Copy link
Member Author

So, we need to support range formats, right?

I'm not an expert, but I think clang supports them in https://github.com/llvm/llvm-project/blob/710dab6e18ad9ed22c2529b9125d7b8813165ede/clang/tools/clang-format/ClangFormat.cpp#L234-L274, does your package have a config that allows them?

@RedYetiDev RedYetiDev changed the title build: upgrade clang-format to use WASM build: upgrade clang-format to v18 Jul 20, 2024
@RedYetiDev
Copy link
Member Author

Okay, I've switched it to a fork which is basically a copy of angular's, except updated to v18. If the WASM one supports the needs, I'm happy to switch it back, this change is (hopefully) only temporary.

@RedYetiDev RedYetiDev marked this pull request as ready for review July 20, 2024 13:45
@RedYetiDev
Copy link
Member Author

Okay, this fork (suprisingly) works well. It only lints the modified files and lines. I'd love to use the WASM version if it gains that support, because who doesn't love WASM, but (at least for now) this fork works!

(Sorry for dragging you around @magic-akari)

@RedYetiDev
Copy link
Member Author

Also, node-core-clang-format is currently at https://github.com/RedYetiDev/node-core-clang-format, but that can always be moved if needed.

@magic-akari
Copy link

So, we need to support range formats, right?

I'm not an expert, but I think clang supports them in https://github.com/llvm/llvm-project/blob/710dab6e18ad9ed22c2529b9125d7b8813165ede/clang/tools/clang-format/ClangFormat.cpp#L234-L274, does your package have a config that allows them?

Not now, but I am very willing to support them.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 20, 2024 •
edited
Loading

Great! If your package supports them, that'd be lovely, but I'd hold off on modifying it for a bit, because this change still needs to be reviewed by the core collaborators.

If this does land, and then your package fits the needs, I'd push for moving node-core-clang-format into the org, and having your package as a dep. but that's waaaaay in the future from this point.

@targos
Copy link
Member

targos commented Jul 20, 2024

fwiw I started working on a fork as well (https://github.com/targos/clang-format/tree/test-build). I just didn't have much time to continue. I think it's important to have an approach where the builds can be trusted. That's why I investigated doing them on GitHub actions.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 20, 2024 •
edited
Loading

Regarding GitHub actions, I came across https://github.com/muttleyxd/clang-tools-static-binaries, which uploads copies of them.
I figured that, if this were to land, it could be automated with something similar.

Although that's a step for the future IMO.

@RedYetiDev
Copy link
Member Author

Sweet! All tests are passing :-)

@magic-akari
Copy link

Starting from version 17.0.6, @wasm-fmt/clang-format supports range formatting.
I plan to incorporate it into my other projects.
Moreover, the versioning will align with LLVM, and I will update with each new LLVM release.

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Jul 21, 2024 •
edited
Loading

Great! I'll try to incorporate it into this PR soon, but I'm quite busy for the rest of this month (sorry!)

Thank you so so much for your development and help to incorporate it, it's truly amazing.

anonrig
anonrig requested changes Jul 22, 2024
View reviewed changes
tools/clang-format/package.json Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev marked this pull request as draft July 22, 2024 00:15
@RedYetiDev RedYetiDev removed c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 1, 2024
@RedYetiDev
Copy link
Member Author

@anonrig are you still blocking?

@marco-ippolito
Copy link
Member

Im ok with this change, but Im not familiar with tooling so I'll defer my review

@RedYetiDev RedYetiDev added request-ci Add this label to start a Jenkins CI on a PR. and removed review wanted PRs that need reviews. labels Sep 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 11, 2024 •
edited
Loading

For the most part, CI LGTM. Some CIs are failing, but there are likely all flakes, as this doesn't change any source code nor tests.

The test team may be interested in the flakes tho.

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

@jasnell It looks like the CI you initiated was stopped, can you redo/resume it?

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

(Aside from the unrelated failure in Linux) CI LGTM

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 13, 2024 •
edited
Loading

  • CI
  • Approvals
  • No outstanding comments

Per the documentation, this should be author ready?

@RedYetiDev RedYetiDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 14, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit c3e1c31 into nodejs:main Sep 14, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c3e1c31

RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@RedYetiDev @RafaelGSS
build: upgrade clang-format to v18
57779ea 
PR-URL: #53957
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@RedYetiDev @RafaelGSS
build: upgrade clang-format to v18
4da42a4 
PR-URL: #53957
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@RedYetiDev @RafaelGSS
build: upgrade clang-format to v18
cd88f9e 
PR-URL: #53957
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@RedYetiDev @RafaelGSS
build: upgrade clang-format to v18
9a07527 
PR-URL: #53957
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 18, 2024
Open
Merged
nodejs-github-bot pushed a commit that referenced this pull request Sep 18, 2024
@legendecas
Revert "build: upgrade clang-format to v18"
7e00be7 
This reverts commit c3e1c31.

PR-URL: #54994
Refs: #53957
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magic-akari magic-akari magic-akari left review comments

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

@marco-ippolito marco-ippolito Awaiting requested review from marco-ippolito

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. 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.

the npm clang-format package is unmaintained and obsolete
8 participants
@RedYetiDev @nodejs-github-bot @targos @magic-akari @marco-ippolito @jasnell @anonrig @lpinca