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] Use clang-format from bzlmod #22504

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 21, 2025

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 Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for feature review of the code parts, please (not necessarily the doc/** parts, though you are welcome to skim that too). After we think the code is okay, I'll seek out individual feature reviewers for each of the IDEs we need to manually re-test.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: 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?

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a 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.

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a 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!

@jwnimmer-tri jwnimmer-tri force-pushed the clang-format-from-bzlmod branch from 0cbf7c8 to 06bbcec Compare January 22, 2025 23:59
Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a 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
Copy link
Contributor

+@SeanCurtis-TRI evaluating vs code instructions.

@SeanCurtis-TRI SeanCurtis-TRI self-assigned this Jan 23, 2025
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

:LGTM: 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.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @jwnimmer-tri)


doc/_pages/vscode.md line 32 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW You might consider specifically calling out setting it in the Workspace settings instead of the user settings.

Sorry, I don't know what that means.

I've opened the branch to pushes, maybe you could push the new text?

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: labeled "do not merge" (waiting on @jwnimmer-tri)


doc/_pages/vscode.md line 32 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Sorry, I don't know what that means.

I've opened the branch to pushes, maybe you could push the new text?

Ah, vscode settings come in two kinds. One is user-level and one is workspace-level. Because this change is appropriate to Drake, it would make sense to set this value in the workspace settings for Drake. Although, it's probably harmless to set it in user settings as long as you're content to apply clang-format-15 to all projects.

It's probably not of great significance.

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.
@jwnimmer-tri jwnimmer-tri force-pushed the clang-format-from-bzlmod branch from 06bbcec to d44ad26 Compare January 28, 2025 20:13
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

:lgtm: the revised External Tool instructions for CLion work well

Reviewed 9 of 15 files at r1, 2 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: labeled "do not merge" (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri merged commit a85f4fb into RobotLocomotion:master Jan 28, 2025
8 of 9 checks passed
@jwnimmer-tri jwnimmer-tri deleted the clang-format-from-bzlmod branch January 28, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants