Skip to content
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

Merged
merged 8 commits into from Jul 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions conan/tools/gnu/gnudeps_flags.py
Expand Up @@ -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 []
Expand All @@ -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"
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what exactly was a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the -LIBPATH: as valid compile options, wasn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. just in case, for link.exe both shoud be legal:

On the command line, an option consists of an option specifier, either a dash (-) or a forward slash (/), followed by the name of the option. 

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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]

Expand Down
3 changes: 2 additions & 1 deletion conan/tools/meson/__init__.py
@@ -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

11 changes: 10 additions & 1 deletion conan/tools/meson/meson.py
@@ -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):
Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it overrides, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

    c_args = {{c_args}} + preprocessor_definitions + c_args
    c_link_args = {{c_link_args}} + c_link_args
    cpp_args = {{cpp_args}} + preprocessor_definitions + cpp_args
    cpp_link_args = {{cpp_link_args}} + cpp_link_args

So, the only part that is being overridden, it's the first definition of the constants:

    [constants]
    ........
    c_args = []
    c_link_args = []
    cpp_args = []
    cpp_link_args = []

Copy link
Member

Choose a reason for hiding this comment

The 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 c_args = {{c_args}} + preprocessor_definitions + c_args will work. Am I misunderstanding something?
Most likely a test that covers this is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 conan_meson_deps_flags.ini. It's not created a file per dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 conan_meson_deps_flags.ini, being overriden by the conan_meston_toolchain.ini file values for those cppflags, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 --native-file conan_meson_deps_flags1.ini --native-file conan_meson_deps_fags2.ini ... --native-file conan_meson_native.ini, and it has to be in the correct order (besides the fact it's a long command line). I would just write everything into conan_meson_native.ini to simplify the workflow and reduce a chance of human error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you'll only have a single file for all the deps: --native-file conan_meson_deps_flags.ini, and that's it. Have a look at the last test that I've added.


cmd += ' "{}" "{}"'.format(build_folder, source_folder)
if self._conanfile.package_folder:
cmd += ' -Dprefix="{}"'.format(self._conanfile.package_folder)
Expand Down
105 changes: 105 additions & 0 deletions conan/tools/meson/mesondeps.py
@@ -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]
deps_c_args = {{c_args}}
deps_c_link_args = {{c_link_args}}
deps_cpp_args = {{cpp_args}}
deps_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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 GnuDepsFlags.

  • GnuDepsFlags -> GnuFlags
  • (new) GnuDepsFlags getting the GNU flags for all the dependencies

Copy link
Contributor

Choose a reason for hiding this comment

The 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
e.g. we already have too many disputes on what cmake generator to use everywhere, and it's hard enough to explain

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Deps but it's not managing any deps, so, what I'm saying is to do this refactor for internal purpose. Users should not be affected at all.

Copy link
Member

Choose a reason for hiding this comment

The 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())
13 changes: 9 additions & 4 deletions conan/tools/meson/toolchain.py
Expand Up @@ -26,6 +26,11 @@ class MesonToolchain(object):
[constants]
preprocessor_definitions = [{% for it, value in preprocessor_definitions.items() -%}
'-D{{ it }}="{{ value}}"'{%- if not loop.last %}, {% endif %}{% endfor %}]
# Constants to be overridden by conan_meson_deps_flags.ini (if exists)
deps_c_args = []
deps_c_link_args = []
deps_cpp_args = []
deps_cpp_link_args = []

[project options]
{% for it, value in project_options.items() -%}
Expand All @@ -52,10 +57,10 @@ class MesonToolchain(object):
{% if b_staticpic %}b_staticpic = {{b_staticpic}}{% endif %}
{% if cpp_std %}cpp_std = '{{cpp_std}}' {% endif %}
{% if backend %}backend = '{{backend}}' {% endif %}
c_args = {{c_args}} + preprocessor_definitions
c_link_args = {{c_link_args}}
cpp_args = {{cpp_args}} + preprocessor_definitions
cpp_link_args = {{cpp_link_args}}
c_args = {{c_args}} + preprocessor_definitions + deps_c_args
c_link_args = {{c_link_args}} + deps_c_link_args
cpp_args = {{cpp_args}} + preprocessor_definitions + deps_cpp_args
cpp_link_args = {{cpp_link_args}} + deps_cpp_link_args
{% if pkg_config_path %}pkg_config_path = '{{pkg_config_path}}'{% endif %}

{% for context, values in cross_build.items() %}
Expand Down
6 changes: 5 additions & 1 deletion conans/client/generators/__init__.py
Expand Up @@ -72,7 +72,8 @@ def __init__(self):
"MesonToolchain", "MSBuildDeps", "QbsToolchain", "msbuild",
"VirtualRunEnv", "VirtualBuildEnv", "AutotoolsDeps",
"AutotoolsToolchain", "BazelDeps", "BazelToolchain", "PkgConfigDeps",
"VCVars", "IntelCC", "XcodeDeps", "PremakeDeps", "XcodeToolchain"]
"VCVars", "IntelCC", "XcodeDeps", "PremakeDeps", "XcodeToolchain",
"MesonDeps"]

def add(self, name, generator_class, custom=False):
if name not in self._generators or custom:
Expand Down Expand Up @@ -113,6 +114,9 @@ def _new_generator(self, generator_name, output):
elif generator_name == "MesonToolchain":
from conan.tools.meson import MesonToolchain
return MesonToolchain
elif generator_name == "MesonDeps":
from conan.tools.meson import MesonDeps
return MesonDeps
elif generator_name in ("MSBuildDeps", "msbuild"):
from conan.tools.microsoft import MSBuildDeps
return MSBuildDeps
Expand Down
@@ -0,0 +1,146 @@
import os
import platform
import textwrap

from conan.tools.files import load
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_mesondeps(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

def test_mesondeps_flags_are_being_appended_and_not_replacing_toolchain_ones(self):
"""
Test MesonDeps and MesonToolchain are keeping all the flags/definitions defined
from both generators and nothing is being messed up.
"""
client = TestClient(path_with_spaces=False)
if platform.system() == "Windows":
deps_flags = '"/GA", "/analyze:quiet"'
flags = '"/Wall", "/W4"'
else:
deps_flags = '"-Wpedantic", "-Werror"'
flags = '"-Wall", "-finline-functions"'
# Dependency - hello/0.1
conanfile_py = textwrap.dedent("""
from conan import ConanFile

class HelloConan(ConanFile):
name = "hello"
version = "0.1"

def package_info(self):
self.cpp_info.libs = ["hello"]
self.cpp_info.cxxflags = [{}]
self.cpp_info.defines = ['DEF1=one_string', 'DEF2=other_string']
""".format(deps_flags))
client.save({"conanfile.py": conanfile_py})
client.run("create . %s" % self._settings_str)
# Dependency - other/0.1
conanfile_py = textwrap.dedent("""
from conan import ConanFile

class OtherConan(ConanFile):
name = "other"
version = "0.1"

def package_info(self):
self.cpp_info.libs = ["other"]
self.cpp_info.defines = ['DEF3=simple_string']
""")
client.save({"conanfile.py": conanfile_py}, clean_first=True)
client.run("create . %s" % self._settings_str)

# Consumer using MesonDeps and MesonToolchain
conanfile_py = textwrap.dedent("""
from conan import ConanFile
from conan.tools.meson import Meson, MesonDeps, MesonToolchain

class App(ConanFile):
settings = "os", "arch", "compiler", "build_type"
requires = "hello/0.1", "other/0.1"

def layout(self):
self.folders.build = "build"

def generate(self):
tc = MesonDeps(self)
tc.generate()
tc = MesonToolchain(self)
tc.preprocessor_definitions["VAR"] = "VALUE"
tc.preprocessor_definitions["VAR2"] = "VALUE2"
tc.generate()

def build(self):
meson = Meson(self)
meson.configure()
meson.build()
""")

meson_build = textwrap.dedent("""
project('tutorial', 'cpp')
cxx = meson.get_compiler('cpp')
# It's not needed to declare "hello/0.1" as a dependency, only interested in flags
executable('demo', 'main.cpp')
""")
client.save({"conanfile.py": conanfile_py,
"meson.build": meson_build,
"main.cpp": "int main()\n{return 0;}\n"},
clean_first=True)

client.run("install . %s -c 'tools.build:cxxflags=[%s]'" % (self._settings_str, flags))
client.run("build .")
deps_flags = deps_flags.replace('"', "").replace(",", "")
flags = flags.replace('"', "").replace(",", "")
meson_log_path = os.path.join(client.current_folder, "build", "meson-logs", "meson-log.txt")
meson_log = load(None, meson_log_path)
meson_log = meson_log.replace("\\", "/")
assert "Build Options: " \
"'--native-file {folder}/conan_meson_native.ini' " \
"'--native-file {folder}/conan_meson_deps_flags.ini'" \
"".format(folder=client.current_folder.replace("\\", "/")) in meson_log
# Flags/Defines from deps and consumer are appearing in meson-log.txt as part
# of the command-line
assert '%s -DVAR="VALUE" -DVAR2="VALUE2" %s' % (flags, deps_flags) in meson_log
assert '-DDEF3=simple_string -DDEF1=one_string -DDEF2=other_string' in meson_log