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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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!

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

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) status: do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants