[POC] MesonToolchain and GNU flags coming from dependencies #11557

franramirez688 · 2022-06-30T14:04:03Z

Changelog: Feature: Added MesonDeps as a new generator to create an extra file with all the GNU flags from all the dependencies. Meson build-helper will add that one (if exists) to complement the created one by MesonToolchain.
Docs: conan-io/docs#2654
Closes: #10683 #11397 #11398

Motivation: Dependencies which are defining only GNU flags, MesonToolchain will never get anything from their build environments because it needs those flags to initialize its own instance, so right now, it's possible to work-around it but it's a bit hard. This PR is proposing that alternative but, for sure, it's open to being discussed.

conans/test/functional/toolchains/meson/test_meson_and_gnu_deps_flags.py

SSE4 · 2022-06-30T16:52:01Z

conan/tools/gnu/autotoolsdeps.py

+    ret = NewCppInfo()
+    for dep in deps:
+        dep_cppinfo = dep.cpp_info.aggregated_components()
+        # In case we have components, aggregate them, we do not support isolated


what exactly does "isolated" target mean?

I've only used the functions that were already there. There's nothing new here.

it doesn't answer my question...

SSE4 · 2022-06-30T16:52:55Z

conan/tools/gnu/autotoolsdeps.py

+    return[dep for dep in reversed(deps.values())]
+
+
+def get_gnu_deps_flags(conanfile):


a free function probably doen't belong to AutoToolsDeps generator, it should be somewhere else in conan.tools?

The logic was already here, so I've kept it here as well. I don't think that it has to be in Conan tools but I'm not saying that this is the better place or even if we want it. By the record, the final import is from conan.tools.gnu import get_gnu_deps_flags so I guess it makes sense... Let's discuss! 😄

SSE4 · 2022-07-01T17:12:34Z

conans/test/functional/toolchains/meson/test_meson_and_gnu_deps_flags.py

+
+            def generate(self):
+                # Get GNU flags from all the dependencies
+                cflags, cxxflags, cpp_flags, libs, ldflags = get_gnu_deps_flags(self)


perhaps, we need some universal interface (object) that could be returned from or accepted by various tools (build helpers).
how about some NamedTuple or struct?

SSE4 · 2022-07-01T17:13:58Z

conan/tools/gnu/autotoolsdeps.py

+    ldflags.extend(flags.lib_paths)
+
+    # set the rpath in Macos so that the library are found in the configure step
+    if conanfile.settings.get_safe("os") == "Macos":


it's weird why is it only Macos?

lasote

Not sure about the toolchain being populated with the dependencies.
I need some clarification because I've read the issues associated. Still, I'm unsure about what would happen if the workarounds that were introduced here are moved to a MesonDeps generator that is based on the AutotoolsDeps? I think it should work right? Why introduce in the toolchain the information about the deps? The Meson tool, when run, will load first the launcher for the environment. am I missing something?

franramirez688 · 2022-07-04T11:25:33Z

moved to a MesonDeps generator that is based on the AutotoolsDeps?

Yeah, I think I'm going to try a MesonDeps instead of this. I think it's going to be much cleaner.
Thanks for the feedback!

SSE4 · 2022-07-04T11:28:50Z

Yeah, I think I'm going to try a MesonDeps instead of this. I think it's going to be much cleaner. Thanks for the feedback!

I can't find anything about MesonDeps, neither in the codebase, nor in the documentation. do you have a link?

franramirez688 · 2022-07-04T11:33:04Z

I can't find anything about MesonDeps, neither in the codebase, nor in the documentation. do you have a link?

Sure you cannot because that's a new generator. It's not created yet. I'm going to study if it's feasible or not and then, update the PR.

franramirez688 · 2022-07-05T09:38:07Z

conan/tools/gnu/gnudeps_flags.py

                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"


It's true that these ones were a bug if the compiler was MSVC:

https://docs.microsoft.com/en-us/cpp/build/reference/i-additional-include-directories?view=msvc-170

https://docs.microsoft.com/en-us/cpp/build/reference/libpath-additional-libpath?view=msvc-170

what exactly was a bug?

@SSE4 Now, the Meson test is passing because of this change. Anyway, as you already know, the Meson bug is still there.

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

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.

Anyway, if they're valid both of them, then, let's use the forward slash. What do you think?

yes, I have no objections. I'd prefer the way it works with meson.

franramirez688 · 2022-07-05T09:40:44Z

