-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cmake] Auto-complete via clangd auto-setup #29313
base: main
Are you sure you want to change the base?
Conversation
EWS run on previous version of this PR (hash 05612f8) |
05612f8
to
845e6b2
Compare
EWS run on previous version of this PR (hash 845e6b2) |
845e6b2
to
06a7bb0
Compare
EWS run on previous version of this PR (hash 06a7bb0) |
|
||
set(webkitbuild_dir ${CMAKE_SOURCE_DIR}/WebKitBuild) | ||
cmake_path(IS_PREFIX webkitbuild_dir ${CMAKE_BINARY_DIR} NORMALIZE clangd_auto_setup_default) | ||
set(CLANGD_AUTO_SETUP ${clangd_auto_setup_default} CACHE BOOL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use option
here?
https://cmake.org/cmake/help/latest/command/option.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option()
is just a specific case of set()
. It might be possible but there would be no difference whatsoever.
Source/cmake/WebKitCommon.cmake
Outdated
endif () | ||
|
||
set(webkitbuild_dir ${CMAKE_SOURCE_DIR}/WebKitBuild) | ||
cmake_path(IS_PREFIX webkitbuild_dir ${CMAKE_BINARY_DIR} NORMALIZE clangd_auto_setup_default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you check this here? How about checking DEVELOPER_MODE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would definitely be an option. This was my train of thought behind this default: "writing things in the source tree instead of the build tree is generally frowned upon, but we need to do it for clangd. However, if the user is building inside WebKitBuild
, they're already doing a build inside their source tree, so it's not dirtying that much more.
However, I could use DEVELOPER_MODE, or even boolean OR of DEVELOPER_MODE or building inside WebKitBuild
. What do you think makes the most sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I have no preference. It's up to you.
Source/cmake/WebKitCommon.cmake
Outdated
# Create a simple symlink in DeveloperTools/compile_commands.json so that | ||
# it can be relied upon regardless of unified or non-unified builds. | ||
add_custom_command( | ||
OUTPUT ${CMAKE_BINARY_DIR}/DeveloperTools/compile_commands.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can generate DeveloperTools/compile_commands.json symlink at CMake time rather than build time for non-unified builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but when using unified builds it would still be generated at build time, so I see no reason to do things differently for non-unified builds.
Ideally I'd like both to be done at CMake generation time. Unfortunately CMake doesn't support that. I looked even into the CMake source code to look for somewhere to hook it into, but compile_commands.json
is only written at the end of generation, long after all configure scripts have run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Update) I ended up changing this to create the symlink in configure time, as it didn't add much complexity.
Source/cmake/WebKitCommon.cmake
Outdated
# it can be relied upon regardless of unified or non-unified builds. | ||
add_custom_command( | ||
OUTPUT ${CMAKE_BINARY_DIR}/DeveloperTools/compile_commands.json | ||
DEPENDS ${CMAKE_BINARY_DIR}/compile_commands.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to regenerate DeveloperTools/compile_commands.json every time compile_commands.json is updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be accurate yes, I would assume the main compile_commands.json
only changes when cmake
is invoked with changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compile_commands.json
is generated along with the Ninja file, so I would expect both to be dirtied simultaneously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeveloperTools/compile_commands.json
is just a symlink constantly refering to ../compile_commands.json
in non-unified build case.
Source/cmake/WebKitCommon.cmake
Outdated
${CMAKE_BINARY_DIR}/DeveloperTools/compile_commands.json | ||
VERBATIM | ||
) | ||
add_custom_target(SymlinkCompileCommandsInDeveloperTools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using a pair of add_custom_command and add_custom_target here. But, above section RewriteCompileCommands is using only add_custom_target. How different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the confusion... it comes from CMake being very janky.
From add_custom_target() documentation:
The target has no output file and is always considered out of date even if the commands try to create a file with the name of the target. Use the add_custom_command() command to generate a file with dependencies.
You may be thinking "wait, does that mean RewriteCompileCommands
is running every time you build?" Yes, the answer is yes. That could very much be fixed and I was tempted on patching that too, but I ultimately considered it outside of the scope of this patch.
You may think that just using add_custom_command() instead would solve it, but the jank strikes again: add_custom_command()
adds a rule to generate the files passed as output, but it does not on itself add a target! If those output files are not used by any targets, then they won't be built. Here is a minimal case showing the problem:
cmake_minimum_required(VERSION 3.16)
project(proof_of_jank)
add_custom_command(
OUTPUT autogenerated.txt
COMMAND echo Hello | tee autogenerated.txt
)
add_executable(foo main.c)
$ ninja all
[1/1] Linking C executable foo
$ ls autogenerated.txt
ls: cannot access 'autogenerated.txt': No such file or directory
So what do you do if, like here, you want a command to run to generate a file, but that file is not part of the sources of an executable? You define a rule with add_custom_command()
and then depend on it in add_custom_target()
. It's not a problem that add_custom_target()
"runs" every time if it doesn't have any command but only dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be thinking "wait, does that mean RewriteCompileCommands is running every time you build?" Yes, the answer is yes. That could very much be fixed and I was tempted on patching that too, but I ultimately considered it outside of the scope of this patch.
(update) I went ahead and fixed this too in the latest version of the patch.
set(CLANGD_AUTO_SETUP ${clangd_auto_setup_default} CACHE BOOL | ||
"Install a .clangd configuration file and a compile_commands.json symlink " | ||
"in the root of the source tree to have out-of-the-box code completion " | ||
"in editors.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does clangd work with GCC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang is the only backend for clangd, however you can use gcc to compile and still use clangd. The compile arguments just may differ a bit leading to inaccurate results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes as long as clang can parse all the GCC flags being used. In the case of WebKit, this is the case, I tested it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about SVG rather than PNG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose PNG for compatibility reasons (text rendering in SVG is quite inconsistent in practice), but I can upload SVG if you prefer, or both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Update) I uploaded now versions in both PNG and SVG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a suggestion. WDYT?
- Rename update-symlink-from-candidates to update-compile-commands-symlink.
- update-compile-commands-symlink takes no arguments
- update-compile-commands-symlink reads
$top/Tools/clangd/update-compile-commands-symlink.conf
and$top/.update-compile-commands-symlink.conf
if exists. - update-compile-commands-symlink generates
$top/compile_commands.json
symlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename update-symlink-from-candidates to update-compile-commands-symlink.
I wouldn't mind.
update-compile-commands-symlink takes no arguments
I initially did it that way and ended up adding arguments. I don't remember what was the reason I switched, maybe simply because it was more honest with the build systems (since you still have to code those paths for CMake and XCode so they know when it needs to be run again. If the command takes no arguments, then it's very easy to end up with a typo or outdated file name in the dependencies of the rule which would cause the command to be ran too many times instead of immediately reporting the error.
update-compile-commands-symlink reads $top/Tools/clangd/update-compile-commands-symlink.conf and $top/.update-compile-commands-symlink.conf if exists.
That feels a bit more elegant, but I think the current approach is better for discoverability actually, and that's much more important in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename update-symlink-from-candidates to update-compile-commands-symlink.
(Update) Done. The other suggestions I didn't follow for the reasons explained in the previous comment.
${TOOLS_DIR}/clangd/compile_commands.conf.example | ||
COMMAND ${Python_EXECUTABLE} | ||
${TOOLS_DIR}/clangd/update-compile-commands-conf | ||
${CMAKE_SOURCE_DIR}/compile_commands.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to pass the path of compile_commands.conf.example as an argument?
#29313 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Update) This is done in the new version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch doesn't work on Windows. It seems that symlinks have to use backslashes on Windows. Could you fix.
https://webkit.slack.com/archives/CU84Q46JZ/p1717377736652069?thread_ts=1717142017.022589&cid=CU84Q46JZ
https://bugs.webkit.org/show_bug.cgi?id=274909 Reviewed by NOBODY (OOPS!). # Goals This patch aims to bring out-of-the box completion with clangd for WebKit developers using CMake builds. clangd is an open source language server for providing code-completion, go-to-definition and similar smart features for C++ code by analyzing it with the same backend clang uses. **You do not need to compile WebKit with clang to take advantage of this**, as clang strives to be flag compatible with GCC, and even MSVC through `clang-cl`. Using clangd with WebKit was already possible before, but required manual intervention in the project files to set it up. This patch attempts to clear some of the existing pitfalls, particularly for CMake builds, so that more people take advantage of Clangd without having to put conscious effort into troubleshooting. This patch also makes WebKit stock clangd setup compatible with both Mac and non-Mac platforms and allows developers to customize clangd configuration without dirtying their repository. --- Getting clangd right takes a lot of context and moving pieces, so this is one of those very long commit messages. Sections with (NEW) in the title explain changes introduced by this patch, whereas the rest hope to explain the background and why those changes are necessary or useful. # Architecture (NEW) Several scripts have been added in Tools/clangd. A diagram describing the build system targets, dependencies and products is also included. The clangd auto-setup needs to generate the following artifacts: * .clangd configuration file * compile_commands.json file/symlink in the WebKit repo root. Starting with this patch `.clangd` is no longer committed in the repository it is now .gitignore'd and auto-generated during the build. # Compatibility with XCode builds (NEW) This patch doesn't improve or worsen the status of clangd support in XCode builds. XCode build users are still expected to generate compile_commands.json manually as per the wiki states: https://trac.webkit.org/wiki/Clangd#macOS1 make release EXPORT_COMPILE_COMMANDS=YES generate-compile-commands WebKitBuild/Release The generation of the `.clangd` file is also included in the XCode build, as part of the WTF XCode project. # clangd config generation (NEW) The script `update-clangd-config` autogenerates a `.clangd` file with an «autogenerated» marker at the top. The file will only be updated as long as that marker exists, therefore allowing end-user customization if needed. The `.clangd` file is generated from a Jinja template, which will produce slightly different options for Mac and non-Mac platforms, fixing the regression introduced in WebKit#23273. `update-clangd-config` is made part of the build by default. For XCode builds, it is generated as part of WTF. For CMake builds, it is enabled as a default target as long as CLANGD_AUTO_SETUP is set, which is the default when building inside the WebKitBuild directory. # clangd configuration changes (NEW) This patch also makes several changes to the clangd configuration file to fix issues detected during testing: * UnifiedSource files are no longer indexed. * The `-fcoroutines` flag is stripped, as it is not recognized by some versions of clang, and by extension, clangd. * `-xobjective-c++` is now only passed in macOS to avoid depending on macOS-only headers in other systems. * `config.h` is no longer prepended to files within the ThirdParty directory. # compile_commands.json generation: context CMake already generates a compile_commands.json file when the Ninja or Makefile file is generated as long as CMAKE_EXPORT_COMPILE_COMMANDS is ON, which is already the default in DEVELOPER_MODE. When using unified builds (which is the default), that `compile_commands.json` is somewhat subpar, as the language server is unaware of most of the source .cpp files and hence has to require to heuristics to match them to other build commands. In XCode builds, the `generate-compile-commands` scripts already includes logic to resolve unified builds. On the CMake side, a RewriteCompileCommands target exists that parses that file along with unified sources to generate an improved file at `<BUILD_DIR>/DeveloperTools/compile_commands.json`. Unfortunately, this may not be obvious to many developers, which may be unaware of its existence. # Same compile_commands.json path for non-unified builds (NEW) The first change this patch makes is solving the unified builds vs non-unified builds dichotomy by introducing a new build target for non-unified builds named SymlinkCompileCommandsInDeveloperTools. This way, **no matter the type of build**, `<BUILD_DIR>/DeveloperTools/compile_commands.json` is the path to be relied upon. # compile_commands.json vs builds For a `compile_commands.json` file to be used, it needs to exist in a parent directory of the sources being worked on. Since any generated `compile_commands.json` is specific for a given build (e.g. Debug vs Release), with build-specific flags, developers need to make a choice of what build to use for the `compile_commands.json` in their WebKit checkout. In XCode builds, this is passed explicitly to the generate-compile-commands command. In CMake builds, this is usually accomplished by symlinking the file into the root of the WebKit repository. Developers who frequently switch and remove builds need to be careful to not let the compile_commands.json symlink become broken, as that would cause the language server to rely on much worsened heuristics. # compile_commands.json symlink autogeneration and management (NEW) This patch introduces automatic compile_commands.json symlink handling. A new target is introduced that intentionally runs with every build: `UpdateCompileCommandsSymlink`, which relies on the `update-compile-commands-symlink` script, also introduced by this patch. The script is meant to be fast enough to not introduce any perceivable delay in builds. `update-compile-commands-symlink` receives as arguments a symlink path and a YAML file listing a series of candidate paths. The symlink is updated to point to the first candidate path that exists. This list of candidate paths is declared in a new .gitignore'd `compile_commands.conf` file that is generated early during any CMake build that uses `CLANGD_AUTO_SETUP`. `compile_commands.conf` will be automatically updated from `Tools/clangd/compile_commands.conf.example`; but similarly to `.clangd`, a user can disable this and manage the file on their own, in this case by setting the `user_managed` property inside the file to True. # Testing clangd auto-setup has been tested with a Linux x86_64 machine using webkit-container-sdk and Visual Studio Code, both with the default GCC and with clang 16, with a fresh installation of VS Code (no settings). It is possible to confirm if a file passes under clangd by using the `clangd --check` command along with the compile lines used during compilation, which can be queried from compile_commands.json. A couple barebones scripts showing this usage have been provided in Tools/clangd as documentation: `clangd-check-file` and `clangd-check-codebase`. I was able to run clangd-check-codebase with very few .cpp files failing, and the failing files had good reasons for failing, like missing includes that were obscured by unified builds.
06a7bb0
to
b506202
Compare
EWS run on current version of this PR (hash b506202) |
(Update) This should now be fixed. |
Summary of the changes in this version:
|
b506202
b506202