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

Overhaul how we search for clang-format #4888

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

StephanTLavavej
Copy link
Member

Our CMake machinery for clang-format is awesome, but it's remained largely unchanged since being added by #2671 on 2022-05-01. Thanks to @TheStormN for bringing my attention to issues here.

The first problem is that we're looking for the clang-format executable in a hardcoded path. That's easily broken if a contributor has installed VS Preview to a non-default location.

Second, during configuration of the STL build, we're simply warning about clang-format being missing. This isn't really helpful - to be successful, contributors really need to have both Clang installed (for testing) and clang-format (to make all but the most trivial source changes without over-relying on PR checks).

Third, we weren't validating what version of clang-format we're picking up. clang-format routinely changes behavior between different major versions, so it's important for all contributors to use the same version that the PR checks are validating.

I'm still a CMake novice, but I've managed to overhaul how we handle clang-format here.

  • I'm dropping the machinery that verbosely printed "Searching for VS clang-format". We'll still print something at the very end. CMake's own caching machinery will ensure that we run the search only once.
  • I'm dropping the "if project is top-level" logic. We're going to search for clang-format when configuring the STL as a whole, so we'll be ready if the format target is invoked later, and this isn't going to affect whether we emit warnings or errors.
  • Instead of telling CMake find_program to search hardcoded PATHS with no defaults (aside: just NO_DEFAULT_PATH would have been sufficient, everything below was redundant), actually use CMake's default behavior, which will search the environment's PATH (among other things). This is how we expect to pick up cl.exe, clang-cl.exe, etc.
  • Additionally, mark it as REQUIRED. This will emit a hard error if clang-format isn't found. (Tested by temporarily removing LLVM from my PATH.)
  • Then, use CMake execute_process to run clang-format --version and inspect the output.
    • OUTPUT_STRIP_TRAILING_WHITESPACE is necessary to strip newlines.
    • If we don't recognize the output at all, emit a fatal error and print what it was. (Tested by temporarily damaging the expected string.)
    • If we do recognize it, use a regex to extract the version number, and perform version-aware comparisons (this is cool).
    • If we found an older clang-format, there's no excuse - emit a hard error. Print what we found and expected. (Tested by manually adjusting the expected version.)
    • If we found the exact version, then print a status message mentioning it. (Like before, just less verbose.)
    • If we found a newer clang-format, emit a warning mentioning the version numbers (similarly tested). There are a couple of reasons why this might happen - perhaps someone has gotten clever and installed a newer Clang/LLVM than the one that's shipping in VS. Or, VS might have just upgraded its Clang version, but we're in the (typically very brief) window between the release of a new VS Preview and when we merge a toolset update that re-formats the codebase. In this case, a contributor or maintainer with a freshly upgraded VS Preview will be running a newer clang-format than what the repo expects. This shouldn't be a fatal error, but the warning will let the contributor/maintainer know to look out for unexpected formatting diffs and manually deal with them.
  • If our search for files to clang-format doesn't find anything, that should always be a fatal error. Something would be seriously wrong there.

@StephanTLavavej StephanTLavavej added the build Related to the build system label Aug 13, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 13, 2024 23:19
@CaseyCarter CaseyCarter removed their assignment Aug 14, 2024
find_program(CLANG_FORMAT
NAMES clang-format
PATHS "C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/bin"
HINTS "$ENV{VCINSTALLDIR}/Tools/Llvm/$ENV{VSCMD_ARG_HOST_ARCH}/bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

VSCMD_ARG_HOST_ARCH is probably wrong.

I have only C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\x64\bin and C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\ARM64\bin

but %VSCMD_ARG_HOST_ARCH% is x86 if I open x86 Native tools and I want to build x86 library.

изображение

And because ARM machines can run x64 binary I think we can change $ENV{VSCMD_ARG_HOST_ARCH} to x64. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ugh, yes, I forgot that x86 LLVM isn't in an x86 directory as is the case for x64 and arm64. I agree we should simply default to x64 for now. We can devise something clever if anyone ever complains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch! 😻 The x86 clang-format is lurking in C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\bin\clang-format.exe but I also agree, there's no point in attempting to ever use it. Validated the change:

Click to expand successful formatting, x64-native:
C:\Temp>"C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvarsall.bat" x64
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.12.0-pre.1.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'

C:\Temp>where clang-format
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\x64\bin\clang-format.exe

C:\Temp>pushd D:\GitHub\STL && cmake --preset x64 && cmake --build --preset x64 --target format
-- The CXX compiler identification is MSVC 19.42.34226.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.42.34226/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test WINDOWS_SDK_VERSION_CHECK
-- Performing Test WINDOWS_SDK_VERSION_CHECK - Success
-- The ASM_MASM compiler identification is MSVC
-- Found assembler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.42.34226/bin/Hostx64/x64/ml64.exe
-- Found clang-format 17.0.3.
-- Found Python: C:/Users/stl/AppData/Local/Programs/Python/Python312/python.exe (found suitable version "3.12.5", minimum required is "3.12") found components: Interpreter
-- Boost.Math: standalone mode ON
-- Configuring done (1.4s)
-- Generating done (0.2s)
-- Build files have been written to: D:/GitHub/STL/out/x64
[1328/1328] C:\WINDOWS\system32\cmd.exe /C "cd /D D:\GitHu...ub/STL/tools/format/../../tests/tr1/tests/regex2/test.cpp" 
Click to expand successful formatting, x86-native (and observe different where clang-format location):
C:\Temp>"C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvarsall.bat" x86
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.12.0-pre.1.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x86'

C:\Temp>where clang-format
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\Llvm\bin\clang-format.exe

C:\Temp>pushd D:\GitHub\STL && cmake --preset x86 && cmake --build --preset x86 --target format
-- The CXX compiler identification is MSVC 19.42.34226.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.42.34226/bin/Hostx86/x86/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test WINDOWS_SDK_VERSION_CHECK
-- Performing Test WINDOWS_SDK_VERSION_CHECK - Success
-- The ASM_MASM compiler identification is MSVC
-- Found assembler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.42.34226/bin/Hostx86/x86/ml.exe
-- Found clang-format 17.0.3.
-- Found Python: C:/Users/stl/AppData/Local/Programs/Python/Python312/python.exe (found suitable version "3.12.5", minimum required is "3.12") found components: Interpreter
-- Boost.Math: standalone mode ON
-- Configuring done (1.5s)
-- Generating done (0.2s)
-- Build files have been written to: D:/GitHub/STL/out/x86
[1328/1328] C:\WINDOWS\system32\cmd.exe /C "cd /D D:\GitHu...ub/STL/tools/format/../../tests/tr1/tests/regex2/test.cpp"

Despite x86's where clang-format difference, both CMake builds used the x64 clang-format.exe:

D:\GitHub\STL>rg CLANG_FORMAT out\x64\CMakeCache.txt out\x86\CMakeCache.txt
out\x64\CMakeCache.txt
24:CLANG_FORMAT:FILEPATH=C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe

out\x86\CMakeCache.txt
24:CLANG_FORMAT:FILEPATH=C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/Llvm/x64/bin/clang-format.exe

This is a nice verification that even on my machine (with only VS Preview's Clang/LLVM installed, no other copies), the HINTS take effect.

@StephanTLavavej StephanTLavavej self-assigned this Aug 15, 2024
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit b191409 into microsoft:main Aug 15, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the clang-format branch August 15, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to the build system
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants