[WIP] Added new toolchain-based Meson helper #7155
Conversation
- removed unneeded methods - cleaned up meson version checks - added warning when using ninja fallback - made MesonToolchain castable to tuple
- Updated code to be more inline with current `CMakeToolchain` - Documented some methods a bit. - Added more TODOs and FIXMEs.
|
This implementation supports the following cases:
|
conans/client/build/mesonx.py
Outdated
| options = dict() | ||
| if self._conanfile.package_folder: | ||
| options['prefix'] = self._conanfile.package_folder | ||
| options['libdir'] = DEFAULT_LIB |
There was a problem hiding this comment.
This is a bit dirty, we need a better way to specify the layout, this is hardcoded, even if the package_info() defines different directories, this will package to a different place, which will be confusing.
I'd prefer to fail at the moment, until we come up with a better way.
There was a problem hiding this comment.
Uhm, but won't this fail during local dev flow? For example, a user invokes conan configure . -bf=local_folder -sf=some_folder, but, since package_folder is not available, this command will raise.
There was a problem hiding this comment.
It is better to provide partial functionality at the moment and raise if something is not implemented yet (for example, the local flow), than provide flaky or hardcoded assumptions, like using DEFAULT_BIN and DEFAULT_LIB directories, that some users will start relying on that behavior, and later will be broken when we change it.
There was a problem hiding this comment.
The problem is that nobody will use (and test) this feature if it's doesn't work in the very basic and common scenario (esp. when the current helper does provide such functionality). Maybe add a note that "local development flow is highly experimental and might be/will be changed in the future updates"?
^ this about about prefix only. I don't have much objections to removing DEFAULT_BIN and friends (I don't see why it's bad though, esp. since conan itself expects a fixed layout when searching for dep binaries).
There was a problem hiding this comment.
Note: removed DEFAULT_BIN and friends, but kept prefix for now.
| host_os = self._ss('os') | ||
| if host_os and 'Windows' in host_os: | ||
| self._conanfile.output.warn("Toolchain: Ignoring fPIC option defined for Windows") | ||
| else: |
There was a problem hiding this comment.
Shouldn't these things be managed in the toolchain files, and not in the build-helper?
There was a problem hiding this comment.
Meson machine files are (mostly) for describing the machine environent (hence the name), i.e. architecture, compiler, search paths and etc. Options like fPIC and std=c++** are configuration options (e.g compiler usually supports more than one std mode and different projects might use different values even if used with the same machine file) and can't be specified in said files.
There was a problem hiding this comment.
The point of the toolchain() feature is that for users working locally, they can build with their command line of the build system, and obtain the same result as if building with the build() method in the Conan cache. So having a very long command line that needs to include all the options, like:
$ meson -Db_staticpic=True -Db_ndebug=if-release -Dcpp_std=...If all those inputs were already introduced by Conan in the previous conan install command, the idea of the toolchain() is that they are saved in a file or something that the build system and the users can easily use.
There was a problem hiding this comment.
Hm... One alternative would be to save all options that don't fit in Meson's machine files in a separate file which is generated during toolchain() and parsed during configure().
| '17': 'c++17', 'gnu17': 'gnu++17', | ||
| '20': 'c++1z', 'gnu20': 'gnu++1z' | ||
| } | ||
| if cppstd not in cppstd_conan2meson: |
There was a problem hiding this comment.
Shouldn't this be managed in the toolchain, not here in the build-helper?
|
@memsharded , @jgsogo , I'd really like to move forward with this PR, esp. in the light of the following quote by jgsogo:
I've replied to all the raised issues in this PR. But I can't do much without further input from conan devs (except for adding more tests). |
There was a problem hiding this comment.
The main idea of the toolchain() approach is that it captures information passed at conan install time, and put it in a build-system format, so calling the build system manually is both simple and also achieve exactly the same build that with build() method and conan create.
So it excedes a bit every build system own definition of "toolchain". The Conan toolchain() could also generate virtualenv scripts if it is necessary to achieve a correct build that is using tools stored in Conan packages. Managing the environment is on the TODO list.
I would say that the current approach is very restricted to the Meson definition of toolchain, and thus the new new MesonX build system helper is still doing too much logic, so it might be missing something.
Also, I am fine if the helper and toolchain only works in Python 3.5+, but they cannot break Python 2.7 users that are not using it. So type annotations needs to be removed. In the same way module scope imports of from pathlib import Path are not possible.
Asking for @jgsogo feedback on this PR.
conans/client/build/mesonx.py
Outdated
| options = dict() | ||
| if self._conanfile.package_folder: | ||
| options['prefix'] = self._conanfile.package_folder | ||
| options['libdir'] = DEFAULT_LIB |
There was a problem hiding this comment.
It is better to provide partial functionality at the moment and raise if something is not implemented yet (for example, the local flow), than provide flaky or hardcoded assumptions, like using DEFAULT_BIN and DEFAULT_LIB directories, that some users will start relying on that behavior, and later will be broken when we change it.
| host_os = self._ss('os') | ||
| if host_os and 'Windows' in host_os: | ||
| self._conanfile.output.warn("Toolchain: Ignoring fPIC option defined for Windows") | ||
| else: |
There was a problem hiding this comment.
The point of the toolchain() feature is that for users working locally, they can build with their command line of the build system, and obtain the same result as if building with the build() method in the Conan cache. So having a very long command line that needs to include all the options, like:
$ meson -Db_staticpic=True -Db_ndebug=if-release -Dcpp_std=...If all those inputs were already introduced by Conan in the previous conan install command, the idea of the toolchain() is that they are saved in a file or something that the build system and the users can easily use.
|
PS: Not sure how to fix CI failure - it seems that it can't find |
|
btw, I totally agree with this, but talking in terms of CMake offers something that is working right now, something concrete, and it is useful as a reference... but yes, CMake is not perfect, if we can improve, let's do it.
|
|
Ok, we are getting somewhere =)
Well, that's currently impossible. Two machine files will be needed at the very least: native (aka build) and cross (aka host). And that's not counting user machine files. And, as I've mentioned above, only a very limited number of options can be (currently) passed via machine files. The easiest (and the dirtiest) way would be just to store the whole generated cmd line in a file (let's say
It's only useful to those who are familiar with CMake and it's toolchain :P |
Ok, it can more than one file... especially if Meson users are already using several files. The idea is not to pass all the arguments one by one in the command line. For example, in the CMake use-case, these are all the command line inputs that are substituted by a single file:
Probably this is the key point: If it is how it works now (release 0.5x) or it is a design decision and it won't ever be changed. ⬆️ ⬆️ ⬆️ ⬆️
I hope not, I prefer the environment variables approach... Conan can generate the corresponding
touché! You win this time 😄 |
AFAIK most arguments could be specified once the mentioned PR is merged (and, as far as I can see, main meson maintainer/author is not against it). TLDR:
|
|
@ milestone change. I don't think it'll be possible to complete this PR for any milestone if it receives feedback only once a month :P |
|
ping? |
|
@jgsogo @memsharded : sorry, but I'm rapidly losing motivation for working on this PR. The last series of feedback ended up in a series of open questions, answers for which are needed to continue the development of this feature. And it's been a month since then (and two months since this PR was submitted). I understand, that there might be other tasks/priorities that might be preventing you from working on this PR. But a simple "sorry, I don't think we will be able to look into this month/next month/this year/any time soon" would be much better than having zero feedback for a month. Since it just feels that devs are not interested in this PR anymore and actually demotivates the PR submitter from doing any contributions to the project altogether. Alternatively, if this feature is not something that you want, please, tell me so, so I could close this PR and move on :\ |
|
I am very sorry we couldn't review this on time for 1.28, and it is being moved for 1.29. You are right, and I understand it can be demotivating, but we do the best we can and work really hard on this project. Even with all the efforts, the amount of activity is so big that it is impossible to keep up with everything, and some things inevitably fail, there might be long delay times to process things, unattended issues, bugs to be fixed that stay there for years… Just as a couple of examples of the situation, the team has processed more than 1500 pull requests just in ConanCenter repo (not this Conan repo) https://github.com/conan-io/conan-center-index/pulls since January, or because of huge demand, we have been doing the online trainings, repeating sessions for 3 months in a row, repeating sessions in EMEA and USA. This takes an incredible amount of time too. 3 months ago we didn't know this was going to happen, and we thought after ConanDays we could focus more on development. Priorities change. We are definitely interested in this contribution, and moving forward towards a better integration with Meson. Believe me when I say that it is even more frustrating to be on this side, working so hard and still not being able to manage all things that need to be managed as fast as we would like. I really apologize for not being able to put attention to this PR, and I understand if you want to close or abandon it, or you aren't available anymore when we are able to provide feedback. I am still grateful for the contribution, and believe that it will be valuable. I am going to propose this plan to move this forward: This might take some time, and as I wouldn’t like to keep the frustration, I will propose that @SSE4 and @jgsogo continue the work. The plan would be the following:
I know this will be slower than desired, but breaking the job in chunks and going step by step will largely increase the likelihood of success, also allowing the team to fully understand the changes going forward. Many thanks again for your contribution |
That's the exactly the reason for the new helper. I've mentioned this before (#7155 (comment)): it's impossible to reproduce meson invocation via cmd with the old helper, because it uses environment variables to pass various information. So, the toolchain implementation will still have to use the same logic as in this PR, i.e. with your proposed solution there will be two different incompatible implementations all crammed up in the single helper instead of a proper helper with a single implementation. TLDR: you can't have a reproducible cmd with the old helper.
This can be done with this PR already: just remove/comment-out all the cross-related parts. |
|
@SSE4 please check when possible if something else needs to be incorporated, or this PR could be closed |
|
@memsharded this could be safely closed. new meson build helper and meson toolchain implement all the things. |
Changelog: (Feature | Fix | Bugfix): Describe here your pull request
Docs: TODO
https://github.com/conan-io/docs/pull/XXXXFixes: #7026
developbranch, 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.
Differences from stock Meson integration:
default_libraryif nosharedoptions was setbuild_subdirin__init__toolchainLimitations:
Can't read
--profile:build, thus works only in cases whennativecompiler is not required by meson project.TODO:
Questions:
Can we use Py3.x? meson requires Python 3.5+Can use 3.5, but it should not leak in global scope.cross-file/native-filekwarg or should they be moved tooptionsinstead?_get_cpu_family_and_endianness_from_archin case he has a custom unknown arch?dump()or in__init__()?MesonDefaultToolchainGeneratortoMesonDefaultToolchain?