Skip to content
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] Generate a usable compile_commands.json #16718

Conversation

TingPing
Copy link
Contributor

@TingPing TingPing commented Aug 15, 2023

@TingPing TingPing self-assigned this Aug 15, 2023
@TingPing TingPing added the CMake Bugzilla component for CMake build system changes label Aug 15, 2023
@TingPing TingPing requested a review from a team August 15, 2023 20:15
@TingPing TingPing force-pushed the eng/CMake-Generate-a-usable-compile_commands-json branch from d3d2671 to 0f0aa58 Compare August 15, 2023 20:19
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 15, 2023
@TingPing TingPing removed the merging-blocked Applied to prevent a change from being merged label Aug 15, 2023
@TingPing TingPing force-pushed the eng/CMake-Generate-a-usable-compile_commands-json branch from 0f0aa58 to 6ba926a Compare August 15, 2023 22:21
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

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

I would add a --help argument, even though the script is probably not intended to be called manually. Somebody else will thank you for it a few years from now....

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 16, 2023
@TingPing
Copy link
Contributor Author

TingPing commented Aug 16, 2023

help is automatically added by argparse.

I’ll fix the windows failure though.

@TingPing TingPing removed the merging-blocked Applied to prevent a change from being merged label Aug 16, 2023
@TingPing TingPing force-pushed the eng/CMake-Generate-a-usable-compile_commands-json branch from 6ba926a to 9463525 Compare August 16, 2023 14:29
@TingPing TingPing added the merge-queue Applied to send a pull request to merge-queue label Aug 16, 2023
https://bugs.webkit.org/show_bug.cgi?id=260218

Reviewed by Michael Catanzaro.

When using unified builds CMake generates a compile_commands.json
that isn't useful by many tools as it lacks much of the sources.

This adds a script to expand UnifiedSources into the full list
of sources. It outputs it to webkit_compile_commands.json.

* Source/cmake/WebKitCommon.cmake:
* Tools/Scripts/rewrite-compile-commands: Added.

Canonical link: https://commits.webkit.org/266947@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/CMake-Generate-a-usable-compile_commands-json branch from 9463525 to 5c9dd47 Compare August 16, 2023 15:23
@webkit-commit-queue
Copy link
Collaborator

Committed 266947@main (5c9dd47): https://commits.webkit.org/266947@main

Reviewed commits have been landed. Closing PR #16718 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 5c9dd47 into WebKit:main Aug 16, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 16, 2023
@philn
Copy link
Member

philn commented Aug 25, 2023

I can't find out how to specify a custom file to clangd, seems like it really expects a file named compile_commands.json... Could we store this custom webkit file in a sub-directory as a workaround?

@TingPing
Copy link
Contributor Author

Could we store this custom webkit file in a sub-directory as a workaround?

Yes I think this would be a good idea as I've hit other tools being hardcoded as well.

})
break
else:
print('Failed to find', include_path)
Copy link
Member

Choose a reason for hiding this comment

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

This triggers for many files here, *MessageReceiver.cpp that don't exist and files from WebCore/DerivedSources/ mostly.

Is this OK?

Copy link
Contributor Author

@TingPing TingPing Aug 26, 2023

Choose a reason for hiding this comment

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

Hmm in my local builds it found everything. What was your specific build config (and which commit maybe)?

Copy link
Contributor Author

@TingPing TingPing Aug 26, 2023

Choose a reason for hiding this comment

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

Also was it ran after everything was built? You can run ninja webkit_compile_commands.json after a build to run it last.

We are kinda in a weird situation where this script technically depends on every generated source, maybe we could somehow actually collect that list, but I don't know a simple solution... So maybe it runs out of order sometimes.

I didn't want to rely on checking for files existing but the rules of where a file ends up didn't have a clear pattern, but maybe some static mapping is possible.

Copy link
Member

@philn philn Aug 27, 2023

Choose a reason for hiding this comment

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

Just doing a clean GTK Debug build triggers the issue here, with sccache enabled, (edit: with ASan enabled too) the script runs early on, I suppose before the MessageReceiver.cpp files have been generated.

Copy link
Member

Choose a reason for hiding this comment

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

Actually those MessageReceiver.cpp files are for features we don't enable, such as GPUProcess, so it's expected that they shouldn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually those MessageReceiver.cpp files are for features we don't enable, such as GPUProcess, so it's expected that they shouldn't exist.

The file has to exist in some include path, it is compiled, otherwise the build would fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Bugzilla component for CMake build system changes
Projects
None yet
6 participants