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

Add a bxl for generating a unified compile_commands. #510

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zjturner
Copy link
Contributor

This adds a simple bxl for generating a global compilation database for all targets in a build. It is very fast, able to generate a database for around 4,000 files in under 4 seconds.

This opens the door for external users being able to integrate with traditional LSPs like clangd.

This addresses several outstanding issues for external users, including #194, #307, and #452.

This adds a simple bxl for generating a global compilation database
for all targets in a build.  It is very fast, able to generate a
database for around 4,000 files in under 4 seconds.

This opens the door for external users being able to integrate with
traditional LSPs like clangd.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 28, 2023
@zjturner
Copy link
Contributor Author

@Overhatted

@zjturner
Copy link
Contributor Author

I'm not sure why this failed, but it doesn't appear to be related to anything in my PR. @ndmitchell do you have any suggestions for how I can fix this?

@JakobDegen
Copy link
Contributor

A fix for the CI issue landed on master, you should pick it up if you rebase.

I have to say I don't quite understand the need for aquery here. The obvious thing to me would have been to run analysis on the whole world to gather all the compilation DBs, and then just merge them. How does that approach go wrong?

@zjturner
Copy link
Contributor Author

zjturner commented Nov 29, 2023

A fix for the CI issue landed on master, you should pick it up if you rebase.

I have to say I don't quite understand the need for aquery here. The obvious thing to me would have been to run analysis on the whole world to gather all the compilation DBs, and then just merge them. How does that approach go wrong?

When I tried that (maybe I did it wrong) it was extremely slow. It was an early attempt, so my memory is rusty, but I recall that generation of all the compilation dbs was very slow, because the sub target to generate the compilation db in the first place had dependencies on other targets (didn’t investigate what) and it would build things before it was able to generate the compilation db. I don’t know what actions that “build things” step ran (didn’t investigate), I just remember that it was slow. This method generates a universe compilation db very fast.

@JakobDegen
Copy link
Contributor

because the sub target to generate the compilation db in the first place had dependencies on other targets

The only action that's run by compilation DB generation does argsfile expansion. I could maybe imagine this forcing targets in the srcs attribute or something of that sort to get built, but we should fix that if so. But if you don't need argsfile expansion, which seems to be the case here, then I think just having adding a compilation-database-unexpanded subtarget, and combining that across the entire repo, would be the right thing to do

@ndmitchell
Copy link
Contributor

So the context here is that when you generate a compilation database, you basically have two choices:

  1. Generate the database with zero dependencies. That basically ends up looking like the aquery solution.
  2. Generate the database with lots of dependencies - this ensures that things like generated code are produced properly. It also means that you can literally run the command lines produced and they work.

I believe our [compilation-database] target goes for lots of dependencies. I originally worked on a version with zero dependencies, but then our IDE teams wanted them added to support their use cases.

@JakobDegen is suggesting we have [compilation-database-unexpanded]. We probably should, but I don't think this solution is harmful, so suggest we merge it anyway?

# can do until buck2 upstream actually stores a Starlark list as the value of this attribute.
cmd = action.attrs.cmd.value()
cmd = cmd.strip("[]").split(',')
cmd = list(map(lambda x: x.strip(), cmd))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get this chunk of code pulled out into a separate function? We probably will want to do something in future to clean this up, and having it separate makes it easier to see what to do.

CC @wendy728 for how we might want to unpack cmd_args values in future.

