This was unintentionally disabled in the refactor commit https://github.com/bitcoin/bitcoin/pull/33101/commits/eb59a192d9ca3b1a27051cb5f2b3483186fe9928 from #33101.
This is useful for the clangd language server, and there is no harm in enabling it even for those that don’t use it as far as I know, I assume that it is only temporarily disabled during secp256k1 compilation because this results in bad metadata being exported to compile_commands.json.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33187.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | purpleKarrot, stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
47@@ -48,4 +48,5 @@ function(add_secp256k1 subdir)
48 set_target_properties(secp256k1 PROPERTIES
49 EXCLUDE_FROM_ALL TRUE
50 )
51+ set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
Thanks, I did not realize that set was scoped to the current function. (https://cmake.org/cmake/help/latest/command/set.html#set-normal-variable)
I’ve fixed this.
DCMAKE_EXPORT_COMPILE_COMMANDS=ON during cmake -B (it’s being referenced in the developer notes on another use case but perhaps could be make it a more clear for this case).
88@@ -89,6 +89,9 @@ if (NOT DEFINED CMAKE_OPTIMIZE_DEPENDENCIES)
89 set(CMAKE_OPTIMIZE_DEPENDENCIES TRUE)
90 endif()
91
92+# Enable export of `compile_commands.json` used by the clangd language server.
93+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
CMAKE_ are not intended to be set by projects. IDEs may set CMAKE_EXPORT_COMPILE_COMMANDS when configuring the project. You may also set CMAKE_EXPORT_COMPILE_COMMANDS as an environment variable in your dotfiles. But don’t force it on other developers that have their own needs.
I agree it seems easy-enough to define this in one’s local environment if it’s wanted.
It is currently docuemented in ./doc/developer-notes.md in the context of running clang-tidy, but perhaps (as an alternative to this PR) it could also be documented as being useful for clangd language servers?
It’s certainly easy enough to define this for one’s environment, but IMO it’s a waste of project resources to document doing this, and a waste of new contributor resources to go around looking for this, when there is no harm at all in enabling it, and it enriches most modern development environments instantly to have this enabled.
In what circumstance would someone ever be harmed by having this enabled? What use-case is disrupted? No one has complained about this being enabled since the introduction of CMake a year ago. This seems definitionally like a sane default.
This is used to export build metadata used by the `clangd` language
server.
In what circumstance would someone ever be harmed by having this enabled?
I think that is the wrong question to ask. We should not enable settings until people complain. We should stick to the defaults unless there is a technical reason not to.
Generating the additional file is not without overhead. It clearly is something to opt-in on an as-needed basis.
Also, we should always keep in mind that people may want to build bitcoin-core as a sub-project of something bigger. So even if you think bitcoin-core is not a big project by itself and the overhead of generating an additional file is negligible, that is not necessarily the case for all customers.
I think that is the wrong question to ask. We should not enable settings until people complain. We should stick to the defaults unless there is a technical reason not to.
There is a reason to enable this, it is useful for new contributors that are using modern development environments that supports LSP tooling, and completely harmless to everyone else.
0$ hyperfine --warmup 3 -P export_commands 0 1 'cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS={export_commands}' --prepare "rm -rf build"
1Benchmark 1: cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=0
2 Time (mean ± σ): 9.892 s ± 0.028 s [User: 5.369 s, System: 4.606 s]
3 Range (min … max): 9.861 s … 9.941 s 10 runs
4
5Benchmark 2: cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1
6 Time (mean ± σ): 9.887 s ± 0.031 s [User: 5.369 s, System: 4.609 s]
7 Range (min … max): 9.827 s … 9.921 s 10 runs
8
9Summary
10 cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1 ran
11 1.00 ± 0.00 times faster than cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=0
12
13$ du -h build/compile_commands.json
14448K build/compile_commands.json
The alternative to enabling this by default is documenting it for no reason other than to sleep easy knowing we respected the cmake defaults and intended semantics, despite the fact that in our use-case a setting different from CMake’s default is clearly useful!
There has been upstream discussion to enable this by default in CMake (https://gitlab.kitware.com/cmake/cmake/-/issues/18386) where the rationale given for not doing so was:
“Once this is turned on by default it can never be turned off even if some future alternative becomes commonplace. I’m open to a user-wide configuration approach so individuals can opt-in.”
That was 7 years ago and no alternative has emerged!
There are plenty of other large C/C++ projects that enable this by default:
It seems to me this is a great example of a sane default, having it enabled benefits some and harms none.
it is useful for new contributors that are using modern development environments
Modern development environments set this variable automatically. There is no need to hardcode that in the project.
That was 7 years ago and no alternative has emerged!
Yes it has. The support for an environment variable was added in version 3.17, which was released more than 5 years ago.
It would be good to link to the environment variables in our documentation and make suggestions which variables we recommend to people new to cmake.
In my dotfiles, I set the following variables. This is highly opinionated and I would not want to force them on anyone else:
0 set -x CMAKE_BUILD_PARALLEL_LEVEL (nproc)
1 set -x CMAKE_C_COMPILER_LAUNCHER (which ccache)
2 set -x CMAKE_COLOR_DIAGNOSTICS 1
3 set -x CMAKE_CXX_COMPILER_LAUNCHER (which ccache)
4 set -x CMAKE_EXPORT_COMPILE_COMMANDS 1
5 set -x CMAKE_GENERATOR Ninja
6 set -x CMAKE_INSTALL_PREFIX $HOME/.local
7 set -x CMAKE_POLICY_VERSION_MINIMUM 3.5
8 set -x CMAKE_PREFIX_PATH $HOME/.local
9 set -x CTEST_EXPERIMENTAL_INSTRUMENTATION "a37d1069-1972-4901-b9c9-f194aaf2b6e0"
10 set -x CTEST_OUTPUT_ON_FAILURE 1
11 set -x CTEST_PARALLEL_LEVEL (nproc)
12 set -x CTEST_PROGRESS_OUTPUT 1
13 set -x CTEST_USE_INSTRUMENTATION 1
14 set -x CTEST_USE_LAUNCHERS_DEFAULT 1
15 set -x CTEST_USE_VERBOSE_INSTRUMENTATION 1
Therefore, I think none of the variables should by set by projects. The fact that many projects do it does not change my opinion.
it is useful for new contributors that are using modern development environments
Modern development environments set this variable automatically. There is no need to hardcode that in the project.
That’s not true for users of editors with native or plugin LSP support like vim, neovim, emacs etc. Nor is it the case for CLion.
If someone is using VSCode and installs and uses the vscode-cmake-tools extension to build then yes, it will set CMAKE_EXPORT_COMPILE_COMMANDS=1 by default.
That was 7 years ago and no alternative has emerged!
Yes it has. The support for an environment variable was added in version 3.17, which was released more than 5 years ago.
The “alternative” referred to in what I quoted was not an alternative to manually setting the flag, but an alternative metadata format to clangd’s compile_commands.json.
This was unintentionally disabled in the refactor commit eb59a19 from #33101.
I’d argue the opposite: db7a198f29c62c5f762eaa25d1d83c57e2f1e211 unintentionally enabled CMAKE_EXPORT_COMPILE_COMMANDS.
The design goal was to use the compilation database compile_commands.json for static code analysis tools such as clang-tidy and IWYU. In particular, we are not interested in analyzing subtrees’ code. This was handled by disabling the EXPORT_COMPILE_COMMANDS property for build targets in all subtrees, including src/secp256k1.
This is useful for the
clangdlanguage server, and there is no harm in enabling it even for those that don’t use it as far as I know, I assume that it is only temporarily disabled during secp256k1 compilation because this results in bad metadata being exported tocompile_commands.json.
Usually, we keep any adjustments that are only useful in specific build environments (such as file ignore patterns, etc) outside the repository, and instead suggest developers adjust their local environment.
And I do concur with all @purpleKarrot’s comments above.
compile_commands.json metadata, but I’ll close this for now as it lacks conceptual support.
Pretty sure this not being enabled has wasted my time (even though I mostly worked on PRs which didn’t include the #33101 change). I assumed it was something broken with my LSP plugin, and was meaning to look into it eventually, getting by through grepping files.
Having this on by default saves many developers from spending time on figuring this aspect out. As shown by the author, changing the default is commonplace among other projects: #33187 (comment)
There is a +1% runtime overhead from generating compile_commands.json on my machine:
0₿ hyperfine --warmup 3 -P export_commands 0 1 'cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS={export_commands}' --prepare "rm -rf build"
1Benchmark 1: cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=0
2 Time (mean ± σ): 18.210 s ± 0.077 s [User: 12.818 s, System: 5.303 s]
3 Range (min … max): 18.072 s … 18.299 s 10 runs
4
5Benchmark 2: cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1
6 Time (mean ± σ): 18.304 s ± 0.087 s [User: 12.894 s, System: 5.310 s]
7 Range (min … max): 18.193 s … 18.488 s 10 runs
8
9Summary
10 cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=0 ran
11 1.01 ± 0.01 times faster than cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1
So there is some reason in not enabling it for all builds.
At the very least I think an improvement over the last/latest push would be to guard around the setting of the variable, as done in the Git project:
0if(NOT DEFINED CMAKE_EXPORT_COMPILE_COMMANDS)
1 set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
2endif()
A different approach that might be more palatable would be to only enable CMAKE_EXPORT_COMPILE_COMMANDS in the dev-mode preset in CMakePresets.json.
Curious to hear what people think.
Having this on by default…
Why doesn’t export CMAKE_EXPORT_COMPILE_COMMANDS=ON in ~/.profile (or something similar) work for you?
We generally don’t include editor-specific configuration, as it gets unclear what the bar for relevance is, and easily gets unmaintainable. For example, .gitignore also doesn’t include such things either (see #32417).
That doesn’t mean that we can’t have some documentation about integration in specific systems.
A different approach that might be more palatable would be to only enable
CMAKE_EXPORT_COMPILE_COMMANDSin the dev-mode preset in CMakePresets.json.
I would object much less than setting this in CMakeLists.txt.
Why doesn’t
export CMAKE_EXPORT_COMPILE_COMMANDS=ONin~/.profile(or something similar) work for you?
It does for me personally, now that I know about this PR/issue.
I think the main concern here for me & PR author is not to fix it for ourselves, but to minimize time spent by other developers onboarding to the project, or existing developers shifting to look at newer commits post #33101.
We generally don’t include editor-specific configuration, as it gets unclear what the bar for relevance is, and easily gets unmaintainable. For example,
.gitignorealso doesn’t include such things either (see #32417).That doesn’t mean that we can’t have some documentation about integration in specific systems.
It is a good general principle, but I believe reliance on compile_commands.json and some form of clangd LSP is fairly ubiquitous. Can’t pretend to have done any surveys though.
A different approach that might be more palatable would be to only enable
CMAKE_EXPORT_COMPILE_COMMANDSin the dev-mode preset in CMakePresets.json.I would object much less than setting this in
CMakeLists.txt.
Nice!
Some random guy suggests setting an environment variable: https://youtu.be/xNHKTdnn4fY?t=1535 :)
We shouldn’t expect the average Bitcoin Core contributor to consume 90min CMake videos but it’s great that you do.
He also suggests every tool should support compile_commands.json and any issue/suggestion to do so would be massively upvoted.
(Adjacent issue: I never build within my IDE, so even if it were to set the environment variable automatically, I would not have benefited from that).
I think that knobs should be set in a way that as much as is reasonable relieves users from the burden of having to pore over documentation to understand what each option does before proceeding successfully, and I maintain that no one has or ever would complain if this was enabled1, in this project or in any other project, and many would enjoy living life in sweet ignorance of CMAKE_EXPORT_COMPILE_COMMANDS=1 while reaping its rewards.
I agree in principle that development-environment-specific stuff should generally not be included in the repository. I lean strongly towards feeling that this is something generally useful and not specific to my environment but I recognize that I may be a little biased about how universal my experience is and honestly this topic has gotten more attention than it deserves so I think it would be productive to leave this closed and move on with the alternative solutions suggested by @pablomartin4btc and @willcl-ark of documenting this flag and @hodlinator of enabling it in the dev-mode preset.
But, this branch would be improved by using this approach where CMAKE_EXPORT_COMPILE_COMMANDS is set only if it is unset by the user so that we don’t override it being explicitly disabled as suggested by @hodlinator above. ↩︎