Bugfix: ensure meson gets correct compiler flags by SSE4 · Pull Request #5222 · conan-io/conan

SSE4 · 2019-05-27T10:32:33Z

closes: #4573
tags: @slow, @svn
Changelog: Bugfix: Meson build-helper gets correct compiler flags, AutoTools build environment adds compiler.runtime flags
Docs: omit

short story: currently meson build helper doesn't propagate required compiler flags, like AutoToolsBuildEnvironment, CMake or MSBuild do. it requires ugly workarounds in each recipe:

        if str(self.settings.compiler) in ["gcc", "clang"]:
            if self.settings.arch == "x86":
                defs["c_args"] = "-m32"
                defs["cpp_args"] = "-m32"
                defs["c_link_args"] = "-m32"
                defs["cpp_link_args"] = "-m32"
            elif self.settings.arch == "x86_64":
                defs["c_args"] = "-m64"
                defs["cpp_args"] = "-m64"
                defs["c_link_args"] = "-m64"
                defs["cpp_link_args"] = "-m64"
        elif self.settings.compiler == "Visual Studio":
            defs["c_args"] = "-%s" % self.settings.compiler.runtime
            defs["cpp_args"] = "-%s" % self.settings.compiler.runtime

as meson supports the same way as auto-tools, I've re-used AutoToolsBuildEnvironment build helper in order to set up correct compile flags.
see also: https://mesonbuild.com/howtox.html#set-extra-compiler-and-linker-flags-from-the-outside-when-eg-building-distro-packages

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

Signed-off-by: SSE4 <tomskside@gmail.com>

lasote · 2019-05-27T11:38:10Z

+    def _run(self, command):
+        with tools.vcvars(self._settings,
+                          output=self._conanfile.output) if self._vcvars_needed else tools.no_op():
+            env_build = AutoToolsBuildEnvironment(self._conanfile)


At first sight it doesn't feel great to use another build helper and especially that one based only on the env vars.
Is there another thing we can do like write a file to be injected to meson?

seems like writing to file is something we will need to solve #4529
but I think it's blocked by #5184
we need to do some design decisions on how to handle such kind of files, are they produced by their own generators, or by existing generators, or some other entities.
I think right now it's too much for the original particular problem.
I prefer to go by small iterations, like first solve flags issue, then solve cross-compiling issue, and then solve toolchain file design problems. I think it's too much to do at single shot.
for the AutoToolsBuildEnvironment, why not re-use it, at least for the first iteration? as soon as meson is compatible with auto-tools variables, we can re-use the code, especially as it's already well-tested and proven to work correctly.

jgsogo · 2019-05-27T11:48:55Z

I would have a look at the Meson build description (meson.build files) approach, although I'm not sure how can we plug it into an existing project 🤔

SSE4 · 2019-05-27T13:03:07Z

this approach may work: https://mesonbuild.com/Cross-compilation.html
I'll take a look into it

lasote · 2019-05-28T16:18:29Z

I'm very confused about the new generator for cross building with Meson that you introduced. Here you are suggesting to go step by step and then suddenly invent a new generator?
Unless you can explain clearly why is that needed for closing the issue I would ask you to create an issue and a different pull request, that won't be a priority until we review the full picture of cross building.

EDIT: There are no single test using the new generator so I suppose it is not necessary to solve the flags issue. right?

SSE4 · 2019-05-28T16:19:24Z

@lasote I am going to remove that generator from the PR, it's still incomplete,

SSE4 · 2019-05-28T16:22:42Z

generator code was removed. it will go to its own PR, probably after the #5184

lasote

Ok. You are right, there is no simple way to improve and applying AutoToolsBuildEnvironment is very straightforward.

jgsogo · 2019-05-29T08:33:27Z

I agree with adding all those flags to Meson build helper, they are needed and very useful. I also like the implementation on top of AutoToolsBuildEnvironment.

My only concern is related to the stability as none of this build helpers are marked as experimental and we are changing the flags that are currently used:

We are adding the runtime to AutoToolsBuildEnvironment (@SSE4, please include this in the changelog)
We are passing a lot of new flags to the Meson one

How does this play with existing recipes? Is this breaking existing behavior? Does it require to change existing recipes so they no longer use the ugly workaround? Can we consider it a bug and just fix it?

SSE4 · 2019-05-29T08:38:48Z

@jgsogo yes, we may change recipes to remove existing workarounds. right now we have to deal with compiler.runtime manually, e.g.:
https://github.com/bincrafters/conan-libiconv/blob/testing/1.15/conanfile.py#L91
https://github.com/bincrafters/conan-ffmpeg/blob/stable/4.1/conanfile.py#L255
otherwise, we don't get correct binaries. same story about meson:
https://github.com/bincrafters/conan-glib/blob/stable/2.58.3/conanfile.py#L79
https://github.com/bincrafters/conan-gstreamer/blob/stable/1.16.0/conanfile.py#L62
and it doesn't seem to be breaking - if recipe is already adding these flags, adding them for the second time will have no effect

jgsogo · 2019-05-29T09:04:19Z

...but if someone was not adding all those flags, now he will. Is this breaking, @lasote? Is it enough to add a paragraph in the release blog-post?

SSE4 · 2019-05-29T09:05:23Z

if someone didn't add those flags, he got incorrect package. anyway, it's up to you to decide.

lasote · 2019-05-29T14:04:31Z

...but if someone was not adding all those flags, now he will. Is this breaking, @lasote? Is it enough to add a paragraph in the release blog-post?

Well, probably yes. Could the previous behavior be considered a bug? probably yes... Is the usage of meson very limited? I would say yes, In general it improves the situation.... probably yes. I would say we should try to introduce it and if someone report as a breaking change we could reconsider based on the specific use case.

jgsogo · 2019-05-29T14:28:15Z

Ok, thanks, so let's go with it!

jgsogo

Thanks for the contribution, @SSE4 !

- ensure meson gets correct compiler flags

22e64c4

Signed-off-by: SSE4 <tomskside@gmail.com>

jgsogo self-requested a review May 27, 2019 10:34

lasote added this to the 1.16 milestone May 27, 2019

lasote reviewed May 27, 2019

View reviewed changes

jgsogo assigned SSE4 May 27, 2019

SSE4 force-pushed the meson_compiler_flags branch from d2b0d1d to 22e64c4 Compare May 28, 2019 16:21

lasote approved these changes May 28, 2019

View reviewed changes

lasote assigned jgsogo and unassigned SSE4 May 28, 2019

jgsogo approved these changes May 29, 2019

View reviewed changes

jgsogo merged commit 11f7df6 into conan-io:develop May 29, 2019

Conversation

SSE4 commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lasote May 27, 2019

Choose a reason for hiding this comment

Uh oh!

SSE4 May 28, 2019

Choose a reason for hiding this comment

Uh oh!

jgsogo commented May 27, 2019

Uh oh!

SSE4 commented May 27, 2019

Uh oh!

lasote commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SSE4 commented May 28, 2019

Uh oh!

SSE4 commented May 28, 2019

Uh oh!

lasote left a comment

Choose a reason for hiding this comment

Uh oh!

jgsogo commented May 29, 2019

Uh oh!

SSE4 commented May 29, 2019

Uh oh!

jgsogo commented May 29, 2019

Uh oh!

SSE4 commented May 29, 2019

Uh oh!

lasote commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgsogo commented May 29, 2019

Uh oh!

jgsogo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SSE4 commented May 27, 2019 •

edited

Loading

lasote commented May 28, 2019 •

edited

Loading

lasote commented May 29, 2019 •

edited

Loading