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

fix for 'Ninja Multi-Config' generator #677

Merged
merged 1 commit into from
Jun 4, 2023

Conversation

mean-ui-thread
Copy link

  • I've checked this Git style guide. Yes
  • I've checked this CMake style guide. Yes
  • My change will work with CMake 3.2 (minimum requirement for Hunter). Yes
  • I will try to keep this pull request as small as possible and will try not to mix unrelated features. Yes

@mean-ui-thread
Copy link
Author

yes I am aware that Ninja Multi-Config has been added since cmake 3.17 and Hunter has to support cmake 3.2. However, since my change only compares generator strings that simply cannot occur in cmake 3.2, my propose change should cause no harm to the oldest cmake versions that hunter must maintain.

@NeroBurner
Copy link

Thanks for this improvement. Will this flag be set always when using Ninja generator, or does one need to explicitly enable this ninja-multi-config generator?

@mean-ui-thread
Copy link
Author

mean-ui-thread commented May 29, 2023

Thanks for this improvement. Will this flag be set always when using Ninja generator, or does one need to explicitly enable this ninja-multi-config generator?

@NeroBurner The latter: One does need the explicitly enable the ninja multi-config generator. Example:

cmake -G "Ninja Multi-Config" -B build/ninja-multi-config/ .

When using -G "Ninja", the new is_ninja_multi_config flag and the existing multiconfig_generator flags are guaranteed to be both false.

So far everything seems to work with ninja multi-config except when building for --config RelWithDebInfo or --config MinSizeRel but I believe it is a known limitation with hunter's current implementation that may also impacts xcode and visual studio (I need to confirm this). Hunter only produces Debug and Release binaries. If hunter with ninja-multi-config needs to work with RelWithDebInfo or MinSizeRel (by linking the regular release binaries in hunter's cache) please let me know and I will look into it (it can be done in a separate PR too if that is okay with you)

@NeroBurner
Copy link

Thanks for the clarification. The changes seem minimal, so @rbsheth please merge.

Yes, Release,Debug are the default build types built by hunter. But you can override this with a toolchain file defining the variable HUNTER_CONFIGURATION_TYPES

@mean-ui-thread
Copy link
Author

@NeroBurner good to know! Thanks!

@mean-ui-thread
Copy link
Author

Do you need anything else from me for merging this pull request? it looks like the CI for it might be stuck...

Right now, my team is manually patching .hunter/_Base/Download/Hunter/0.24.16/09668c2/Unpacked/cmake/modules/hunter_sanity_checks.cmake by hand because we much prefer using Ninja Multi-Config, It's not ideal but it will do for now.

@rbsheth rbsheth merged commit 0bfdf60 into cpp-pm:master Jun 4, 2023
@rbsheth
Copy link
Member

rbsheth commented Jun 4, 2023

Sorry for the delay here. Merged!

@mean-ui-thread mean-ui-thread deleted the patch-1 branch June 4, 2023 20:39
@mean-ui-thread
Copy link
Author

Thank you! 😊

@NeroBurner
Copy link

neandreithal pushed a commit to regulaforensics/hunter that referenced this pull request Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants