Open thernstig opened 2 weeks ago
@thernstig What version of clang-tidy/format do you have installed? From PR https://github.com/microsoft/vscode-cpptools/pull/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?
> 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)
https://github.com/microsoft/vscode-cpptools/pull/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.
@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?
@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 https://github.com/microsoft/vscode-cpptools/issues/12718#issuecomment-2354668461.
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.
@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.
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.
@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?
@bobbrow how would we proceed?
Let's use this issue to track the request. I can't guarantee that we'll get to it immediately, but if we see an uptick in 👍 reactions to it, that will help bump the priority. I'll update the title to make it more discoverable to others.
Hi, I came here because I have a similar issue: the extension doesn't use clang-tidy from the system path, and I have to specify the path manually (same for clang-format). I'm a bit confused about the discussion here. You are saying that the extension will not use clang-tidy from the system path if it's older than the bundled version. But the bundled version is 18.1.8 and the version in the system path is 19.1.0, which is newer, so why is it not being used?
@bshoshany I never myself contemplated checking the version, I just trusted @bobbrow immediately knew what was wrong.
But when i checked now, you are completely right. Even I have a newer version locally in my env than the bundled one:
> /home/tobias/.vscode-server/extensions/ms-vscode.cpptools-1.21.6-linux-x64/bin/../LLVM/bin/clang-tidy --version
LLVM (http://llvm.org/):
LLVM version 18.1.8
Optimized build.
~/code> clang-tidy --version
Ubuntu LLVM version 20.0.0
Optimized build.
@bobbrow is there a bug here in play? In addition to the issue about an improvement in this area.
Environment
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:
The
clang-format
binary exists in the PATH:The same problem occurs for
clang-tidy
.According to the settings it should by default use it from the path:
Steps to reproduce: 1.Make sure to have
clang-tidy
in the path.Expected behavior: It should use the binaries from the env.
Configuration and Logs
Other Extensions
No response
Additional context
No response