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
[POC] MesonToolchain and GNU flags coming from dependencies #11557
Changes from 4 commits
a3a8f8e
7f774ba
005b591
5bc83e6
b0e8b32
bf54fec
8e98884
6cbccf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,6 @@ def _format_frameworks(self, frameworks, is_path=False): | |
os_ = self._conanfile.settings.get_safe("os") | ||
if not frameworks or not is_apple_os(os_): | ||
return [] | ||
# FIXME: Missing support for subsystems | ||
compiler = self._conanfile.settings.get_safe("compiler") | ||
if str(compiler) not in self._GCC_LIKE: | ||
return [] | ||
|
@@ -60,15 +59,14 @@ def _format_frameworks(self, frameworks, is_path=False): | |
def _format_include_paths(self, include_paths): | ||
if not include_paths: | ||
return [] | ||
# FIXME: Missing support for subsystems | ||
return ["-I%s" % (self._adjust_path(include_path)) | ||
pattern = "/I%s" if is_msvc(self._conanfile) else "-I%s" | ||
return [pattern % (self._adjust_path(include_path)) | ||
for include_path in include_paths if include_path] | ||
|
||
def _format_library_paths(self, library_paths): | ||
if not library_paths: | ||
return [] | ||
# FIXME: Missing support for subsystems | ||
pattern = "-LIBPATH:%s" if is_msvc(self._conanfile) else "-L%s" | ||
pattern = "/LIBPATH:%s" if is_msvc(self._conanfile) else "-L%s" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true that these ones were a bug if the compiler was MSVC: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what exactly was a bug? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SSE4 Now, the Meson test is passing because of this change. Anyway, as you already know, the Meson bug is still there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. just in case, for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, if they're valid both of them, then, let's use the forward slash. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I have no objections. I'd prefer the way it works with meson. |
||
return [pattern % self._adjust_path(library_path) | ||
for library_path in library_paths if library_path] | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from conan.tools.meson.toolchain import MesonToolchain | ||
from conan.tools.meson.meson import Meson | ||
from conan.tools.meson.mesondeps import MesonDeps | ||
from conan.tools.meson.toolchain import MesonToolchain | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
import os | ||
|
||
from conan.tools.build import build_jobs | ||
from conan.tools.meson import MesonToolchain | ||
from conan.tools.meson.toolchain import MesonToolchain | ||
from conan.tools.meson.mesondeps import MesonDeps | ||
|
||
|
||
class Meson(object): | ||
|
@@ -15,10 +16,18 @@ def configure(self, reconfigure=False): | |
generators_folder = self._conanfile.generators_folder | ||
cross = os.path.join(generators_folder, MesonToolchain.cross_filename) | ||
native = os.path.join(generators_folder, MesonToolchain.native_filename) | ||
deps_flags = os.path.join(generators_folder, MesonDeps.filename) # extra machine files layer | ||
has_deps_flags = os.path.exists(deps_flags) | ||
|
||
if os.path.exists(cross): | ||
cmd += ' --cross-file "{}"'.format(cross) | ||
if has_deps_flags: | ||
cmd += ' --cross-file "{}"'.format(deps_flags) | ||
else: | ||
cmd += ' --native-file "{}"'.format(native) | ||
if has_deps_flags: | ||
cmd += ' --native-file "{}"'.format(deps_flags) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but it overrides, isn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it's not. They are being added as an extended part of the MesonToolchain:
So, the only part that is being overridden, it's the first definition of the constants:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure this won't be breaking. The values from multiple machine files override each other, only last file wins. If MesonDeps adds some flags, and MesonToolchain too, they will remove the previously defined one. I don't think the idiom There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is not working so. AutotoolDeps, as MesonDeps, is getting the flags from all the dependencies altogether so I'm creating only one file, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And, the test is already there for sure. That test is not passing without those deps flags. I can try to debug something from the log or try to print anything else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not a problem of one file per dependency. It will be the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this complicates local flow significantly. now, if one wants to replicate conan buid exactly, will have to type something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, you'll only have a single file for all the deps: |
||
|
||
cmd += ' "{}" "{}"'.format(build_folder, source_folder) | ||
if self._conanfile.package_folder: | ||
cmd += ' -Dprefix="{}"'.format(self._conanfile.package_folder) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
import textwrap | ||
|
||
from jinja2 import Template | ||
|
||
from conan.tools.gnu.gnudeps_flags import GnuDepsFlags | ||
from conan.tools.meson.helpers import to_meson_value | ||
from conans.model.new_build_info import NewCppInfo | ||
from conans.util.files import save | ||
|
||
|
||
class MesonDeps(object): | ||
"""Generator that manages all the GNU flags from all the dependencies""" | ||
|
||
filename = "conan_meson_deps_flags.ini" | ||
|
||
_meson_file_template = textwrap.dedent(""" | ||
[constants] | ||
c_args = {{c_args}} | ||
c_link_args = {{c_link_args}} | ||
cpp_args = {{cpp_args}} | ||
cpp_link_args = {{cpp_link_args}} | ||
""") | ||
|
||
def __init__(self, conanfile): | ||
self._conanfile = conanfile | ||
self._ordered_deps = [] | ||
# constants | ||
self.c_args = [] | ||
self.c_link_args = [] | ||
self.cpp_args = [] | ||
self.cpp_link_args = [] | ||
|
||
# TODO: Add all this logic to GnuDepsFlags? Distinguish between GnuFlags and GnuDepsFlags? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this POC is accepted, then I'll propose a refactor of the actual
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it might be too mind blowing (for users), let's not mulltiply entities and use Occam's razor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean. I'm not thinking of a new generator. GnuDepsFlags is mostly used as an internal helper class and, apart from that, its name is saying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GnuDepsFlags will not be exposed to users, it is internal |
||
@property | ||
def ordered_deps(self): | ||
if not self._ordered_deps: | ||
deps = self._conanfile.dependencies.host.topological_sort | ||
self._ordered_deps = [dep for dep in reversed(deps.values())] | ||
return self._ordered_deps | ||
|
||
def _get_cpp_info(self): | ||
ret = NewCppInfo() | ||
for dep in self.ordered_deps: | ||
dep_cppinfo = dep.cpp_info.aggregated_components() | ||
# In case we have components, aggregate them, we do not support isolated | ||
# "targets" with autotools | ||
ret.merge(dep_cppinfo) | ||
return ret | ||
|
||
def _rpaths_flags(self): | ||
flags = [] | ||
for dep in self.ordered_deps: | ||
flags.extend(["-Wl,-rpath -Wl,{}".format(libdir) for libdir in dep.cpp_info.libdirs | ||
if dep.options.get_safe("shared", False)]) | ||
return flags | ||
|
||
def get_gnu_flags(self): | ||
flags = GnuDepsFlags(self._conanfile, self._get_cpp_info()) | ||
|
||
# cpp_flags | ||
cpp_flags = [] | ||
cpp_flags.extend(flags.include_paths) | ||
cpp_flags.extend(flags.defines) | ||
|
||
# Ldflags | ||
ldflags = flags.sharedlinkflags | ||
ldflags.extend(flags.exelinkflags) | ||
ldflags.extend(flags.frameworks) | ||
ldflags.extend(flags.framework_paths) | ||
ldflags.extend(flags.lib_paths) | ||
|
||
# set the rpath in Macos so that the library are found in the configure step | ||
if self._conanfile.settings.get_safe("os") == "Macos": | ||
ldflags.extend(self._rpaths_flags()) | ||
|
||
# libs | ||
libs = flags.libs | ||
libs.extend(flags.system_libs) | ||
|
||
# cflags | ||
cflags = flags.cflags | ||
cxxflags = flags.cxxflags | ||
return cflags, cxxflags, cpp_flags, ldflags, libs | ||
|
||
def _context(self): | ||
cflags, cxxflags, cpp_flags, ldflags, _ = self.get_gnu_flags() | ||
self.c_args.extend(cflags + cpp_flags) | ||
self.cpp_args.extend(cxxflags + cpp_flags) | ||
self.c_link_args.extend(ldflags) | ||
self.cpp_link_args.extend(ldflags) | ||
|
||
return { | ||
"c_args": to_meson_value(self.c_args), | ||
"c_link_args": to_meson_value(self.c_link_args), | ||
"cpp_args": to_meson_value(self.cpp_args), | ||
"cpp_link_args": to_meson_value(self.cpp_link_args), | ||
} | ||
|
||
def _content(self): | ||
context = self._context() | ||
content = Template(self._meson_file_template).render(context) | ||
return content | ||
|
||
def generate(self): | ||
save(self.filename, self._content()) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import textwrap | ||
|
||
from conans.test.assets.sources import gen_function_cpp | ||
from conans.test.functional.toolchains.meson._base import TestMesonBase | ||
from conans.test.utils.tools import TestClient | ||
|
||
|
||
class TestMesonToolchainAndGnuFlags(TestMesonBase): | ||
|
||
def test_mesontoolchain_using_gnu_deps_flags(self): | ||
client = TestClient(path_with_spaces=False) | ||
client.run("new hello/0.1 -s") | ||
client.run("create . hello/0.1@ %s" % self._settings_str) | ||
app = gen_function_cpp(name="main", includes=["hello"], calls=["hello"]) | ||
|
||
conanfile_py = textwrap.dedent(""" | ||
from conan import ConanFile | ||
from conan.tools.meson import Meson | ||
|
||
class App(ConanFile): | ||
settings = "os", "arch", "compiler", "build_type" | ||
requires = "hello/0.1" | ||
generators = "MesonDeps", "MesonToolchain" | ||
|
||
def layout(self): | ||
self.folders.build = "build" | ||
|
||
def build(self): | ||
meson = Meson(self) | ||
meson.configure() | ||
meson.build() | ||
""") | ||
|
||
meson_build = textwrap.dedent(""" | ||
project('tutorial', 'cpp') | ||
cxx = meson.get_compiler('cpp') | ||
hello = cxx.find_library('hello', required: true) | ||
executable('demo', 'main.cpp', dependencies: hello) | ||
""") | ||
|
||
client.save({"conanfile.py": conanfile_py, | ||
"meson.build": meson_build, | ||
"main.cpp": app}, | ||
clean_first=True) | ||
|
||
client.run("install . %s" % self._settings_str) | ||
client.run("build .") | ||
assert "[2/2] Linking target demo" in client.out |
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.
Why? In theory VS also admits
-I
without problems. Maybe it is a bit more compatible if this is used by some other build tool, like autotools + cl.exe?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.
Yes, it admits both but Meson is failing because of an internal bug on its side. So this change is making life easier for Meson users and works with VS.
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.
if it's more compatible with tools (e.g. meson), let's switch