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

Generate compilation database on load #285

Merged
merged 15 commits into from
Jun 26, 2024
Merged

Conversation

lionel-
Copy link
Member

@lionel- lionel- commented Jun 25, 2024

LSPs like clangd provide intellisense features for native files but they need to know the build flags (mostly include flags) for each root file in the project (flags for included files are inferred from root files). These tools are not able to infer the flags by themselves and so require the generation of a "compilation database": https://clang.llvm.org/docs/JSONCompilationDatabase.html.

For R packages the flags are generated by R CMD build and depend on the package Makevars file, possibly generated from configure. Generating the database after a load_all() seems like a reasonable update point to keep track of the changing source files (which can be added or removed during development) while ensuring the presence of an up-to-date Makevars file.

There are two main approaches for the generation of a compilation database. The first is to intercept the compiler calls during an actual build to collect arguments. This is the approach used by tools like bear and it works great in conjunction with R CMD build. The other is to generate build commands, e.g. with make --dry-run.

@kevinushey suggested we could use the latter approach with R CMD shlib --dry-run. This is a nice idea because it means we don't need any external tools to create the database. This PR implements this approach. A few caveats:

  • For some reason the shlib command does not add include flags for LinkingTo dependencies, so I had to inject those manually.

  • In addition to --dry-run we need to pass --always-make to make. See sneak '--always-make' into R CMD SHLIB invocation rstudio/rstudio#11917 for the motivation and the workaround reused here.

  • We need to pass target source files to shlib. These can easily be inferred when Makevars does not override OBJECTS. When it is overridden, we need to retrieve the contents of this variable, and then match the object files to potential sources. This sometimes clashes, e.g. in igraph there are a .c and .f files that match a single object file. We just select one file in this case and ignore the others. This should not be a big deal and should be a rare occurrence.

  • There could be make directives interspersed between compile commands in the dry-run output (this happens with rlang). To detect actual commands, we simply select all lines starting with a compiler command. These commands (CC, CXX, FC, etc) could be retrieved from R CMD config but this tool seems extremely slow. Instead we retrieve them from the makefiles manually. These commands should be in the same order as the source files passed to shlib. A consistency check verifies that we have as many commands as source files and that each command indeed references the corresponding source file (typically in a -f flag).

The compilation database is not generated by default, the user must explicitly opt in by inserting this line in their DESCRIPTION file:

Config/devtools/compilation-database: true

In addition the compile_commands.json file generated at the root should be gitignored and Rbuildignored. We'll provide a usethis function to accomplish these tasks.

The tools for generating this database should probably be moved and reexported from pkgbuild in the future (cc @gaborcsardi), but I'm implementing them here as I'd like to include the feature in the next release of pkgload.

Untested on Windows but should work on Unixes. Tested on macOS with readxl, rlang, igraph, and r-tree-sitter.
Edit: now working on Windows

@lionel- lionel- requested a review from DavisVaughan June 25, 2024 13:27
@lionel- lionel- force-pushed the feature/compilation-database branch from 839321a to 9ea4dc9 Compare June 25, 2024 15:38
Copy link
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

Works on macOS for many packages for me, works on Windows for vctrs with the newest changes!

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/compilation-db.R Outdated Show resolved Hide resolved
R/compilation-db.R Outdated Show resolved Hide resolved
@lionel- lionel- force-pushed the feature/compilation-database branch from 2317247 to b4f4557 Compare June 25, 2024 21:03
@lionel-
Copy link
Member Author

lionel- commented Jun 26, 2024

I've added a currently unexported util to help set this up: pkgload:::use_compilation_db()

@lionel- lionel- force-pushed the feature/compilation-database branch from 743b170 to 5d6a2a1 Compare June 26, 2024 09:17
@lionel- lionel- merged commit 621f693 into main Jun 26, 2024
13 checks passed
@lionel- lionel- deleted the feature/compilation-database branch June 26, 2024 09:29
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