Add method to get values from config tool based dependency #2512
Conversation
mesonbuild/dependencies/ui.py
Outdated
| @@ -23,12 +23,16 @@ | |||
|
|
|||
| from .. import mlog | |||
| from .. import mesonlib | |||
| from ..mesonlib import MesonException, Popen_safe, version_compare, extract_as_list | |||
| from ..mesonlib import ( | |||
There was a problem hiding this comment.
[flake8]
- [F401] '..mesonlib.version_compare' imported but unused
mesonbuild/dependencies/dev.py
Outdated
| # to figure out for ourselves what to link with. We'll do that by | ||
| # checking in the directory provided by --libdir for a library | ||
| # called libLLVM-<ver>.(so|dylib|dll) | ||
| libs = self.get_config_value(['--libdir'], 'link_args')[0] |
There was a problem hiding this comment.
[flake8]
- [F841] local variable 'libs' is assigned to but never used
mesonbuild/dependencies/dev.py
Outdated
| expected_name = 'libLLVM-{}'.format(self.version) | ||
| re_name = re.compile(r'{}.(so|dll|dylib)'.format(expected_name)) | ||
|
|
||
| for file_ in os.listdir(libdir): |
There was a problem hiding this comment.
[flake8]
- [F821] undefined name 'libdir'
mesonbuild/dependencies/dev.py
Outdated
|
|
||
| for file_ in os.listdir(libdir): | ||
| if re_name.match(file_): | ||
| self.link_args = ['-L{}'.format(libdir), |
There was a problem hiding this comment.
[flake8]
- [F821] undefined name 'libdir'
mesonbuild/dependencies/base.py
Outdated
| # with the tools and tool_name class attributes set, this class is then | ||
| # instantiated and returned. The reduce function (method) is also | ||
| # attached, since python's pickle module won't be able to do anything | ||
| # with this dynamically generated class otherwise. |
There was a problem hiding this comment.
[flake8]
- [W291] trailing whitespace
dcbaker
force-pushed
the
wip/config-tool-variables
branch
3 times, most recently
from
a1f3b4e
to
80d6054
Compare
dcbaker
force-pushed
the
wip/config-tool-variables
branch
from
80d6054
to
6ec4109
Compare
|
I've rebased this on master, added libwmf, added a deprecation warning for the old per-tool method arguments, and fixed the libwmf test. I expect LLVM to fail on this without #2595 |
dcbaker
force-pushed
the
wip/config-tool-variables
branch
2 times, most recently
from
1bdb90f
to
7ec94d4
Compare
|
the build failures seem to be CI failures not failures with this series. |
| error('MESON_SKIP_TEST LLVM not installed.') | ||
| endif | ||
|
|
||
| inc = dep_llvm.get_configtool_variable('--includedir') |
There was a problem hiding this comment.
Should this perhaps be includedir as that is its actual name?
|
Overall this seems like a nice improvement. Now ⚔️ conflicts, so needs rebasing and something to be done to the review comment about variable names. |
dcbaker
force-pushed
the
wip/config-tool-variables
branch
3 times, most recently
from
bf9f2db
to
847dac6
Compare
|
Anything missing to merge this? I need the get_configtool_variable method for a couple of components in mesa. |
|
Asking again, why have the double dashes in the variable name to get? It would indicate a command line option rather than a variable name. |
|
Ultimately each of these tools is a one-off solution, so the only way to get variables is to pass a command line argument, since different tools provide different options. |
|
It would be nice for the interface to be consistent. Currently for pkgconfig variables you'd do but here it would be: We might even in the future add a method called, say, |
The current latest version is 0.2.8, but the test expects 3.0. 0.2.8 was released in 2011, so it seems quite safe to require the latest version.
This basically boils down to using map() and expecting a list, but map returns an iterator. The better solution is to use a list comprehension anyway, so do that.
This class is meant abstract away some of the tedium of writing a config tool wrapper dependency, and allow these instances to share some basic code that they all need.
Some dependencies can be detected multiple ways, such as a config tool and pkg-config. For pkg-config a new PkgConfigDependency is created and used to check for the dependency, config tool dependencies are handled ad-hoc. This allows the ConfigToolDependency to be used in the same way that PkgConfigDependency is.
This mirrors the get_pkgconfig_variable but for config tool based dependencies.
There are currently entries for cups and pcap; but not LLVM, GnuStep, or WxWidgets. Instead of having an entry for each of these just have a single entry for all of them, since the majority of the information is duplicated between them anyway.
dcbaker
force-pushed
the
wip/config-tool-variables
branch
from
847dac6
to
a52c22d
Compare
|
I've updated with the requested change to |
|
@jpakkane, is this looking good, or would you like to see some more changes? I'd really like to see this land for 0.44, and I'm setting aside time to make any changes you need to see to get this landed. |
What I want is to bet able to get the includepath from LLVM. What ended up happening was a lot of refactoring to get there.
This series adds a new ConfigToolDependency class, which acts as a share base class for for config tool dependencies like llvm, wxwidgets, and gnustep. It also allows this class to be used in factory type constructors (like cups and pcap). Among the features this adds is unified argument detection, support for multiple version comparisons, and a unified config-tool method name.
This is based on my previous llvm patches (that are waiting for merge already), because this code allows the LLVMDependency to be simplified further.