Skip to content

Make add_clang_tool add a dependency on checkedc-headers.#1112

Merged
sulekhark merged 2 commits into
checkedc:masterfrom
correctcomputation:checkedc-headers-dep
Jul 23, 2021
Merged

Make add_clang_tool add a dependency on checkedc-headers.#1112
sulekhark merged 2 commits into
checkedc:masterfrom
correctcomputation:checkedc-headers-dep

Conversation

@mattmccutchen-cci

Copy link
Copy Markdown
Member

This ensures that building any Clang tool will automatically copy the Checked C headers to the build directory along with the Clang built-in headers (the clang-resource-headers target). The tool we currently care about is clangd, but it's good to have a generic solution in case some user tries to use some other tool in the repository that might be applicable to Checked C code (maybe clang-tidy?).

Clean up code that is now redundant:

  • Remove explicit clang -> checkedc-headers dependency, since clang is defined via add_clang_tool. The clang -> clang-resource-headers dependency was already redundant, but that's an upstream Clang problem and doesn't merit introducing a diff from upstream.

  • Migrate 3c from add_clang_executable to add_clang_tool, so we can remove its explicit dependencies on checkedc-headers and clang-resource-headers. A quick look at which targets in the repository use add_clang_executable versus add_clang_tool suggests that 3c should probably be using add_clang_tool anyway. In fact, it appears that clang/tools/3c/CMakeLists.txt may have been copied from clang/tools/clang-check/CMakeLists.txt back when clang-check used add_clang_executable; clang-check later switched to add_clang_tool.

There are some targets in the repository that depend on clang-resource-headers by means other than add_clang_tool. Maybe they should all depend on checkedc-headers too, but I'm unsure I can make that change safely, so I'm not doing it unless/until we find a need for it.

Testing: If I modify a checked header file and then run ninja clang, ninja 3c, or ninja clangd, the header file is copied to the build directory, as expected. Given that that works, I think it's extremely unlikely that this change to build dependencies would break any tests, so I'm not running any more tests on my side.

This ensures that building any Clang tool will automatically copy the
Checked C headers to the build directory along with the Clang built-in
headers (the clang-resource-headers target). The tool we currently care
about is clangd, but it's good to have a generic solution in case some
user tries to use some other tool in the repository that might be
applicable to Checked C code (maybe clang-tidy?).

Clean up code that is now redundant:

- Remove explicit `clang` -> `checkedc-headers` dependency, since
  `clang` is defined via add_clang_tool. The `clang` ->
  `clang-resource-headers` dependency was already redundant, but that's
  an upstream Clang problem and doesn't merit introducing a diff from
  upstream.

- Migrate `3c` from add_clang_executable to add_clang_tool, so we can
  remove its explicit dependencies on `checkedc-headers` and
  `clang-resource-headers`. A quick look at which targets in the
  repository use add_clang_executable versus add_clang_tool suggests
  that `3c` should probably be using add_clang_tool anyway. In fact, it
  appears that clang/tools/3c/CMakeLists.txt may have been copied from
  clang/tools/clang-check/CMakeLists.txt back when clang-check used
  add_clang_executable; clang-check later switched to add_clang_tool.

There are some targets in the repository that depend on
clang-resource-headers by means other than add_clang_tool. Maybe they
should all depend on checkedc-headers too, but I'm unsure I can make
that change safely, so I'm not doing it unless/until we find a need for
it.
@dtarditi

Copy link
Copy Markdown
Member

This looks good to me, thank you! Could you resolve the conflict, and I'll merge this.

@mattmccutchen-cci

Copy link
Copy Markdown
Member Author

Thanks for the review. I resolved the conflict.

@sulekhark

Copy link
Copy Markdown
Contributor

I have started the tests on ADO in order to merge this PR. Again, this is not reflected here because I had to merge master before starting the tests.

@sulekhark

Copy link
Copy Markdown
Contributor

All the five Dev tests on ADO are successful.

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.

3 participants