started MesonToolchain improvements #10174

memsharded · 2021-12-13T23:59:35Z

Changelog: Feature: Improvements in MesonToolchain, including some cross-building functionality.
Docs: Omit

I have started to do some changes to update MesonToolchain, it needs some updates:

The model shouldn't be using double quoted strings. Users should be able to do meson.build_type = "debug", and not meson.build_type = "'debug'" (double quoted).
Formatting to Meson files (quotes, lists, ints...) should be done only at render time (context) or even better, in the template.
Using the Environment to handle the definition of environment instead of the raw EnvVars should allow avoiding splitting them in the first place. Also avoid doing "to_meson_value" for these.
Avoiding creating private self._backend vars that aren't used later at all. Define just the public ones like self.backend that users can change later in recipes
It should add support for msvc new setting.

Some other minor things:

Using many methods, many of them just 1 line are being called in just one place is too much, requiring a lot of jumping to understand what is being done, more spaguetti code. Better simplify.
The less code the better. Without becoming cryptic or difficult to read, the whole _get_backend() method can be collapsed to 2 very simple to read lines in the constructor.

@SSE4 as you have been working in this, if you want to follow up and finish these changes, feel free to continue this work, otherwise I will keep working on it when possible, hopefully 1.44.

SSE4 · 2021-12-14T05:57:01Z

well, right now I am more interested and motivated in functional changes for users, than (mostly cosmetic) refactoring.
for instance, I'd like to move forward with #9711, there are some unaddressed questions I would be happy to discuss with you (e.g. #9711 (comment), how can we handle blocks).
so I'd love to see MesonToolchain be usable in practice first (e.g. in the context of CCI).

SSE4 · 2021-12-14T05:59:28Z

conan/tools/meson/toolchain.py

-                              self._conanfile.settings.get_safe("compiler")
-        self._vscrt = self._conanfile.settings.get_safe("compiler.base.runtime") or \
-                      self._conanfile.settings.get_safe("compiler.runtime")
+        # Values are kept as Python built-ins so users can modify them more easily, and they are


these were private (internal) variables, why do we need users to modify them?
in general, I expect users to modify inputs (in our world, settings and conf), instead of messing up with an internal structure of objects. our objects have public interfaces (e.g. generate method and few others), but directly modifying object fields doesn't sound like a good idea for me.

The self.backend, self.b_vscrt are public variables that can be changed, but are very inconveniently assigned as doubled quoted strings, not regular strings, they should be regular strings.

A design principle of all toolchains is that they should allow easy customization, like:

tc = MesonToolchain(self) tc.b_vscrt = xxxx if something else yyyy tc.generate()

This is only nice if b_vscrt is a regular string

I agree they shouldn't contain extra quotes, that's fine, I am not arguing with that at all
I just don't like to expose too many internal implementation details to the user
e.g. I didn't see anyone actually requested b_vscrt to be modifiable directly
we have other (and more important) actual requests for Meson (e.g. cross-building), I'd just keep b_vscrt and others to be fully private members for now, as they aren't currently documented anyway, and it gives us the freedom to do breaking changes
in other words, I wouldn't expose and document them right now, until it's actually requested

about self.backend and self._backend - two are redundant and confusing, for sure
but the method _get_backend fully mimics _get_generator from CMakeToolchain
(backend in meson worlds is just like generator in cmake world)
it could be consistent across different toolchains, I think

conan/tools/meson/toolchain.py

memsharded · 2021-12-14T09:05:09Z

well, right now I am more interested and motivated in functional changes for users, than (mostly cosmetic) refactoring.
for instance, I'd like to move forward with #9711, there are some unaddressed questions I would be happy to discuss with you (e.g. #9711 (comment), how can we handle blocks).

This is beyond cosmetic refactoring. A good design is important, I have changed the title to "improvements", as there are many of those requested changes that are not refactors. For example having the single-quoted strings is something that if not fixed now will be bad if we freeze it as stable in 2.0. Supporting msvc compiler is also important, as we will start removing Visual Studio in 2.0 soon. It is important to build cleanly from the foundations. Having duplicated self._backend and self.backend hurts the readability and maintenance of the class (the former variable is not needed at all, it can be avoided). Formatting the values in the wrong place hurts the readability and maintenance of the class. Single line methods, called just once in the code highly increase the cognitive overhead of the reader, the same effect can be achieved by renaming some variables. Building a _render(template, context) abstraction over Template(template).render(context) is simply too much, it doesn't add any value, but creates more spagetti code. It seems there were also a couple of dead methods, not used at all.

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

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

SSE4 · 2022-01-08T06:08:37Z

This is beyond cosmetic refactoring. A good design is important, I have changed the title to "improvements", as there are many of those requested changes that are not refactors. For example having the single-quoted strings is something that if not fixed now will be bad if we freeze it as stable in 2.0. Supporting msvc compiler is also important, as we will start removing Visual Studio in 2.0 soon. It is important to build cleanly from the foundations. Having duplicated self._backend and self.backend hurts the readability and maintenance of the class (the former variable is not needed at all, it can be avoided). Formatting the values in the wrong place hurts the readability and maintenance of the class. Single line methods, called just once in the code highly increase the cognitive overhead of the reader, the same effect can be achieved by renaming some variables. Building a _render(template, context) abstraction over Template(template).render(context) is simply too much, it doesn't add any value, but creates more spagetti code. It seems there were also a couple of dead methods, not used at all.

I think we need to separate changes, at very least, the ones affecting users (e.g. changing the behavior, like MSVC compiler support) from ones doing "cosmetic" improvements (improving readability and maintenance of the class, but in general invisible to the user, like formatting in wrong places or extracting/inlining methods). otherwise, it's too hard to follow, there is no general goal, there is just a random collection of various unrelated improvements.

SSE4 · 2022-01-08T06:12:29Z

conan/tools/meson/toolchain.py

-        else:
-            self._write_native_file()
+        content = Template(self._meson_file_template).render(self._context)
+        filename = self.native_filename if not self.cross_build else self.cross_filename


reversed condition would read better (without not)

conan/tools/meson/toolchain.py

memsharded · 2022-01-07T21:31:07Z

conan/tools/meson/meson.py

-        if cross_building(self._conanfile):
-            cmd += ' --cross-file "{}"'.format(MesonToolchain.cross_filename)
+        generators_folder = self._conanfile.generators_folder
+        cross = os.path.join(generators_folder, MesonToolchain.cross_filename)


Using the existence of the file, as the decission if it is cross-building or not was already taken by the toolchain.

I think it is reliable enough. of course, some process could create or delete the file in the middle of conan's execution, but realistically it's a low chance, and anyway, further execution wouldn't be correct.

The problem with using here a hardcoded if cross_building() is that if the MesonToolchain decides (the user can do it) that it is not cross-building, this will be wrong. The build helpers like Meson should not decide things just execute what the toolchain has decided to do.

could it be problematic in some flows, e.g. if I run several toolchain executions for multiple profiles?

conan/tools/meson/toolchain.py

SSE4 · 2022-01-09T08:15:45Z

conan/tools/meson/toolchain.py

