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

tooling: introduce target for generating json compilation database #17065

Merged
merged 1 commit into from Sep 6, 2021

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Aug 6, 2021

both mainstream c language servers (clangd, ccls) require a json compilation
database to function correctly.

this patch adds a makefile target which utilizes the 'bear' cli to
compile this json database.

after the database is generated all code within the bpf directory is
indexable by clangd and ccls allowing the tooling to work correctly.

Signed-off-by: Louis DeLosSantos louis@isovalent.com

both mainstream c language servers (clangd, ccls) require a json compilation
database to function correctly.

this patch adds a makefile target which utilizes the 'bear' cli to
compile this json database.

after the database is generated all code within the bpf directory is
indexable by clangd and ccls allowing the tooling to work correctly.

Signed-off-by: Louis DeLosSantos <louis@isovalent.com>
@ldelossa ldelossa added the release-note/misc This PR makes changes that have no direct user impact. label Aug 6, 2021
@ldelossa ldelossa requested a review from a team August 6, 2021 02:43
@ldelossa ldelossa requested review from a team as code owners August 6, 2021 02:43
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

As far as I known, we do not include support for specific IDE/dev tools in other parts of the build system. Should we really make an exception for this?

Also, make gen_compile_comands is more to type than the command it invokes (bear -- make).



BEAR_CLI = $(shell which bear 2> /dev/null)
gen_compile_commands:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use the actual target (compile_commands.json) here with the correct dependencies (presumably all C code in the directory)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment in pr.

@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 6, 2021

@twpayne i was keeping the naming consistent with the way you do this in the kernel: https://github.com/torvalds/linux/tree/master/scripts/clang-tools

Cant speak for other tools, but the c LSPs tie themselves to the code base tightly, so its typical, and almost mandatory that the build systems are aware of their existence. Tho, only via this json compilation database.

@christarazi christarazi added area/build Anything to do with the build, more general then area/CI kind/enhancement This would improve or streamline existing functionality. labels Aug 6, 2021
@joestringer
Copy link
Member

As far as I known, we do not include support for specific IDE/dev tools in other parts of the build system. Should we really make an exception for this?

If it's an optional dependency available to developers who need it, it seems fine to me. I don't think we're taking on a big maintenance burden from adding stuff like this. For a similar example, I added make tags a few years ago, it improves my dev workflow, and I barely even think about it.

@ldelossa ldelossa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 5, 2021
@ldelossa
Copy link
Contributor Author

ldelossa commented Sep 6, 2021

Changes only effect bpf Makefile, integration tests can be skipped.

@aditighag aditighag merged commit 49788ab into cilium:master Sep 6, 2021
@ldelossa ldelossa deleted the louis/json-compilation-db branch September 8, 2021 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Anything to do with the build, more general then area/CI kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants