Skip to content

Use the same version of clang-tidy and clang-format as clang#6006

Merged
wujingyue merged 12 commits intomainfrom
wjy/tidy
Feb 26, 2026
Merged

Use the same version of clang-tidy and clang-format as clang#6006
wujingyue merged 12 commits intomainfrom
wjy/tidy

Conversation

@wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Feb 24, 2026

...so clang-tidy and clang-format work with pre-compiled headers. Otherwise, I got

error: PCH file uses a newer PCH format that cannot be read [clang-diagnostic-error]

when I lintrunner.

Also cc @rdspring1 who wrote the initial code

This makes sure clang and clang-tidy have the same version so clang-tidy
works with pre-compiled headers. Otherwise, I got:

```
error: PCH file uses a newer PCH format that cannot be read [clang-diagnostic-error]
```
@github-actions
Copy link

github-actions bot commented Feb 24, 2026

Review updated until commit 759f66b

Description

  • Use llvm-config to locate clang-tidy and clang-format binaries instead of hardcoded paths

  • Install clang-tidy-19 and clang-format-19 alongside clang-19 for PCH compatibility

  • Remove Windows-specific shell handling and path conversion code

  • Update lintrunner configuration to use system-installed LLVM tools

Changes walkthrough

Relevant files
Enhancement
clangformat_linter.py
Use llvm-config to locate clang-format binary                       

tools/linter/adapters/clangformat_linter.py

  • Remove --binary argument and use llvm-config --bindir to find
    clang-format
  • Remove Windows-specific IS_WINDOWS variable and as_posix function
  • Replace shell=True with capture_output=True in subprocess calls
  • Update error messages to reference LLVM installation instead of
    lintrunner init
  • +30/-25 
    clangtidy_linter.py
    Use llvm-config to locate clang-tidy binary                           

    tools/linter/adapters/clangtidy_linter.py

  • Remove --binary argument and use llvm-config --bindir to find
    clang-tidy
  • Add TODO comment about CUDA include path for PCH support
  • Update error messages to reference LLVM installation instead of
    lintrunner init
  • +27/-20 
    Configuration changes
    apt-install-things.sh
    Install LLVM 19 tools and setup llvm-config                           

    tools/apt-install-things.sh

  • Install clang-tidy-19 and clang-format-19 alongside clang-19
  • Install llvm-19 and llvm-19-dev packages
  • Create llvm-config symlink to llvm-config-19 for binary discovery
  • Remove old llvm-18 version to avoid conflicts
  • Clean up cuda-keyring deb file after installation
  • +21/-6   
    lint.yml
    Setup environment before running lintrunner                           

    .github/workflows/lint.yml

  • Run apt and pip installation scripts in parallel before sourcing
    environment
  • Source tools/setup-env.sh after installations complete
  • +5/-0     
    .lintrunner.toml
    Remove binary paths and init commands from lintrunner config

    .lintrunner.toml

  • Remove --binary arguments from clangformat and clangtidy commands
  • Remove init_command sections that installed specific tool versions via
    pip
  • Rely on system-installed LLVM tools discovered via llvm-config
  • +0/-15   

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review
    Missing llvm-config fallback

    The code now depends on llvm-config being available in PATH, but there's no fallback mechanism if llvm-config exists but points to a different LLVM version than expected. Consider adding version validation.

    llvm_bindir = subprocess.check_output(
        ["llvm-config", "--bindir"], text=True
    ).strip()
    Windows compatibility removed

    Windows-specific handling (IS_WINDOWS, as_posix, shell=True) was removed but llvm-config behavior on Windows may differ from Unix systems. Verify Windows support is maintained.

    try:
        llvm_bindir = subprocess.check_output(
            ["llvm-config", "--bindir"], text=True
        ).strip()
    except FileNotFoundError:
        err_msg = LintMessage(
            path="<none>",
            line=None,
            char=None,
            code="CLANGTIDY",
            severity=LintSeverity.ERROR,
            name="command-failed",
            original=None,
            replacement=None,
            description="llvm-config doesn't exist (is LLVM installed?).",
        )
        print(json.dumps(err_msg._asdict()), flush=True)
        exit(0)
    
    binary_path = os.path.join(llvm_bindir, "clang-tidy")
    Symlink creation failure handling

    The llvm-config symlink creation could fail silently if llvm-config-19 doesn't exist, potentially breaking the linter. Add better error handling for this critical step.

    sudo ln -sf "$(command -v llvm-config-19)" /usr/bin/llvm-config

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Feb 24, 2026

    Greptile Summary

    This PR resolves PCH format incompatibility by ensuring clang-tidy and clang-format use the same LLVM version (19) as the clang compiler. The changes replace pip-installed tools with system packages and use llvm-config for dynamic binary discovery instead of hardcoded paths.

    Key changes:

    • Added system installation of clang-tidy-19, clang-format-19, llvm-19, and llvm-19-dev in apt-install-things.sh
    • Modified both linter scripts to use llvm-config --bindir to locate binaries dynamically
    • Removed init_command sections and hardcoded --binary arguments from .lintrunner.toml
    • Added parallel dependency installation in the GitHub workflow
    • Cleaned up Windows-specific code in clangformat_linter.py

    Notes:

    • The error handling for llvm-config calls catches FileNotFoundError but not CalledProcessError (already noted in previous review comments)
    • Some previous review comments contain incorrect assessments (e.g., the llvm-config-19 verification in apt-install-things.sh correctly happens after installation, not before)

    Confidence Score: 4/5

    • This PR is safe to merge with minimal risk - it improves LLVM version consistency and solves the stated PCH compatibility issue
    • The changes are well-structured and solve a real problem (PCH format mismatch). The approach of using llvm-config for dynamic discovery is more maintainable than hardcoded paths. Minor error handling improvements noted in previous comments should be addressed but don't block merging, as the FileNotFoundError handling covers the most common failure case
    • No files require special attention - the changes are straightforward infrastructure updates

    Important Files Changed

    Filename Overview
    .github/workflows/lint.yml Added parallel installation of dependencies before running lintrunner to ensure llvm-config and related tools are available
    .lintrunner.toml Removed hardcoded binary paths and init_command sections for clang-format and clang-tidy, now using llvm-config for dynamic discovery
    tools/apt-install-things.sh Added installation of clang-format-19, clang-tidy-19, llvm-19, and llvm-19-dev; removed llvm-18; created llvm-config symlink for version consistency
    tools/linter/adapters/clangformat_linter.py Replaced hardcoded binary path with llvm-config-based discovery, removed Windows-specific code, simplified subprocess calls
    tools/linter/adapters/clangtidy_linter.py Replaced hardcoded binary path with llvm-config-based discovery, maintaining existing include path logic

    Last reviewed commit: 759f66b

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    5 files reviewed, 2 comments

    Edit Code Review Agent Settings | Greptile

    Copy link
    Collaborator

    @mdavis36 mdavis36 left a comment

    Choose a reason for hiding this comment

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

    You will likely get an error with clang-tidy still w/ PCH. We need to have lintrunner use the clang-tidy-19 executable otherwise it will throw a different error:

    ❯ lintrunner --take CLANGTIDY --force-color csrc/type_promotion.cpp
      CLANGTIDY failure                                                                                                                                                                                                                              
                                                                                                            
    
    >>> Lint for csrc/type_promotion.cpp:
    
      Error (CLANGTIDY) command-failed
        Failed due to Command '['/home/dev/.local/bin/clang-tidy', '-p=/home/dev/
        workspace/Fuser/python/build', '--extra-arg', '-I/usr/lib/llvm-11/include/
        openmp', '--extra-arg', '-I/usr/include/python3.12', '--extra-arg', '-I/
        home/dev/workspace/Fuser/third_party/pybind11/include', '--extra-arg',
        '-I/usr/include/c++/13', '--extra-arg', '-I/usr/include/x86_64-linux-
        gnu/c++/13', '--extra-arg', '-I/usr/include/c++/13/backward', '--extra-
        arg', '-I/usr/lib/gcc/x86_64-linux-gnu/13/include', '--extra-arg',
        '-I/usr/local/include', '--extra-arg', '-I/usr/include/x86_64-linux-
        gnu', '--extra-arg', '-I/usr/include', '/home/dev/workspace/Fuser/csrc/
        type_promotion.cpp']' returned non-zero exit status 1.
        
        stdout:
        error: PCH file '/home/dev/workspace/Fuser/python/build/CMakeFiles/
        codegen_internal.dir/cmake_pch.hxx.pch' built from a different branch
        ((0ubuntu1~24.04.2)) than the compiler () [clang-diagnostic-error]
        
        stderr:
        1 error generated.
        Error while processing /home/dev/workspace/Fuser/csrc/type_promotion.cpp.
        Found compiler error(s).
    

    From claude on why (I use 20 for no particular reason, and was trying this w/ 20 versions on my system):

    the PCH was built by a system-packaged clang-20 (0ubuntu1~24.04.2 branch) while the pip-installed clang-tidy 20.1.0 is a vanilla upstream build with no branch string. They're technically different builds of clang-20, so the PCH is still incompatible.

    Removing the init command and specifying the system binary fixed this for me.

          178 -init_command = [                                                                                       
          179 -    'python3',                                                                                       
          180 -    'tools/linter/adapters/pip_init.py',                                                               
          181 -    '--dry-run={{DRYRUN}}',                                                                          
          182 -    'clang-tidy==19.1.0.1',                                                                          
          183 -]                                                                                               
          178  command = [                                                                                     
          179      'python3',
          180      'tools/linter/adapters/clangtidy_linter.py',
          181 -    '--binary=~/.local/bin/clang-tidy',                                                         
          181 +    '--binary=/usr/bin/clang-tidy-20',                                                          
          182      '--build_dir=./python/build',
          183      '--',
          184      '@{{PATHSFILE}}'
    

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    1 file reviewed, no comments

    Edit Code Review Agent Settings | Greptile

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    3 files reviewed, 2 comments

    Edit Code Review Agent Settings | Greptile

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    3 files reviewed, 2 comments

    Edit Code Review Agent Settings | Greptile

    @wujingyue wujingyue changed the title Install clang-tidy-19 Use the same version of clang-tidy and clang-format as clang Feb 24, 2026
    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    4 files reviewed, 1 comment

    Edit Code Review Agent Settings | Greptile

    @wujingyue
    Copy link
    Collaborator Author

    You will likely get an error with clang-tidy still w/ PCH.

    Yes, I planned to fix that in multiple PRs. However, that lost the important context so I decided to include more.

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    4 files reviewed, 1 comment

    Edit Code Review Agent Settings | Greptile

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Feb 24, 2026

    Additional Comments (1)

    tools/linter/adapters/clangformat_linter.py
    shell=True with list breaks argument passing

    On POSIX, subprocess.run(args, shell=True) where args is a list passes only args[0] as the command string to /bin/sh -c. The remaining list elements become $0, $1, etc. to the shell — they are not appended as arguments to the command.

    This means when check_file calls _run_command([binary, filename], ...) at line 91, it effectively runs sh -c "/usr/lib/llvm-19/bin/clang-format" "<filename>", where <filename> is set as $0 in the shell rather than being passed as an argument to clang-format. The result is clang-format runs with no file argument and reads from stdin (which hangs or returns empty output).

    The previous code had shell=IS_WINDOWS which was False on Linux, so shell=False was the correct behavior. Since Windows support was removed, you can just hardcode shell=False (or remove the shell kwarg entirely since False is the default):

                args,
                stdout=subprocess.PIPE,
                stderr=subprocess.PIPE,
                timeout=timeout,
    

    # "/usr/local/cuda-13.0/targets/x86_64-linux/include". I'll try to add that in
    # a future PR.
    include_dir = [
    "/usr/lib/llvm-11/include/openmp",
    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    What's this?

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    5 files reviewed, 4 comments

    Edit Code Review Agent Settings | Greptile

    @wujingyue
    Copy link
    Collaborator Author

    !build

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    5 files reviewed, no comments

    Edit Code Review Agent Settings | Greptile

    from typing import Any, List, NamedTuple, Optional


    IS_WINDOWS: bool = os.name == "nt"
    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    YAGNI

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    5 files reviewed, 2 comments

    Edit Code Review Agent Settings | Greptile

    @wujingyue wujingyue requested a review from mdavis36 February 26, 2026 00:27
    @wujingyue
    Copy link
    Collaborator Author

    !build

    @wujingyue
    Copy link
    Collaborator Author

    !build

    @wujingyue wujingyue merged commit 0cfc24b into main Feb 26, 2026
    15 of 16 checks passed
    @wujingyue wujingyue deleted the wjy/tidy branch February 26, 2026 02:57
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants