Overhaul how we search for clang-format by StephanTLavavej · Pull Request #4888 · microsoft/STL (original) (raw)

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).
    * See CMake's documentation for Comparisons and Version Comparisons.
  • 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.