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

new qmk subcommand for creating compile_commands.json for better clangd/IDE interop #10264

Closed
wants to merge 37 commits into from

Conversation

xton
Copy link
Contributor

@xton xton commented Sep 7, 2020

Description

(I'm not able to re-open #8916, so I'm filing a new PR)

I use clangd as a languageserver along with coc.vim[0]. It requires a compilation database[1] to help it figure out how to find all the required files for a given compilation pass. This PR adds a new subcommand (compiledb) which creates that for you. Now your dev environment can figure out what #include QMK_KEYBOARD_H means.

[0] https://github.com/neoclide/coc.nvim/wiki/Language-servers#ccobjective-c
[1] https://clang.llvm.org/docs/JSONCompilationDatabase.html

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@xton xton changed the title Compile commands new qmk subcommand for creating compile_commands.json for better clangd/IDE interop Sep 7, 2020
@drashna
Copy link
Member

drashna commented Sep 9, 2020

two questions. I think I've asked this before, actually ...

  1. This requires version 12 of clang, correct? If so, that may be a problem. I know that MacOS is limited to 11.0.3 on catalina.
  2. This applies to only one keyboard, at a time, correct? I'm pretty sure it's "yes", based on code and comments, but want to be 100% sure.

@drashna drashna requested a review from a team September 9, 2020 03:20
@drashna drashna added cli qmk cli command python labels Sep 9, 2020
@xton
Copy link
Contributor Author

xton commented Sep 9, 2020

  1. No, I'm using this with version 10 on mojave and it works just fine. Also, to be clear the compiledb subcommand doesn't use clang at all. It just munges output from the existing make rule. Things other than clangd can use it as well, I believe (e.g. CLion).
  2. Yes. Just one at a time. It's quite quick to rebuild though.

@xton
Copy link
Contributor Author

xton commented Sep 9, 2020

(also to be clear, I'm not using clang to build qmk. this is strictly used by my dev environment to help intellisense)

@drashna
Copy link
Member

drashna commented Sep 9, 2020

Okay, sounds good then. I just wanted to make sure.

@drashna drashna requested a review from a team September 9, 2020 03:52
@MForster
Copy link

Just some quick feedback: This seems to work well for me, and I'd like to see this merged into master.

.gitignore Show resolved Hide resolved
Co-authored-by: Michael Forster <[email protected]>
@xton
Copy link
Contributor Author

xton commented Sep 27, 2020

committed suggestion.
ptal @MForster

@xton xton removed the request for review from a team September 27, 2020 05:50
@MForster
Copy link

committed suggestion.
ptal @MForster

lgtm

@drashna drashna requested a review from a team October 4, 2020 15:52
@xton
Copy link
Contributor Author

xton commented Oct 11, 2020

Usage line added. Anything else?

ptal @skullydazed

Copy link
Member

@tzarc tzarc left a comment

Choose a reason for hiding this comment

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

Just a minor docs change, otherwise LGTM.

lib/python/qmk/cli/compiledb.py Outdated Show resolved Hide resolved
Co-authored-by: Nick Brassel <[email protected]>
@skullydazed
Copy link
Member

Apologies for not getting back to this sooner. Overall this looks good but I wasn't able to come up with a name that worked better. Normally I'd have merged it anyway but 2020 has changed a lot of routines, and I hadn't cycled around to reviews again in a while.

In the meantime a lot of our current and future work has coalesced around a naming pattern for this sort of this. Could you make one last change to fit this scheme? Calling it qmk generate-compile-commands fits our existing scheme, and isn't too bad to tab complete.

@stale
Copy link

stale bot commented Jan 30, 2021

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@stale
Copy link

stale bot commented Mar 5, 2021

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

@stale stale bot closed this Mar 5, 2021
@j-hui
Copy link

j-hui commented Jul 27, 2021

Was there anything else blocking this PR, aside from deciding on the name of the target? I'd also love to see this feature merged in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants