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

Bugfix: ensure meson gets correct compiler flags #5222

Merged
merged 1 commit into from May 29, 2019

Conversation

Projects
None yet
3 participants
@SSE4
Copy link
Contributor

commented May 27, 2019

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.

- ensure meson gets correct compiler flags
Signed-off-by: SSE4 <tomskside@gmail.com>

@jgsogo jgsogo self-requested a review May 27, 2019

@lasote lasote added this to the 1.16 milestone May 27, 2019

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)

This comment has been minimized.

Copy link
@lasote

lasote May 27, 2019

Contributor

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?

This comment has been minimized.

Copy link
@SSE4

SSE4 May 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Member

commented May 27, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

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

@lasote

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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

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

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

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

@lasote

lasote approved these changes May 28, 2019

Copy link
Contributor

left a comment

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

@lasote lasote assigned jgsogo and unassigned SSE4 May 28, 2019

@jgsogo

This comment has been minimized.

Copy link
Member

commented May 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

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

@lasote

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

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

This comment has been minimized.

Copy link
Member

commented May 29, 2019

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

@jgsogo

jgsogo approved these changes May 29, 2019

Copy link
Member

left a comment

Thanks for the contribution, @SSE4 !

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

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.