filename = entry["file"]
if not files.contains(filename):
entries.append(entry)
files.add(filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent this line one level?

facebook-github-bot pushed a commit to facebook/buck2-prelude that referenced this pull request Feb 21, 2024
Summary:
1. I made the default platform in the prelude public so it can be inherited from. I don't know why it wasn't already like that. Maybe I'm misunderstanding how to use it
2. Not sure if I'm using the config_setting and constraint_setting properly since it seems duplicated.
3. The script to generate the compile_commands.json was more or less copied from facebook/buck2#510.

There are still a lot of improvements to be made but I think this is good enough to be merged:
1. With MSVC the compilation should probably be done with /Z7 by default and then, if the "stripped" sub-target is asked for, that flag is removed.
2. The cxx toolchain needs the compilation options looked at, not sure if Linux's defaults work properly.
3. The Install.ps1 and Copy.ps1 need to be ported to Linux.

X-link: facebook/buck2#517

Reviewed By: KapJI

Differential Revision: D53692733

Pulled By: JakobDegen

fbshipit-source-id: 8d5962084831a3cccde128f0aa0360a1aaf72004
facebook-github-bot pushed a commit that referenced this pull request Feb 21, 2024
Summary:
1. I made the default platform in the prelude public so it can be inherited from. I don't know why it wasn't already like that. Maybe I'm misunderstanding how to use it
2. Not sure if I'm using the config_setting and constraint_setting properly since it seems duplicated.
3. The script to generate the compile_commands.json was more or less copied from #510.

There are still a lot of improvements to be made but I think this is good enough to be merged:
1. With MSVC the compilation should probably be done with /Z7 by default and then, if the "stripped" sub-target is asked for, that flag is removed.
2. The cxx toolchain needs the compilation options looked at, not sure if Linux's defaults work properly.
3. The Install.ps1 and Copy.ps1 need to be ported to Linux.

Pull Request resolved: #517

Reviewed By: KapJI

Differential Revision: D53692733

Pulled By: JakobDegen

fbshipit-source-id: 8d5962084831a3cccde128f0aa0360a1aaf72004
wavewave pushed a commit to MercuryTechnologies/buck2-prelude that referenced this pull request Apr 26, 2024
Summary:
1. I made the default platform in the prelude public so it can be inherited from. I don't know why it wasn't already like that. Maybe I'm misunderstanding how to use it
2. Not sure if I'm using the config_setting and constraint_setting properly since it seems duplicated.
3. The script to generate the compile_commands.json was more or less copied from facebook/buck2#510.

There are still a lot of improvements to be made but I think this is good enough to be merged:
1. With MSVC the compilation should probably be done with /Z7 by default and then, if the "stripped" sub-target is asked for, that flag is removed.
2. The cxx toolchain needs the compilation options looked at, not sure if Linux's defaults work properly.
3. The Install.ps1 and Copy.ps1 need to be ported to Linux.

X-link: facebook/buck2#517

Reviewed By: KapJI

Differential Revision: D53692733

Pulled By: JakobDegen

fbshipit-source-id: 8d5962084831a3cccde128f0aa0360a1aaf72004
@cbarrete
Copy link
Contributor

@zjturner Did you plan on finishing this PR and getting it merged? If not, would you be ok with someone picking it up? It seems that @ndmitchell only had minor comments about this version and was otherwise ready to merge it.

@zjturner
Copy link
Contributor Author

Sorry for the lack of response, I've been gone for some time. Please feel free to take it over.

@dav1d-wright
Copy link

I have a question about this bxl script, and would be happy to help out if possible. Bear in mind I'm only just getting started with buck2 and have no buck1 experience yet.

The issues I am currently facing is with how the targets are passed to this script, namely:

  1. I somehow can't manage to pass multiple targets to the script
    a) e.g. I'd like for the compdb to be built for //:main and //:test, but I can't figure out the command-line syntax for it
  2. If I try to build the compdb for all targets, ctx.configured_targets(target_filter) fails for my demo project that uses thirdparty dependencies that have BUCK files. Details on this below.

I put together a very simple demo project, with which I am trying to get acquainted with the build system. I have integrated googletest and range-v3 at the moment, as a way to understand how to integrate thirdparty dependencies. I am considering git submodules or vendoring, where I will add BUCK files for projects that don't have them, or modify them for projects that have them but some issues arise (e.g. not supporting buck2).

The problem that I have stumbled across is with range-v3. This library does have BUCK files, and provides them also for examples contained in the repository. If I run the bxl script, I get the following error:

buck2 bxl //compile_commands.bxl:gen_compilation_database --

Traceback (most recent call last):
  File <builtin>, in <module>
  * compile_commands.bxl:68, in _gen_compilation_database_impl
      targets = ctx.configured_targets(target_filter)
error: Error in configured node dependency, dependency chain follows (-> indicates depends on, ^ indicates same configuration as previous):
       root//thirdparty/range-v3/example:accumulate_ints (prelude//platforms:default#904931f735703749)
    -> root//:range-v3 (^)

  --> compile_commands.bxl:68:15
   |
68 |     targets = ctx.configured_targets(target_filter)
   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |


Caused by:
    0: looking up unconfigured target node `root//:range-v3`
    1: Unknown target `range-v3` from package `root//`.
       Did you mean one of the 2 targets in root//:BUCK?

What I found is that these examples refer to parts of the library, expecting them to be in the root folder, which is not the case for my setup (and presumably not for most). My project is not building these example project, but it seems like they are still picked up by configured_targets for some reason, and failing.

Any help on these two issues would be greatly appreciated. I'd be happy to contribute the solution back, if something can be 🙂

cbarrete added a commit to cbarrete/buck2 that referenced this pull request Nov 15, 2024
This is a diffent approach than facebook#510. The
main differences are:
- All required dependencies, such as generated code, are materialized, so that
  tools that use the compilation database work can find those and work properly.
- Files that are included in multiple targets result in multiple entries in the
  compilation database.

Closes facebook#307
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants