[PATCH] D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

Thanks for taking a look and sorry for the confusion with naming. I should've renamed `Integration` before sending this patch.



================
Comment at: clangd/BuildSystem.h:29
+/// Default compilation database used by clangd, based on the build system.
+class Integration : public GlobalCompilationDatabase {
+public:
----------------
klimek wrote:
> 'Integration' is a bit of a weird name, as it doesn't tell me what it does.
> What I'd have expected is an abstraction like 'Workspace' (which I believe is a domain term in IDEs) that would:
> -> be in some form coupled to 1 build system
> -> provide a file system view on the workspace
> -> provide the ability to watch for changed files
> 
> Then the BuildSystem would:
> -> provide the ability to watch for changed inputs / commands
> -> provide the ability to "prepare all inputs"
> -> potentially provide the ability to get a build graph
> 
> If Integration wants to be that, it should:
> - not extend GlobalCompilationDatabase
> - not have getCompileCommand itself
> 'Integration' is a bit of a weird name, as it doesn't tell me what it does.
It is, sorry about that. This is not final. I intended to put this into the 'buildsystem' namespace, so it'd be used as 'buildsystem::Integration', which would make more sense.

> What I'd have expected is an abstraction like 'Workspace' (which I believe is a domain term in IDEs) that would:
> -> be in some form coupled to 1 build system
> -> provide a file system view on the workspace
> -> provide the ability to watch for changed files
The abstraction you're looking for is called `BuildSystem` in this patch.
It is decoupled from watching for files (this is handled by FileSystemProvider), it can watch files internally but tells the users about the changes in terms of source files that had their inputs changed.

> Then the BuildSystem would:
> -> provide the ability to watch for changed inputs / commands
> -> provide the ability to "prepare all inputs"
This is exactly what `BuildSystem` does.

> -> potentially provide the ability to get a build graph
I was thinking about this as well, but still not sure if this information is better inferred by clangd when running the compiler, so decided to leave it out until we have a use-case for that.


> If Integration wants to be that, it should:
> not extend GlobalCompilationDatabase
> not have getCompileCommand itself
There is a discrepancy between the needs of the build system implementations and what clangd has at the protocol level.
- At the protocol level, clangd operates on the files and it needs to provide features on a per-file manner: get diagnostics for a file, run code completion in a file, format a file, etc.
- At the build system implementation level, we care about different things: discovering which build system is used, watching for changes to important files (`BUILD`, `CMakeLists.txt`, `compiler_commands.json`, etc.), reporting those changes to the build system users, building the generated files, providing compile commands, adding build depenndecies, etc.

`buildsystem::Integration` serves as a layer that ties those two together and provides an interface that higher layers of clangd are more comfortable with, e.g. a file-based interface that `GlobalCompilatioDatabase` provides.

My initial goal was to **replace** the `GlobalCompilationDatabase` with `Integration` and make it a non-abstract class, making it clear it just wraps build system integration into a form that's a bit more convenient for clangd uses. This turned out to be some work (we have `OverlayCDB` and test compilation databases that rely on this interface, so it's some work and it's not clear if that would make the code simpler at the end, so I decided to postpone it for now.



================
Comment at: clangd/BuildSystemPlugin.h:31
+  /// The 'compile_commands.json' has a weight of 1.
+  virtual float weight() = 0;
+  /// Attempt to detect and load the build system that resides in \p Dir.
----------------
klimek wrote:
> Do you actually foresee more than 1 being available in a single workspace? Feels a bit premature generalization to me :)
It's not a problem with `compile_commands.json`, but I foresee that being a problem if we start supporting the actual build systems like cmake and bazel. Sadly it's not uncommon for a C++ project to have multiple build systems (in a transition phase, to support more platforms, etc).


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54952/new/

https://reviews.llvm.org/D54952