Page MenuHomePhabricator

[LibTooling] Allow compilation database to explicitly specify compilation database file and source root
Needs RevisionPublic

Authored by zturner on Aug 17 2016, 11:43 AM.



Allow explicit specification of a compilation database file and source root.

While trying to create a compilation database test for D23455, I ran into the problem that compilation database tests require shell. Shell tests are inefficient and cumbersome for a number of reasons. For one it takes a long time reading the test to understand what it does. Secondly it makes it harder to modify the test. Third creating many processes as is common with a shell test is slow on Windows.

I decided to solve this by introducing a new command line parameter and slightly enhancing the functionality of an existing one, detailed below:

  1. There was no way to specify an arbitrary compilation database. At best you could specify the directory of a compilation database, but it would still be forced to look for compile_commands.json. This is an inflexible design when you want to have multiple tests which each use their own compilation database. You could put them in different folders and call each one compile_commands.json, but that leads to many different directories with only one file, which is kind of silly.

The -p option let you specify a build path, which would be the folder where a compilation database existed, but it did not let you specify a *specific file* to use as a compilation database. I improved expanded this behavior of this option to check if the specified path is a regular file or a directory. If it is a regular file it doesn't search for a compilation database, and instead just uses the file you specify. The nice thing about this is that now we can put many compilation databases for various tests in the same directory, and have the run line of the test reference the exact compilation database needed for the test.

  1. Instead of a "real" compilation database, the test consisted of a template. And the test used shell in order to invoke sed to modify the template so that the files and directories would be correct. So the test would output the modified template into the CMake output directory, and then it had to use more shell commands to copy the source files over to the CMake output directory so that they would be in the right hierarchy in order to be found by the relative paths written to the compilation database.

In order to remove the dependency on shell commands, we need a way to reference the source files in their original location in the source tree, and not require them to be copied to the output tree. To do this I introduced a new command line option -r that lets you specify the root at which relative paths are resolved against.

These two things combined eliminate the need to modify the compilation database with sed and write it to the output folder because we can write *actual* source code on disk and check it in for the tests, and the compilation database can refer to these files using actual relative paths. Then because of #1, the run line can point directly to the compilation database in question.

I re-wrote the compilation database test to use this new method, and it now no longer depends on shell. I will upload the re-written test in a separate patch since I can't do cross-repo diffs.

After applying this patch, the compilation database test can be written using a single run line. A patch that does exactly that is in D23533

Diff Detail

Event Timeline

zturner updated this revision to Diff 68391.Aug 17 2016, 11:43 AM
zturner retitled this revision from to [LibTooling] Allow compilation database to explicitly specify compilation database file and source root.
zturner updated this object.
zturner added reviewers: djasper, alexfh, klimek.
zturner added a subscriber: cfe-commits.
zturner updated this revision to Diff 68404.Aug 17 2016, 1:09 PM
zturner edited edge metadata.

Added full context diff.

alexfh requested changes to this revision.Aug 18 2016, 12:50 PM
alexfh edited edge metadata.

As discussed offline, since the functionality being added in this patch is only useful for tests, the related simplification of tests seems not worth the added complexity.

This revision now requires changes to proceed.Aug 18 2016, 12:50 PM