-
Notifications
You must be signed in to change notification settings - Fork 994
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
Feature: MesonToolchain: improve cross-compilation workflow on Apple systems #9711
Conversation
c257a2f
to
354d88d
Compare
self.objcpp_link_args.extend(args) | ||
|
||
|
||
class MesonToolchain(MesonBaseToolchain, MesonAppleMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, no multiple inheritance. And avoid inheritance as much as possible. Go with composition unless there is an extremely compelling case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a well-known mixin pattern?
/cc @jgsogo
suggestion on how to to it better are welcomed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@memsharded do you want me to implement AppleSystemBlock
like it was done in CMakeToolchain
?
https://github.com/conan-io/conan/blob/develop/conan/tools/cmake/toolchain.py#L280
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please, avoid inheritance and mixins unless it is strictly necessary. Composition scales better and is more flexible for user customization. Going with a "blocks" approach like the CMakeToolchain
will be better, please go that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking at how to implement blocks similarly to how it was done in CMake toolchain.
there are several ideas, and I think I'll need some advice.
first off, there are two main things Apple block should define:
- compiler executables,
c
(known asCC
in autotools world) andcpp
(known asCXX
) - flags for compilers and linkers, like
c_args
(CFLAGS
),cpp_args
(CXXFLAGS
),link_args
(LDFLAGS
), etc.
for executables, it should take the very least priority (only use when not explicitly defined), for variables, it should effectively append them to any existing.
then, unlike CMake
, meson machine files are simple .ini files with very limited syntax. they are not turing-complete, and cannot contain conditions, loop, and other flow control instructions. there are very few operators supported, like +
and /
(https://mesonbuild.com/Machine-files.html#constants), but that's it.
on the other hand, there is an interesting feature that meson supports multiple cross-files overriding each other: https://mesonbuild.com/Machine-files.html#loading-multiple-machine-files
so, we may use something like --cross-file conan_meson_cross_apple.ini --cross-file conan_meson_cross_main.ini
.
therefore, there are likely 3 major approaches on how to implement that:
- use multiple cross-files overriding each other
- use a single cross-file with several blocks
- use a single cross-file with one block, but compose several contexts within the toolchain
- is easy to implement, but it might be kinda misleading: too many files to deal with for the consumer, e.g. too long command lines, and easy to mess up with an order
- might be tricky with a limited .ini syntax. e.g. I can't define the same value twice within the same machine file, I will get an error:
configparser.DuplicateOptionError
option 'buildtype' in section 'built-in options' already exists
and I can't check if it's already defined above, without using an external check in the code.
- is also easy to implement, it will be just a matter of creating several context dictionaries and merging them according to our specific rules (executables override, flags append). but it's not really a blocks approach.
looking for the advice, thanks!
if not is_apple_os(conanfile.settings.get_safe("os")): | ||
return | ||
|
||
xcrun = XCRun(conanfile.settings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better move XCRun first to modern approach. It should be less detection based and more configuration based.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the XCRun is already imported from a new namespace conan.tools.apple
or what exactly do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anyway, I think improvements of XCRun, if any, could be done in another, their own PR. this is outside of the scope of this PR IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that generators like this one do not auto-detect anything. XCRun is deducing things by executing some commands right? That should be moved to input configuration, like the new [conf]
.
This is a general effort we are doing in 2.0, we are removing all the platform, architecture, compiler, env-vars detection in generators and helpers, and that should happen in the input configuration. Please try that approach better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know right now how it should look like and how it should be implemented.
if you have some example, I can take a look, but my point it belongs to XCRun, not to the meson toolchain.
in other words, as I understand, XCRun will have some code to read conf
file instead of executing commands.
I don't see any reason why it should be done inside this particular PR - it is just using XCRun, and for meson toolchain it's an implementation detail how exactly it is implemented under the hood.
I also don't understand how is it going to work in CI, e.g. in case of ConanCenter, e.g. who, when and how exactly should populate conf with the input configutation?
really, I am just relying on the existing implementations of conan.tools
namespace here, if they are somehow bbad/buggy or don't meet the conan 2.0 requirements, let's create an another issue and improve in another PR.
I would love to see this moving forward
we have a plenty of foundational recipes using
|
@SSE4 Does the new meson build helper only fail to cross compile to M1 or does it fail to cross build in all cases? |
no, not just Apple. see issue #9922 for the similar incident with Visual Studio cross-compilation (x86_64 -> x86). |
Signed-off-by: SSE4 <[email protected]>
354d88d
to
74a94bc
Compare
In the end, it was superseded by #10174 |
closes: #9710
Changelog: Feature: MesonToolchain: improve cross-compilation workflow on Apple systems
Docs: omit
adds a mixin
MesonAppleMixin
to add some important flags specific to the Apple systemsadds missing
obj[cpp]_[link]_args
to the cross-file (compiler/linker flags for Objective-C and Objective-C++ languages)adds missing
root
to the cross-file (sysroot)new test for M1 cross-building
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.