+        if compiler == "Visual Studio":
+            vscrt = self._conanfile.settings.get_safe("compiler.base.runtime") or \
+                    self._conanfile.settings.get_safe("compiler.runtime")
+            self._b_vscrt = {"MD": "md",


that might use .lower()

another note (might be slightly unrelated), in CCI users, have started to adopt MSVC compiler already, and now recipes need to use a fairly complicated logic to manage compiler runtime, as settings are very different between old and new compilers. do you have a plan to add some sort of helper, with can return uniform runtime values (e.g. in formats like MD or MultiThreadedDebug)?

SSE4 · 2022-01-09T08:20:18Z

conan/tools/meson/toolchain.py

+                arch_target = settings_target.get_safe("arch")
+                self.cross_build["target"] = self._to_meson_machine(os_target, arch_target)
+
+        default_comp = ""


I'd like to avoid anything related to the default comp here, as much as possible.
I think we already agreed to use explicit things rather than autodetection.
many people complain that conan picks the wrong compiler executable (e.g. /usr/bin/gcc instead of gcc-10 or whatever).

for CL, we can make an exception, as we know it's always cl.exe.
for GCC/Clang, we basically, don't know, it could be just gcc.exe, gcc-10.exe, something like arm-linux-gnueabi-gcc.exe...
I'd prefer to hard fail instead of silently producing incorrect binaries with the wrong compiler.

and for this kind of thing, I suppose we need to follow blocks approach, as we discuss in #9710

I beg you to exclude this kind of tentative/risky changes from this PR, and continue the discussion we started before

This is not true. This is not auto-detection, this is a very sane default.
The information is already defined in the settings.compiler, we are not auto-detecting the compiler.

What doesn't make sense is to always force users to define a [buildenv] CC=gcc CXX=g++ when settings.compiler=gcc. We don't do it in any other build system integration, and it shouldn't be done in Meson either. Assuming a gcc/g++, clang/clang++ is a good default, and the user always has the chance to override it defining the env-var. Otherwise, it is impossible to build a Meson project in Linux or OSX with the default detected profile, and requires all users to manually add CC/CXX env-vars to it.

So this is staying, it must be possible for every build system integration to be able to build a basic project with the default detected profile, for a default system installation of compilers and build systems.

the user might not have a /usr/bin/gcc or /usr/bin/clang at all.
for the default profile, it will pick /usr/bin/gcc for settings.compiler=gcc.
but it will also pick /usr/bin/gcc for non default (e.g. armv7) profile (e.g. settings.arch=armv7).
that might not be something desired in this particular case, and in many others (e.g. Android NDK).
there is no check this choice is done for the default profile only, it always applies.
yes, it makes some use-cases more smooth, but other use-cases might be silently broken because of that default.
I'd limit these defaults to some known cases only, e.g. :

apple-clang with XCode, where the compiler should be located via XCRun

MSVC/Visual Studio, where VCVars is responsible for the compiler discovery

Android NDK, where we have a clear mapping of settings to the compiler executables

for others, we don't know and can't be sure what is the sane choice. it might be gcc.exe, gcc-10.exe, gcc-10.0.exe or arm-linux-gnueabi-gcc.exe.

but it will also pick /usr/bin/gcc for non default (e.g. armv7) profile (e.g. settings.arch=armv7).
that might not be something desired in this particular case, and in many others (e.g. Android NDK).

Of course the default will not work in cases of cross-compiling, and it will require specifying the specific compiler you want for cross-compile. This doesn't mean that for 75% of the users, doing native builds, we shouldn't define a default. Again, this is the same default as our CMake integration, by default it will pick the system "gcc/g++", and only for cross-compile scenarios you need to specify the CC/CXX env-vars.

how about limiting the scope for native file only?

Are you aware that if we don't add this, it will be also challenging to build packages using Meson in ConanCenter, where we are using the default profiles? If we change all default profiles in ConanCenter, which are different to the user default profiles, the risk of things not working also increases.

right now, in CCI, the only problematic use case is M1. all other default profiles work.

Because the recipes there are not using MesonToolchain yet. If MesonToolchain requires defining CC/CXX in all profiles, for all builds, things will be broken, it will be not possible to update without changing all the profiles in ConanCenter. Recall that it is necessary to move all recipes to the new integrations to be ready for 2.0.

we already tried MesonToolchain in CCI in the past: https://github.com/conan-io/conan-center-index/pull/7360/files https://github.com/conan-io/conan-center-index/pull/6678/files.
the thing is that it requires explicit CC/CXX for cross-building.
but it doesn't require it for native builds. e.g. it is able to deduce cl for Visual studio and gcc for GCC and clang for XCode builds.
profiles don't need to change.
the only one we currently have trouble is M1, but for this one, it needs a bunch of other flags (like -arch and -isysroot) from the XCrun.

SSE4 · 2022-01-09T08:22:52Z

conan/tools/meson/meson.py

-        source = self._conanfile.source_folder
-        if source_folder:
-            source = os.path.join(self._conanfile.source_folder, source_folder)
+    def configure(self,  build_script_folder=None):


I doubt in meson world they call it build script folder, might be misleading for the people

Suggestion for an alternative name?

in documentation, they refer to it as a builddir

SSE4 · 2022-01-09T08:23:44Z

conan/tools/meson/meson.py

        self._conanfile.run(cmd)

    def test(self):
-        cmd = 'meson test -v -C "{}"'.format(self._build_dir)
+        meson_build_folder = self._conanfile.build_folder
+        cmd = 'meson test -v -C "{}"'.format(meson_build_folder)
        # TODO: Do we need vcvars for test?
        # TODO: This should use conanrunenv, but what if meson itself is a build-require?


it's actually most likely, meson is a build requirement

SSE4 · 2022-01-10T19:54:29Z

conan/tools/meson/toolchain.py

+                            "MT": "mt",
+                            "MTd": "mtd"}.get(vscrt, "none")
+        elif compiler == "msvc":
+            # TODO: Fill here the msvc model


Suggested change

# TODO: Fill here the msvc model

# TODO: Fill here the msvc model

self._b_vscrt = "m{}{}".format(

"t" if self.settings.compiler.runtime == "static" else "d",

"d" if self.settings.compiler.runtime_type == "Debug" else "",

)

…lchain Feature/refactor meson toolchain

franramirez688

Looking good! 👍

@memsharded I think we should add to the PR description that it closes (perhaps not completely) the issue #9710, wdyt?

started MesonToolchain refactors

4946b61

SSE4 reviewed Dec 14, 2021

View reviewed changes

conan/tools/meson/toolchain.py Outdated Show resolved Hide resolved

memsharded changed the title ~~started MesonToolchain refactors~~ started MesonToolchain improvements Dec 14, 2021

memsharded added 5 commits January 7, 2022 14:58

Merge branch 'develop' into refactor/mesontoolchain

07f2de9

wip

9053dd3

remove print excpt

ccd44b1

wip

5d02ea5

wip

8c1040b

memsharded commented Jan 7, 2022

View reviewed changes

conans/test/functional/toolchains/meson/test_ios.py Show resolved Hide resolved

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

fix test

08b5014

memsharded added this to the 1.45 milestone Jan 7, 2022

memsharded added 2 commits January 7, 2022 23:09

fix ios

0becfa9

fix meson test

af5e22b

SSE4 reviewed Jan 8, 2022

View reviewed changes

conan/tools/meson/toolchain.py Outdated Show resolved Hide resolved

SSE4 reviewed Jan 8, 2022

View reviewed changes

conan/tools/meson/toolchain.py Outdated Show resolved Hide resolved

wip

a6fb97f

memsharded commented Jan 8, 2022

View reviewed changes

SSE4 reviewed Jan 9, 2022

View reviewed changes

conan/tools/meson/toolchain.py Outdated Show resolved Hide resolved

SSE4 reviewed Jan 9, 2022

View reviewed changes

conan/tools/meson/toolchain.py Outdated Show resolved Hide resolved

SSE4 reviewed Jan 9, 2022

View reviewed changes

SSE4 mentioned this pull request Jan 9, 2022

[question] cross compile meson packages from CCI #10285

Open

1 task

review

6d08e8a

memsharded added 2 commits January 10, 2022 19:14

fix error in -arch

9e5cf53

fix

017998e

SSE4 reviewed Jan 10, 2022

View reviewed changes

fix x86 build

65ce123

memsharded assigned franramirez688 Jan 11, 2022

memsharded and others added 15 commits January 18, 2022 19:56

Merge branch 'develop' into refactor/mesontoolchain

5d539e9

Test passing

b0f44a9

Refactored some modules

21abaf1

Fixed bad type

65f49a9

Fixed tests

ee952d5

Refactored cross compilation tests

cf1b5d3

Minor changes

6dc871e

Merge pull request #40 from franramirez688/feature/refactor_meson_too…

911c797

…lchain Feature/refactor meson toolchain

minor fixes

9a47b1a

Refactored block

3614bad

Added integration test

afaa138

Added integration test

a1b93ca

Deleted include(default)·

ec5acfd

Merge pull request #41 from franramirez688/feature/refactor_meson_too…

e474e23

…lchain Feature/refactor meson toolchain

moved _add_apple_flags

0eb27a6

memsharded marked this pull request as ready for review January 21, 2022 11:45

franramirez688 requested a review from SSE4 January 21, 2022 11:53

Adding skipif py2 to test

b157104

franramirez688 approved these changes Jan 21, 2022

View reviewed changes

Missing import sys

4f461e8

memsharded merged commit 64ea735 into conan-io:develop Jan 21, 2022

memsharded deleted the refactor/mesontoolchain branch January 21, 2022 14:52

This was referenced Jan 24, 2022

Updating MesonToolchain information conan-io/docs#2369

Merged

[feature][MesonToolchain] Improve Android cross-compilation #10407

Closed

franramirez688 mentioned this pull request Jul 8, 2022

Feature: MesonToolchain: improve cross-compilation workflow on Apple systems #9711

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

started MesonToolchain improvements #10174

started MesonToolchain improvements #10174

memsharded commented Dec 13, 2021 •

edited by czoido

SSE4 commented Dec 14, 2021

SSE4 Dec 14, 2021

memsharded Dec 14, 2021

SSE4 Dec 14, 2021

SSE4 Dec 14, 2021

memsharded commented Dec 14, 2021

SSE4 commented Jan 8, 2022

SSE4 Jan 8, 2022

memsharded Jan 7, 2022

SSE4 Jan 9, 2022

memsharded Jan 9, 2022

SSE4 Jan 9, 2022

SSE4 Jan 9, 2022

SSE4 Jan 10, 2022

SSE4 Jan 9, 2022

memsharded Jan 9, 2022

SSE4 Jan 9, 2022

memsharded Jan 10, 2022

SSE4 Jan 10, 2022

memsharded Jan 10, 2022

SSE4 Jan 10, 2022

memsharded Jan 10, 2022

SSE4 Jan 10, 2022

SSE4 Jan 9, 2022

memsharded Jan 9, 2022

SSE4 Jan 9, 2022

SSE4 Jan 9, 2022

SSE4 Jan 10, 2022

franramirez688 left a comment

-            # TODO: Fill here the msvc model
+            # TODO: Fill here the msvc model
+            self._b_vscrt = "m{}{}".format(
+                    "t" if self.settings.compiler.runtime == "static" else "d",
+                    "d" if self.settings.compiler.runtime_type == "Debug" else "",
+                )

started MesonToolchain improvements #10174

started MesonToolchain improvements #10174

Conversation

memsharded commented Dec 13, 2021 • edited by czoido

SSE4 commented Dec 14, 2021

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

memsharded commented Dec 14, 2021

SSE4 commented Jan 8, 2022

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

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 left a comment

Choose a reason for hiding this comment

memsharded commented Dec 13, 2021 •

edited by czoido