-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 specific dependency handling for clang #13134
base: master
Are you sure you want to change the base?
Conversation
The poorly named `print_tool_versions()` doesn't just print the tools versions, it finds them and populates a global table, without which some tests will fail. Rename the function and add a `report` argument so that calls can decide whether they want to have the printed message, because the single runner doesn't in quick mode.
Clang has two ways to be found, CMake, and by hand. Clang has some hurdles of use because of the way it's installed on many Linux distros, either in a separate prefix, in a prefix with LLVM, or in a common prefix, which requires some amount of effort to make it work. TODO: tests for C++ TODO: fill out skip expectations on various CI platforms…
81506a3
to
ab5966d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty reasonable so far.
Both libclang (the C interface) and the C++ interfaces are supported via the | ||
`language` keyword. The default is to search for the `C++` interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we can't do this for Rust dependencies, @xclaesse.
Clang modules are supported, and must be passed in the format `clangBasic`, with | ||
proper capitalization and the `clang` prepended. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add an example of how to use modules above ^
|
||
modules = stringlistify(extract_as_list(kwargs, 'modules')) | ||
if not modules and language == 'cpp': | ||
mlog.warning('Clang dependency without modules works correctly for dynamically linked Clang, ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing on 'clang' seems inconsistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment here would be good because I am not understanding the language check
# Version.h is a C++ header, and this will fail if we look | ||
# for clang-c. The inc is just the basic | ||
version = self.clib_compiler.get_define('CLANG_VERSION', '#include <clang/Basic/Version.inc>', env, lib, self.ext_deps)[0] | ||
if not self.version_reqs or (mesonlib.version_compare_many(version, self.version_reqs)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraneous parens?
if not modules: | ||
return | ||
if language != 'cpp': | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe merge the check?
/cc @thesamesam |
Clang is fun. It has a C API library and C++ API libraries. It has the following configurations that need to be accounted for:
On top of that, to get the Clang Version for the C API you need to resort to some trickery. I've checked the implementation I came up with for clang 6-17 and it works for all of those. For the C++ API it's easy, because the header required is a C++ header.
TODO: