-
Notifications
You must be signed in to change notification settings - Fork 543
CXX-3198 upgrade to ClangFormat 19 and replace clang_format.py with clang-format-all.sh #1299
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
Conversation
Do we not need any of clang_format.py's features? It had some interesting support for integrating with git, but if nobody uses that I guess we don't need it. It also ran formatting in parallel, which we could do using xargs in the new script. Do we have a security policy about which external https certificates should be trusted for arbitrary remote code execution? |
AFAIK only the format, lint, and "find and download the exact Clang binary version" features were being actively used. Git-awareness is primarily for the purpose of finding and filtering the files within the repository to be sorted; simply listing the directories/files to be sorted is a far simpler and straightforward approach (and does not lead to formatting issues with untracked/missing source files).
This could be done, but I've opted not to do this to avoid overlapping format warnings in EVG output. Preventing overlapping would require some type of job scheduler such as GNU Parallel which is not readily available on EVG distros. This script is primarily for use by the EVG config; local dev tooling and scripts may be used to reduce and select the files that need to be reformatted (e.g.
I believe this is handled by DEVPROD when configuring EVG distros and their installed/available system certificates. Nevertheless, filed DEVPROD-13413 requesting preinstallation of Astral UV in EVG distros so the curl+sh pattern can be removed in the future. |
After further investigation, there doesn't appear to be a significant benefit to running ClangFormat in parallel vs. passing all source files to ClangFormat. Using Single command with all files:
Command per file with max parallelism:
Command per file, no parallelism:
Unless there is a flaw I overlooked in my benchmarking method, this seems to suggests using |
If I understand correctly my feedback is still desired here and I was just removed above because I was out sick; if I misunderstood, please disregard. The extra tests and justifications around the simpler clang-format invocation sound good and I appreciate the clarification. Re executing code over https, I personally have reservations about giving a 3rd party key infrastructure this much power, but if it's not against policy I can approve the change. |
To alleviate concerns regarding the Note This means the uv binary version is pinned to 0.5.9. AFAICT Furthermore, the script is manually patched with the SHA256 checksum values for the 0.5.9 archives (manually obtained from the release assets) and unconditionally validated with each download by setting An incorrect checksum is reported as follows (triggered locally by manually setting the last digit to
The absence of "no checksums to verify" (from the original patch) in the script output indicates the checksum is being validated. Latest changes verified by this patch. |
Nice! I appreciate the much reduced opportunity for tampering, thanks :) |
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.
LGTM. One remark.
class InstallUV(Function): | ||
name = 'install-uv' | ||
commands = bash_exec( | ||
command_type=EvgCommandType.SETUP, | ||
working_dir='mongo-cxx-driver', | ||
script='.evergreen/scripts/uv-installer.sh' | ||
) |
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.
Can this be modified to install uv
on a cache/scratch space specific to mongo-cxx-driver
and the desired uv
version?
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.
Added a new set-cache-dir
EVG function which defines a MONGO_CXX_DRIVER_CACHE_DIR
EVG expansion. This expansion points to a dedicated cache directory for the C++ Driver (e.g. ~/.cache/mongo-cxx-driver
or equivalent). uv
is now installed into this cache directory in the uv-0.5.9
(versioned) subdirectory and also defines a UV_INSTALL_DIR
EVG expansion for subsequent use in EVG scripts.
downloading uv 0.5.9 x86_64-unknown-linux-gnu
installing to /home/ubuntu/.cache/mongo-cxx-driver/uv-0.5.9
uv
uvx
everything's installed!
Summary
Resolves CXX-3198. Verified by this patch (containing the proposed changes in #1298).
Replaces the convoluted
clang_format.py
script with aclang-format-all.sh
script that mirrors the one being used by the C Driver.Unlike the C Driver, this PR proposes using Astral UV to obtain the clang-format binary used by this script. This is the first use of Astral UV in the Evergreen config since its introduction in #1244. All the complexity of obtaining the clang-format binary and verifying its version is delegated to Astral UV. The script only needs to be executed using the command:
The current latest ClangFormat version provided by the
clang-format
package and used by this PR is ClangFormat 19.1.4 (see corresponding docs). A big upgrade from ClangFormat 7.0.1! 🎉ClangFormat Options
The
.clang-format
file is updated with the latest configuration options usingclang-format --dump-config --style=Google
. The Google style is used as the base style for consistency with the former optionBasedOnStyle: Google
. Old settings were then imported as-is into the new config and the contents cleaned up (de-Googled).Some minor tweaks were made to the config file to take advantage of new options available and improve formatting behavior to better match our existing coding style:
QualifierAlignment: Left
to enforce consistent const style (const T
). (Although personally I would much prefer usingT const
...)PenaltyBreakScopeResolution
was increased to avoid breaking on namespace scope resolution operators.The only required manual modifications to source code were disabling ClangFormat in the X macro headers (to avoid treating
BSONCXX_ENUM
like an attribute macro) and an incorrect fix applied to thentop
function (std::uint8_t const* src
(original) ->std::const uint8_t* src
(wrong) ->const std::uint8_t* src
(correct)).This PR focuses on the ClangFormat and EVG config upgrades. More major changes to the configuration options are deferred to subsequent PRs for separate and more targeted review.