Skip to content

Add support for "Ninja Multi-Config" CMake generator (#8813) - #8814

Merged
memsharded merged 2 commits into
conan-io:developfrom
mrjoel:feature/recognize-ninja-multiconfig
Apr 20, 2021
Merged

Add support for "Ninja Multi-Config" CMake generator (#8813)#8814
memsharded merged 2 commits into
conan-io:developfrom
mrjoel:feature/recognize-ninja-multiconfig

Conversation

@mrjoel

@mrjoel mrjoel commented Apr 15, 2021 •
edited by czoido
Loading

Copy link
Copy Markdown
Contributor

The "Ninja Multi-Config" generator was added in CMake 3.17. Add
support for recognizing it as multi-config from
is_multi_configuration(), as well as inserting needed envvars as with
regular (single configuration) Ninja usage.

Signed-off-by: Joel Johnson mrjoel@lixil.net

Changelog: Fix: Recognize Ninja Multi-Config as a CMake multi-configuration generator.
Docs: Omit

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

@mrjoel
Add support for "Ninja Multi-Config" CMake generator (conan-io#8813)
a596693 
The "Ninja Multi-Config" generator was added in CMake 3.17. Add
support for recognizing it as multi-config from
is_multi_configuration(), as well as inserting needed envvars as with
regular (single configuration) Ninja usage.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>
@CLAassistant

CLAassistant commented Apr 15, 2021 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@mrjoel mrjoel mentioned this pull request Apr 15, 2021
Open
memsharded
memsharded reviewed Apr 15, 2021
View reviewed changes

@memsharded memsharded left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this. If we want to add support for Ninja Multi-Config, we need to do a few more things:

  • It is very important that changes to build systems are at least (or only) targeted to the new tools in conan.tools.cmake, not only the old conans.xxxx space.
  • We are adding functional tests for almost all things. This was merged just today: #8005. We really need to make sure things really work, otherwise there is too much untested code that easily breaks.

We really want to add support for Ninja Multi-Config, but it will require a bit more effort. Adding tests is not that straightforward, we can help with this at some point if necessary.

@mrjoel

mrjoel commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Yep, I just needed a minimal is_multi_configuration test to work for some internal items. I started to take a look at adding test cases but also saw it as not straightforward. I didn't see any prior activity on the Ninja Multi-Config generator though so wanted to get something out as a starting point.

@mrjoel

mrjoel commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author 
* It is very important that changes to build systems are at least (or only) targeted to the new tools in `conan.tools.cmake`, not only the old `conans.xxxx` space.

What additional changes would you expect here? I did look at a brief look in conan/tools/cmake and the only Ninja usage are currently "Ninja" in generator which will match the multi-config one as well (and the behavior should also be desired), or in toolchain.py uses the is_multi_configuration(generator) path.

@memsharded

memsharded commented Apr 16, 2021

Copy link
Copy Markdown
Member

What additional changes would you expect here? I did look at a brief look in conan/tools/cmake and the only Ninja usage are currently "Ninja" in generator which will match the multi-config one as well (and the behavior should also be desired), or in toolchain.py uses the is_multi_configuration(generator) path.

Yes, this was more a general consideration of how to think regarding changes in build system integrations, and being aware that the future integrations for Conan 2.0 are those in conan.tools.xxx.

Sure, thanks, this is good as a starting point and will help to push things. We will try to help with the tests, it is important to add some, specially in the new part that is currently under heavy development.

@memsharded memsharded added this to the 1.36 milestone Apr 20, 2021
@memsharded
memsharded merged commit 1a16f96 into conan-io:develop Apr 20, 2021
@memsharded

memsharded commented Apr 20, 2021

Copy link
Copy Markdown
Member

We will be adding tests for Ninja multi-config in CI when possible, but that might take some time. As this change seems no risk at all, I think it is ok to merge it now for 1.36, but I have changed the changelog, until we have full proper testing, better not to say that we added support for it.

@mrjoel
mrjoel deleted the feature/recognize-ninja-multiconfig branch April 20, 2021 12:33
@memsharded memsharded mentioned this pull request Apr 20, 2021
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@memsharded memsharded memsharded left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.36

Development

Successfully merging this pull request may close these issues.

3 participants

@mrjoel @CLAassistant @memsharded