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

Support custom output configurations #202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chadnorvell
Copy link

@chadnorvell chadnorvell commented Jun 26, 2024

This change adds two main features:

  1. You can configure compile commands for certain targets or groups of targets to go into separate files

For embedded systems development in particular, clangd doesn't work well with compile_commands.json generated for multiple targets. For example, if you have a build target that runs on your host machine in one configuration and another that runs on device in another configuration, compile_commands.json will contain multiple conflicting compile commands for the same source files. clangd will just use the first one it finds, which may not be the one you want to use for code intelligence. It's convenient to have separate compile commands files for each target, and switch the file clangd uses depending on how you want to navigate your code.

By providing the target_collections argument, you can associate targets with named collections. Separate compile commands files will be generated for each of the specified collections, and will be placed in subdirectories with the specified names. This is most useful when you associate multiple targets with a collection, for example, to configure code intelligence to use the compile commands for all of the targets that build for one architecture or device.

This means that you can now specify a target more than once, generating compile commands for builds of the same target but with different flags (e.g. --platform). Before, you implicitly could only specify each target once since the targets were dict keys.

  1. You can specify a different output path

If you are generating multiple compile commands files, its preferable not to output them into the workspace root. So you can specify a separate output path, relative to the workspace root.

This patch doesn't change any existing behavior; if you don't add either of the new arguments to your invocation of refresh_compile_commands, everything will work exactly as it did before.

@cpsauer
Copy link
Contributor

cpsauer commented Jun 27, 2024

Hey Chad! A good improvement. Thanks!

IMO the best solution here might be for clangd to properly handle multiple commands per file, like Xcode does. Filed an issue over there a few years ago, clangd/clangd#681, that I think is the canonical one. Lmk if we might be able to get the Googlers over there to tackle. Last I was over there the development seemed to be pretty google-driven. Sam McCall, @sam-mccall would be the guy to contact, I think.

(Need to run for the moment, but I'll get back to this soon.)

@chadnorvell
Copy link
Author

chadnorvell commented Jun 27, 2024

I agree it would be ideal for clangd to handle this, your issue describes the problem well. Beyond errors and suggestions, this impacts code navigation. In Pigweed, we use a facades/backend architecture, and it's really useful to follow the source trail behind a particular interface and see what concrete code is executed on a particular platform. Or to see constexprs evaluated in the editor with values that match the target platform (like memory alignment, data type sizes, etc.). In those cases, simply getting a union or intersection wouldn't be sufficient, and it's better to narrow down the scope of compile commands to a particular target.

@cpsauer
Copy link
Contributor

cpsauer commented Jun 28, 2024

Hey, sorry, Chad. Most of the last 24h turned into taking care of a temporarily digestively ill family member 🥴

Good points on the other browsing features. I tried to edit the clangd issue to better factor those in--but found myself thinking that one would probably want the union for other browsing features, too, right? That is, if there are multiple compile commands for different platforms, show both implementations for jump-to-definition, both ifdef branches for highlighting/analysis, both constexpr values, etc. If so, that's now reflected over there. If not, lmk so I can improve it further.

Of course, this tool should still support creating great, focused compile commands databases when people want it. Didn't mean to be implying otherwise--just wanted to quickly tap back, alerting that the best cross-platform development experience probably requires those fixes in clangd, and that you might want to engage Google resources there, too, since that might be on the critical path to a great developer experience. Otherwise clangd is just providing a single-platform view on cross-platform files, after all.

@cpsauer
Copy link
Contributor

cpsauer commented Jun 28, 2024

Back to the main issue of supporting focused compilation databases: We'd settled into the same facades/backend pattern for cross-platform code, so I've got some firsthand understanding, but we solved things a little differently. So I want to make sure I'm fully understanding the need you're running into, so we get to the best implementation that meets your needs, too. I'm certainly open to this one, but I think it's worth a quick sync to make sure I don't have something even better for you and so that I understand your needs into the future.

In particular we were seeing: (1) code understanding/navigation for the wrong platform impeding editing (e.g. wrong ifdefs greyed out, wrong jump-to-definition through facade) (2) incomplete errors, especially when accidentally using platform-specific features (caught later by builds/tests). Does that sound the same?

As clangd workarounds, we'd found ourselves: (a) listing the targets in priority order (i.e. host/test last) and (b) having multiple refresh_compile_command targets with different orderings (or more focused subsets) that we'd occasionally run when we were focusing on specific platforms (e.g. if we were focused on developing for "my_other_microcontroller" instead of "my_default_board"). When we'd rerun those other orderings, it'd replace the existing compilation database, and the changes would get hot reloaded by clangd and apply, with no additional config needed. The tool stayed fast because of its internal caching. And if we did end up browsing files for other platforms, during that session, well, they were still there further down in the ordering.

I'm imagining that there's something that breaks that solution in your case--or that makes the multiple files necessary--and that I should probably understand. (Similarly, I'm imagining there's something motivating putting the dictionary in one target rather than the simpler design of having multiple targets, each specifying an output path.) I should say also that there's the case where a single target compiles a given file multiple times (like for a host tool that's also in the shipped binary.)

Happy also to hop on a GVC to quickly align if that'd be faster (or more fun!) than typing things out. Friday afternoon? You know how to reach me :)

Chris

@chadnorvell chadnorvell force-pushed the split-files branch 16 times, most recently from 7a3be95 to 1a7cde6 Compare July 13, 2024 03:32
@chadnorvell
Copy link
Author

I've been tinkering with this a bit and I think the new version is a better solution. We've been using it internally and it's been really useful!

Let's catch up on this next week.

This change adds two main features:

1. You can configure compile commands for certain targets or groups of
   targets to go into separate files

For embedded systems development in particular, `clangd` doesn't work
well with `compile_commands.json` generated for multiple targets. For
example, if you have a build target that runs on your host machine in
one configuration and another that runs on device in another
configuration, `compile_commands.json` will contain multiple conflicting
compile commands for the same source files. `clangd` will just use the
first one it finds, which may not be the one you want to use for code
intelligence. It's convenient to have separate compile commands files
for each target, and switch the file `clangd` uses depending on how you
want to navigate your code.

By providing the `target_collections` argument, you can associate
targets with named collections. Separate compile commands files will be
generated for each of the specified collections, and will be placed in
subdirectories with the specified names. This is most useful when you
associate multiple targets with a collection, for example, to configure
code intelligence to use the compile commands for all of the targets
that build for one architecture or device.

This means that you can now specify a target more than once, generating
compile commands for builds of the same target but with different flags
(e.g.  `--platform`). Before, you implicitly could only specify each
target once since the targets were dict keys.

2. You can specify a different output path

If you are generating multiple compile commands files, its preferable
not to output them into the workspace root. So you can specify a
separate output path, relative to the workspace root.

This patch doesn't change any existing behavior; if you don't add either
of the new arguments to your invocation of `refresh_compile_commands`,
everything will work exactly as it did before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants