Add a better way to select the bundled clang tools so that we can pick the env version by default.

[thernstig]

Environment

• OS and Version: Windows 11 23H2
• VS Code Version: 1.92.2
• C/C++ Extension Version: v1.21.6
• If using SSH remote, specify OS of remote machine: wsl (Ubuntu 20.04)

Bug Summary and Steps to Reproduce

Bug Summary:

The LSP is not using the correct clang-tidy and clang-format binaries from environment. This is severe as it is using the built-in binaries. The large problem with this is e.g. what faults you get in VS Code vs running clang-tidy and clang-format in CI will differ if they are of different versions.

See this from the logs below:

/home/tobias/.vscode-server/extensions/ms-vscode.cpptools-1.21.6-linux-x64/bin/../LLVM/bin/clang-format

The clang-format binary exists in the PATH:

> which clang-format
/usr/bin/clang-format

The same problem occurs for clang-tidy.

According to the settings it should by default use it from the path:

  // The full path of the `clang-format` executable. If not specified, and `clang-format` is available in the environment path, that is used. If not found in the environment path, the `clang-format` bundled with the extension will be used.
  "C_Cpp.clang_format_path": "",
  // The full path of the `clang-tidy` executable. If not specified, and `clang-tidy` is available in the environment path, that is used. If not found in the environment path, the `clang-tidy` bundled with the extension will be used.
  "C_Cpp.codeAnalysis.clangTidy.path": "",

Steps to reproduce:
1.Make sure to have clang-tidy in the path.
2. Check the LSP output.

Expected behavior:
It should use the binaries from the env.

Configuration and Logs

{
    "configurations": [
        {
            "name": "Linux",
            "includePath": [
                "${workspaceFolder}/**"
            ],
            "defines": [],
            "compilerPath": "/usr/bin/gcc",
            "cStandard": "c99",
            "cppStandard": "gnu++14",
            "intelliSenseMode": "linux-gcc-x64",
            "configurationProvider": "ms-vscode.cmake-tools"
        },
        {
            "name": "CMake",
            "compileCommands": "${config:cmake.buildDirectory}/compile_commands.json",
            "configurationProvider": "ms-vscode.cmake-tools"
        }
    ],
    "version": 4
}

-------- Diagnostics - 9/4/2024, 1:10:29 PM
Version: 1.21.6
Current Configuration:
{
    "name": "Linux",
    "includePath": [
        "/home/tobias/code/c-test/**"
    ],
    "defines": [],
    "compilerPath": "/usr/bin/gcc",
    "cStandard": "c99",
    "cppStandard": "gnu++14",
    "intelliSenseMode": "linux-gcc-x64",
    "configurationProvider": "ms-vscode.cmake-tools",
    "compilerPathIsExplicit": true,
    "cStandardIsExplicit": true,
    "cppStandardIsExplicit": true,
    "intelliSenseModeIsExplicit": true,
    "compilerPathInCppPropertiesJson": "/usr/bin/gcc",
    "configurationProviderInCppPropertiesJson": "ms-vscode.cmake-tools",
    "mergeConfigurations": false,
    "browse": {
        "path": [
            "/home/tobias/code/c-test/**",
            "${workspaceFolder}"
        ],
        "limitSymbolsToIncludedHeaders": true
    }
}
Custom browse configuration: 
{
    "browsePath": [
        "/home/tobias/code/c-test/src"
    ],
    "compilerPath": "/usr/bin/gcc",
    "compilerArgs": [],
    "compilerFragments": [
        "-g"
    ]
}
cpptools version (native): 1.21.6.0
Translation Unit Mappings:
[ /home/tobias/code/c-test/src/main.c - source TU]:
Translation Unit Configurations:
[ /home/tobias/code/c-test/src/main.c ]:
    Process ID: 30660
    Memory Usage: 15 MB
    Compiler Path: /usr/bin/gcc
    Includes:
    System Includes:
        /usr/lib/gcc/x86_64-linux-gnu/9/include
        /usr/local/include
        /usr/include/x86_64-linux-gnu
        /usr/include
    Standard Version: c17
    IntelliSense Mode: linux-gcc-x64
    Other Flags:
        --gcc
        --gnu_version=90400
Total Memory Usage: 15 MB

------- Workspace parsing diagnostics -------
Number of files discovered (not excluded): 4908

LSP: (received) cpptools/didChangeActiveEditor: file:///home/tobias/code/c-test/src/main.c
LSP: (invoked) cpptools/didChangeActiveEditor: file:///home/tobias/code/c-test/src/main.c
LSP: (received) cpptools/formatDocument: file:///home/tobias/code/c-test/src/main.c (id: 161)
LSP: (invoked) cpptools/formatDocument: file:///home/tobias/code/c-test/src/main.c (id: 161)
Formatting document: file:///home/tobias/code/c-test/src/main.c
Formatting Engine: clangFormat
/home/tobias/.vscode-server/extensions/ms-vscode.cpptools-1.21.6-linux-x64/bin/../LLVM/bin/clang-format -style=file -fallback-style=LLVM --Wno-error=unknown -assume-filename=/home/tobias/code/c-test/src/main.c
LSP: Sending response (id: 161)
LSP: (received) textDocument/willSaveWaitUntil: file:///home/tobias/code/c-test/src/main.c (id: 162)
LSP: (invoked) textDocument/willSaveWaitUntil: file:///home/tobias/code/c-test/src/main.c (id: 162)
LSP: Sending response (id: 162)
willSaveWaitUntil: 0ms
LSP: (received) textDocument/didSave: file:///home/tobias/code/c-test/src/main.c
LSP: (invoked) textDocument/didSave: file:///home/tobias/code/c-test/src/main.c
Intellisense update pending for: file:///home/tobias/code/c-test/src/main.c
tag parsing file: /home/tobias/code/c-test/src/main.c
LSP: (received) cpptools/fileChanged: file:///home/tobias/code/c-test/src/main.c
LSP: (invoked) cpptools/fileChanged: file:///home/tobias/code/c-test/src/main.c
IntelliSense update scheduled and TU acquisition started for: file:///home/tobias/code/c-test/src/main.c
Update IntelliSense time (sec): 0.006
LSP: (received) cpptools/getFoldingRanges: file:///home/tobias/code/c-test/src/main.c (id: 163)
LSP: (invoked) cpptools/getFoldingRanges: file:///home/tobias/code/c-test/src/main.c (id: 163)
LSP: Sending response (id: 163)
LSP: (received) cpptools/getDocumentSymbols: file:///home/tobias/code/c-test/src/main.c (id: 164)
LSP: (invoked) cpptools/getDocumentSymbols: file:///home/tobias/code/c-test/src/main.c (id: 164)
LSP: Sending response (id: 164)
Database safe to open.
LSP: (received) cpptools/didChangeActiveEditor: file:///home/tobias/code/c-test/src/main.c
LSP: (invoked) cpptools/didChangeActiveEditor: file:///home/tobias/code/c-test/src/main.c
LSP: (received) cpptools/didChangeTextEditorSelection
LSP: (invoked) cpptools/didChangeTextEditorSelection
LSP: (received) cpptools/getCodeActions: file:///home/tobias/code/c-test/src/main.c (id: 165)
LSP: (invoked) cpptools/getCodeActions: file:///home/tobias/code/c-test/src/main.c (id: 165)
LSP: Sending response (id: 165)

Other Extensions

No response

Additional context

No response

[sean-mcmanus]

@thernstig What version of clang-tidy/format do you have installed? From PR #4968 we don't automatically use the env one if it's an older version. Can you set the path "C_Cpp.clang_format_path" , "C_Cpp.codeAnalysis.clangTidy.path" to the env one as a workaround?

[thernstig]

> clang-tidy --version
Ubuntu LLVM version 20.0.0
  Optimized build.
> clang-format --version
Ubuntu clang-format version 20.0.0 (++20240828064146+c43190f99445-1~exp1~20240828184310.1138)

#4968 and using the built-in if it is newer than the env is not how any other VS Code linter extensions I use do it, and I use quite a few in both JavaScript and Python.

It also does not make sense. Imagine that most teams use a version that is the same in development and in CI/CD. If you are overriding this with this extension in the env you will get a different result than in CI. I think it is quite idiomatic to always use the one from env if there is one.

[sean-mcmanus]

@thernstig Are you saying the 20.0.0 version from your env is not getting used?

If you want us to change the default behavior for older clang-tidy/format in the env, can you file a new issue to track that?

[thernstig]

@sean-mcmanus correct, it is not used.

If you see the initial post the first thing I show is this:

/home/tobias/.vscode-server/extensions/ms-vscode.cpptools-1.21.6-linux-x64/bin/../LLVM/bin/clang-format

You just said yourself it is using the built-in version, if the built-in version is laster than the one in environment. This is, I would argubaly say incorrect design. No other extension does this kind of logic, as I wrote in #12718 (comment).

[bobbrow]

Hi @thernstig,

Your opinion is that the env should always win, but in #4963 the opinion was that we should pick the newest since there is no easy way to pick the bundled version when the user is stuck with an older one since the path to the bundled one changes with each update.

So we have conflicting requests from customers. I think both arguments have merit. To satisfy both perhaps a new setting is required, but you do currently have the option of updating settings to tell the extension not to use the bundled version. I don't personally have a strong opinion on the matter, but the current design does allow for both camps to configure the extension such that it works for them.

[thernstig]

@bobbrow this is not a novel idea nor a very subjective opinion of mine; it works like that for other extensions. It is arguably the most correct way. I think my explanation as to why is straightforward: Users need to have reproducible environments between local development and CI/CD.

There are many example. If you want to look at one of the latest and greatest, see:
https://docs.astral.sh/ruff/editors/settings/#importstrategy

Also VS Code ESlint does this.

It is pretty idiomatic amongst linter/formatter extensions to do it like this. And there is, as mentioned again above, a very strong rationale.

There is an easy way to fix this, just like Ruff does it. Use predefined variables like useBundled or fromEnvironment, or last but not least a real path to an executable. fromEnvironment should be the default, and pick the one found from env. Or fall back to useBundled if one in the env is not found.

edit: I realize I come of as strong here. I apologize for that! I just believe there is a very valid point to not pick "latest" bundled, and I think that merit holds true for all linters/formatters and amongst the bigger community. That is the reason why pinning versions is so prevalent.

[bobbrow]

edit: I realize I come of a strong here. I apologize for that!

No worries. We understand that everyone has an opinion, and hopefully I'm not coming across as being dismissive of your suggestion. I said that both arguments have merit.

The Ruff approach is essentially what I suggested, though I didn't go into implementation details:

To satisfy both perhaps a new setting is required

Since we made the change over 4 years ago, I can't say I remember the discussions, but since we already have so many settings, it was probably the case that we avoided adding a new one at that time and tried to make it work within the confines of the existing setting.

[thernstig]

@bobbrow how would we proceed? If a fromEnvironment and useBundled were added, with fromEnvironment set as default, it would in effect solve this.

Ruff takes the approach of having both a https://docs.astral.sh/ruff/editors/settings/#importstrategy and a https://docs.astral.sh/ruff/editors/settings/#path, where the former has the fromEnvironment and useBundled. But the path latter path option takes precedence only if set.

Should I write a new issue for this, or keep this one going?