conan/tools/meson/mesondeps.py

+        self.cpp_args = []
+        self.cpp_link_args = []
+
+    # TODO: Add all this logic to GnuDepsFlags? Distinguish between GnuFlags and GnuDepsFlags?


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

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

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.

GnuDepsFlags will not be exposed to users, it is internal

franramirez688 · 2022-07-05T09:41:18Z

conan/tools/meson/meson.py

        else:
            cmd += ' --native-file "{}"'.format(native)
+            if has_deps_flags:
+                cmd += ' --native-file "{}"'.format(deps_flags)


https://mesonbuild.com/Machine-files.html#loading-multiple-machine-files

but it overrides, isn't it?

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 = []

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.

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.

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.

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.

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.

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.

memsharded · 2022-07-05T15:49:33Z

conan/tools/meson/meson.py

        else:
            cmd += ' --native-file "{}"'.format(native)
+            if has_deps_flags:
+                cmd += ' --native-file "{}"'.format(deps_flags)


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.

memsharded · 2022-07-07T23:19:29Z

conan/tools/gnu/gnudeps_flags.py

@@ -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"


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?

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.

if it's more compatible with tools (e.g. meson), let's switch

franramirez688 requested review from SSE4, lasote and czoido June 30, 2022 14:04

franramirez688 assigned memsharded Jun 30, 2022

This was linked to issues Jun 30, 2022

[bug] MesonToolchain -LIBPATH is ignored? #11398

Closed

[bug] AutoToolsDeps sets includes to CPPFLAGS, but MesonToolchain reads only CFLAGS/CXXFLAGS? #11397

Closed

franramirez688 added this to the 1.51 milestone Jun 30, 2022

SSE4 reviewed Jun 30, 2022

View reviewed changes

conans/test/functional/toolchains/meson/test_meson_and_gnu_deps_flags.py Outdated Show resolved Hide resolved

SSE4 reviewed Jun 30, 2022

View reviewed changes

SSE4 reviewed Jul 1, 2022

View reviewed changes

lasote reviewed Jul 4, 2022

View reviewed changes

franramirez688 added 4 commits July 4, 2022 18:01

Added get_gnu_deps_flags. Added functional test

a3a8f8e

Trying Windows

7f774ba

Reversed

005b591

Fixed bad CL flags and created new MesonDeps (based on AutotoolsDeps)

5bc83e6

franramirez688 force-pushed the feature/meson_and_gnu branch from 1ac716c to 5bc83e6 Compare July 5, 2022 09:36

franramirez688 commented Jul 5, 2022

View reviewed changes

franramirez688 requested review from lasote and SSE4 July 5, 2022 10:00

SSE4 approved these changes Jul 5, 2022

View reviewed changes

memsharded requested changes Jul 5, 2022

View reviewed changes

franramirez688 added 4 commits July 6, 2022 10:58

Added new tests to check MesonDeps flags addtion

b0e8b32

Extra assert

bf54fec

Fixed test. Changed variables names

8e98884

Bad change

6cbccf6

franramirez688 requested a review from memsharded July 6, 2022 11:25

memsharded approved these changes Jul 7, 2022

View reviewed changes

lasote approved these changes Jul 11, 2022

View reviewed changes

memsharded merged commit ee28057 into conan-io:develop Jul 11, 2022

franramirez688 mentioned this pull request Jul 21, 2022

[generator] MesonDeps conan-io/docs#2654

Merged

franramirez688 mentioned this pull request Aug 9, 2022

- [glib] use MesonToolchain conan-io/conan-center-index#11053

Closed

4 tasks

SSE4 mentioned this pull request Feb 4, 2023

[bug] Deprecate MesonDeps #13048

Closed

		return[dep for dep in reversed(deps.values())]


		def get_gnu_deps_flags(conanfile):

[POC] MesonToolchain and GNU flags coming from dependencies #11557

[POC] MesonToolchain and GNU flags coming from dependencies #11557

Conversation

franramirez688 commented Jun 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasote left a comment

Choose a reason for hiding this comment

franramirez688 commented Jul 4, 2022

SSE4 commented Jul 4, 2022

franramirez688 commented Jul 4, 2022

franramirez688 Jul 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franramirez688 Jul 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franramirez688 commented Jun 30, 2022 •

edited

franramirez688 Jul 5, 2022 •

edited

franramirez688 Jul 6, 2022 •

edited