-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Use clang-format from bzlmod #22504
base: master
Are you sure you want to change the base?
[build] Use clang-format from bzlmod #22504
Conversation
5c43b28
to
2779587
Compare
2779587
to
0cbf7c8
Compare
+@xuchenhan-tri for feature review of the code parts, please (not necessarily the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on everything outside of doc/
.
Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)
doc/_pages/code_style_tools.md
line 57 at r1 (raw file):
For development on Ubuntu: format a file that has been staged in git
git clang-format --binary=/path/to/drake/bazel-bin/tools/lint/clang-format -- [file name]
BTW, to be more clear, remind people to build it first?
MODULE.bazel
line 78 at r1 (raw file):
# Add additional modules we use as tools (not runtime dependencies). bazel_dep(name = "toolchains_llvm", version = "1.2.0")
BTW, is there a particular reason that we don't use the latest release 1.3.0?
tools/lint/clang_format_lint.py
line 35 at r1 (raw file):
print("ERROR: {} needs clang-format".format(filename)) print("note: fix via {} -style=file -i {}".format( "bazel-bin/tools/lint/clang-format", filename))
BTW, remind people to build it when necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, labeled "do not merge"
MODULE.bazel
line 78 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, is there a particular reason that we don't use the latest release 1.3.0?
Heh. Because it was released 6 hours ago =P.
I'll check to see if it still works for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)
MODULE.bazel
line 78 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Heh. Because it was released 6 hours ago =P.
I'll check to see if it still works for us.
Aha, glad I checked!
Remove clang-format prereqs from the OS. This makes it easier to use an identical version number across all of our supported development OS's, without relying on their various package managers to remain consistent. Note that downstream projects re-using our linters via WORKSPACE will be broken, but that our linters are NOT part of our Stable API.
0cbf7c8
to
06bbcec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)
+@SeanCurtis-TRI evaluating vs code instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slight modification to the vscode docs check out.
Reviewed 1 of 15 files at r1.
Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @jwnimmer-tri)
doc/_pages/vscode.md
line 32 at r2 (raw file):
In the VS Code Options configuration, check that the option for ``C_Cpp: Clang_format_path`` is set to Drake's preferred value ``/path/to/drake/bazel-bin/tools/lint/clang-format``.
BTW You might consider specifically calling out setting it in the Workspace
settings instead of the user settings.
Towards #20731 and #21714 and #14967 and #4843.
For now, the clang-format remains pinned to 15 (no change). In a future PR, I'll aim to upgrade that to the latest and greatest.
This change is