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 API to Record Compile Invocation #682

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

Conversation

schrieveslaach
Copy link

This commit adds API to record the compilation invocations in order to
emit compile_commands.json a.k.a. JSON compilation database.

Fixes #497

See following screencast that uses Neovim with rust-analyzer and clangd as language servers:

output

@dot-asm
Copy link
Contributor

dot-asm commented Jun 1, 2022

Not that I have an actual say, or that Alex wants to have one now, but consider #612 (comment). Not even Something as simple as which was frown upon:-) But in either case, I for one would argue that [at the very least] making it optional would be more than appropriate.

@schrieveslaach schrieveslaach force-pushed the compile-commands branch 2 times, most recently from 67d9f9e to 4f67fe1 Compare July 4, 2022 08:37
@schrieveslaach
Copy link
Author

Not that I have an actual say, or that Alex wants to have one now, but consider #612 (comment). Not even Something as simple as which was frown upon:-) But in either case, I for one would argue that [at the very least] making it optional would be more than appropriate.

I switched to tinyjson to reduce compile times and I also added a feature flag so that user can opt in.

@schrieveslaach
Copy link
Author

@HKalbasi, as you have the need to use compile_commands.json (see #691), do you mind to test this PR and provide some feedback?

@schrieveslaach schrieveslaach force-pushed the compile-commands branch 3 times, most recently from ba1a308 to 65c867e Compare July 7, 2022 07:41
@HKalbasi
Copy link
Member

HKalbasi commented Jul 8, 2022

Hi @schrieveslaach! unfortunately I'm using cc indirectly, and I'm totally newbie in c/c++ tooling, so it is not easy for me to experiment with this PR. But the result in the GIF looks great.

I just have a (probably newbie) question: would this design work if some project needs multiple calls to .compile? Or each call to .recorded_compile will override the previous dump?

@schrieveslaach
Copy link
Author

@HKalbasi, thanks for the response.

would this design work if some project needs multiple calls to .compile? Or each call to .recorded_compile will override the previous dump?

recorded_compile return a Vec<CompileCommand> which you have to pass to store_json_compilation_database to store the file as JSON. If you have multiple calls to recorded_compile you can collect the results in a single Vec<CompileCommand> before passing them to store_json_compilation_database.

Does that clarify your question?

@schrieveslaach schrieveslaach force-pushed the compile-commands branch 3 times, most recently from c3c46ce to 4eba480 Compare July 23, 2022 07:44
This commit adds API to record the compilation invocations in order to
emit compile_commands.json a.k.a. JSON compilation database.

Fixes rust-lang#497
@schrieveslaach
Copy link
Author

@m-ou-se, @Mark-Simulacrum, you seem to be the maintainers of the project. Do you mind to have a look at this PR?

@HKalbasi
Copy link
Member

Does that clarify your question?

Yes. I thought .recorded_compile would generate the file on its own. But in this way everything is fine.

It might make sense to have some wrapper type over Vec<CompileCommands> with a .generate(file_name) method for convenience. Not really important, just something that might worth considering after this PR gets some attention from maintainers.

And thanks for this PR!

@dot-asm
Copy link
Contributor

dot-asm commented Jul 25, 2022

Wouldn't it be more natural to have .record(<path-to-compilation-database>) method, as opposed to making users replace .compile() methods and perform extra steps actually saving the database?

BTW, is it correct understanding that the file name is actually fixed, compile_commands.json? So that the only variable is the target directory, right? But is it actually? I mean the file is surely searched for in specific locations. For example it's suggested that clangd starts in the directory where the source file is and walks up the tree. This sounds like it would be possible and even appropriate to infer even the destination directory for the database... In other words I wonder if it could be just .record(), or .store_compile_database(), or .save_compile_commands(), or... Or even more radical, why additional methods at all, just enable the feature and leave build.rs as is... Well, it all might be just wishful thinking, in which case my apologies for unnecessary distraction :-)

@schrieveslaach
Copy link
Author

I chose to add store_json_compilation_database as a separate function because if you have multiple calls to Build::recorded_compile you can collect them and store them in one file.

I checked the clangd docs and clangd searches for compile_commands.json in the following way:

We first check for a compilation database in the directory containing the source file, and then walk up into parent directories until we find one.

Running clangd --help reveals:

--compile-commands-dir= - Specify a path to look for compile_commands.json. If path is invalid, clangd will look in the current directory and parent paths of each source file

So, I assume what I could do is that recorded_compile could write compile_commands.json into the current directory (might be changed optionally, e.g. Build::compile_database_directory(path)). And if the file already exists than it can check if there is already an entry for the particular file and if so it updates it.

@dot-asm, @m-ou-se, @Mark-Simulacrum, @HKalbasi what do you think about that approach?

@GopherJ
Copy link

GopherJ commented May 8, 2023

looking forward to using this pr!

@NobodyXu
Copy link
Collaborator

Having an api for recording cmds run is awesome, though IMO the API should be as simple as possible:

Just let the users specify a callback Fn(cmd: &Command) -> () and let them decide how to record it.

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.

Emit compile_commands.json
5 participants