MesonToolchain: add extra_variables and exe_wrapper property#20032
MesonToolchain: add extra_variables and exe_wrapper property#20032perseoGI wants to merge 2 commits into
Conversation
jcar87
left a comment
There was a problem hiding this comment.
up for discussion - but I'd just limit the PR to exposing tools.meson.mesontoolchain:extra_variables , which would just work, and improves the UX that can otherwise be achieved with what is decribed in #19912 (comment)
I'm failing to see any case in which one would want to set the exe_wrapper inside a recipe itself, it feels orthogonal and external and depending on factors that are not always visible to the recipe.
I appreciate setting it via the toolchain inside the recipe was requested in #18718, but the example provided by the user sets it to /usr/bin/qemu-aarch64 - which we wouldn't advise to do in a recipe.
| "pkgconfig": self.pkgconfig, | ||
| "pkg-config": self.pkgconfig | ||
| "pkg-config": self.pkgconfig, | ||
| "exe_wrapper": self.exe_wrapper, |
There was a problem hiding this comment.
Do we need a public/modifiable exe_wrapper in the toolchain interface?
As it stands - it can be done with
tc.binaries['exe_wrapper'] = ...
and
tc.exe_wrapper = ...
Not sure we need the dual interface.
On the other hand, I can't really think of a case where one would like to "set" the exe_wrapper from the recipe itself? this tends to be completely external to the recipe itself - and whether or not a wrapper needs to be used depends on factors that go beyond what we can evaluate in a recipe. For example, I could be "cross" building to a different Linux x86_64 variant (where conan cross_building returns false and can_run returns True) . Given that we dont capture libc/libstdc++ info in the settings, we dont have a full way of knowing.
TL;DR: it would be helpful if we had actual scenarios where the exe_wrapper would need to be specified in the recipe, rather than externally (via a conf in the a profile, or in the package_info() of a tool_requires package for a toolchain)
There was a problem hiding this comment.
I completely agree with you. I was not convinced at all in having the exe_wrapper property to be directly accessible from a recipe.
I think this is a profile side responsibility.
I've removed the property and the related.
After the changes, we only have two ways of modifying the exe_wrapper:
- From
self.binariestoolchain property (even though this means we can still modify it from the recipe, it is not "documented". At the end,self.binarieswill be translated to meson .ini file) - From the new config
tools.meson.mesontoolchain:extra_variables-> the important one here.
| self.binaries.update(extra_variables.get("binaries", {})) | ||
| self.project_options.update(extra_variables.get("project_options", {})) | ||
|
|
||
| # Issue: https://github.com/conan-io/conan/issues/19217 |
There was a problem hiding this comment.
this issue is already fixed - presumably being added for additional context as it preserves the behaviour of the fix already in place?
There was a problem hiding this comment.
I just moved the code keeping the comment.
I've removed the comments now.
| # or via tools.meson.mesontoolchain:extra_variables conf | ||
| if self.cross_build: | ||
| has_exe_wrapper = self.exe_wrapper is not None or self.binaries.get("exe_wrapper") is not None | ||
| self.properties["needs_exe_wrapper"] = has_exe_wrapper or not can_run(self._conanfile) |
There was a problem hiding this comment.
per my earlier comment, I think I'd just check if tools.meson.mesontoolchain:extra_variables has binaries['exe_wrapper'] , or perhaps even document the expectation that a user has to set properties['needs_exe_wrapper'] in their profile, along with the exe wrapper itself
while you may think that's more admin in the profile, as it stands, I feel like for this to work properly in some scenarios, unless cross_build and can_run are defined via a conf, we may not be passing the right thing - e.g. for true crossbuild cases of same OS and same arch (e.g. glibc-Linux to glibc-Musl, or really any Linux to same-arch-linux where ld.so is in a different fixed location)
if a user knows they need an exe wrapper, I'd expect them to set
tools.meson.mesontoolchain:extra_variables
withbinaries['exe_wrapper'] = ...
There are scenarios where meson already knows it needs a wrapper, may be okay to let meson decide that.
Otherwise, when os and arch are the same in both host and build profiles, they may need to set:
tools.build.cross_building:cross_buildtools.build.cross_building:can_run
and in cases where meson can't determine it, they should also setneeds_exe_wrapper- so we're looking at 4 confs potentially in a profile.
However, I'd personally not try to be too clever here (which is how we'd ended up in this situation)
There was a problem hiding this comment.
In this case I feel like 50/50.
In one hand, we have seen than giving Conan too much inteligence may not be what users expect.
On the other hand, if we no longer set needs_exe_wrapper we could potentially break some users because we will be changing Conan's current behavior.
Trying to find a intermediate solution, I've changed the code to this:
if self.cross_build:
has_exe_wrapper = self.binaries.get("exe_wrapper") is not None
# Auto evaluation of needs_exe_wrapper only if the user has not set it in the properties
if "needs_exe_wrapper" not in self.properties:
self.properties["needs_exe_wrapper"] = has_exe_wrapper or not can_run(self._conanfile)Conan will only set needs_exe_wrapper "automatically" if the user has not defined the needs_exe_wrapper property by itself (allowing users to overwrite Conans default behavior), and if the user has defined an exe_wrapper path or if can_run evaluates to false -> behavior currently in develop2 code.
| # exe_wrapper can be set via tc.exe_wrapper attribute or via tc.binaries["exe_wrapper"] | ||
| # or via tools.meson.mesontoolchain:extra_variables conf | ||
| if self.cross_build: | ||
| has_exe_wrapper = self.exe_wrapper is not None or self.binaries.get("exe_wrapper") is not None |
There was a problem hiding this comment.
side note:
there's a test that has a nametest_extra_variables_conf_has_priority_over_toolchain, but the fact that there's two ways - but for exe_wrapper, the toolchain is taking preference? or am I missing something
There was a problem hiding this comment.
This has just changed
Changelog: Feature: MesonToolchain: add new configuration
tools.meson.mesontoolchain:extra_variablesChangelog: Feature: MesonToolchain: add new
exe_wrapperpropertySecond attempt for #19940
Close #18718
Close #19912
Docs: TBD
developbranch, documenting this one.