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

Feature: set CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON #8599

Merged
merged 6 commits into from Mar 23, 2021

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Mar 4, 2021

closes: #8582
/cc @mpusz
Changelog: Feature: Set CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON.
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.

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.

Signed-off-by: SSE4 <tomskside@gmail.com>
@memsharded memsharded added this to the 1.35 milestone Mar 15, 2021
@solvingj
Copy link
Contributor

It seems to me that this should be controlled via [conf]. As @SSE4 has said in the past, if many people using CMake try to move over to the toolchain generator and it turns out this behavior is breaking for them, it would be great if they had a way to opt-out right away.

Copy link
Contributor

@solvingj solvingj left a comment

Choose a reason for hiding this comment

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

Consider adding [conf] item for this.

@memsharded
Copy link
Member

Sounds good, lets add a [conf] to opt-out this behavior, I'd suggest something like:

[conf]
tools.cmake.cmaketoolchain:find_package_prefer_config=False

Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4
Copy link
Contributor Author

SSE4 commented Mar 17, 2021

now CMAKE_FIND_PACKAGE_PREFER_CONFIG is controlled by tools.cmake.cmaketoolchain:find_package_prefer_config

elif isinstance(self.find_package_prefer_config, six.integer_types):
self.find_package_prefer_config = bool(self.find_package_prefer_config)
self.find_package_prefer_config = "ON" if self.find_package_prefer_config else "OFF"

Copy link
Contributor Author

@SSE4 SSE4 Mar 17, 2021

Choose a reason for hiding this comment

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

two notes:

Signed-off-by: SSE4 <tomskside@gmail.com>
Signed-off-by: SSE4 <tomskside@gmail.com>
@@ -143,6 +148,15 @@ def __init__(self, conanfile, **kwargs):

self.build_type = None

self.find_package_prefer_config = \
conanfile.conf["tools.cmake.cmaketoolchain"].find_package_prefer_config
if self.find_package_prefer_config is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This is not opting-out, it is opting-in please recall my comment:

Sounds good, lets add a [conf] to opt-out this behavior, I'd suggest something like

Then simplify code, as you said they are always strings, so go for a oneliner:

if self.find_package_prefer_config is not None:
    self.find_package_prefer_config = "OFF" if self.find_package_prefer_config.lower() in ("false", "0", "off") else "ON"

Copy link
Contributor Author

@SSE4 SSE4 Mar 18, 2021

Choose a reason for hiding this comment

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

I am not sure I understand. this is opt-out, the behavior matches CMake exactly:

  • conf not defined -> CMAKE_FIND_PACKAGE_PREFER_CONFIG not defined
  • conf=True -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
  • conf=False -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=OFF

Copy link

Choose a reason for hiding this comment

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

The whole idea was to have it ON by default so inexperienced users using conanfile.txt without any conf sections have a friendly dev env. Advanced users may want to disable it if they so desire.

As a side note, still there are many inexperienced users out there both for Conan and CMake. Doing things that we consider trivial is not easy for them at all. For the last two days, I am remotely helping one of them to enable mp-units support via conanfile.txt even though I thought that my docs are clear: https://mpusz.github.io/units/usage.html#conan-cmake-release. Today I finally saw that person's CMake file and it turned out to be:

image

Copy link
Contributor Author

@SSE4 SSE4 Mar 18, 2021

Choose a reason for hiding this comment

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

okay, I added the ON as default again. there was a confusion because initially I implemented ON as default, then others said it should be controlled by conf instead. hopefully the current behavior makes sense:

  • conf not defined -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
  • conf=True -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
  • conf=False -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=OFF

(although it's not exactly an opt out for the default CMake behavior, it should help for your case)

Copy link

Choose a reason for hiding this comment

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

Actually, in this case, a user correctly applied CONFIG REQUIRED as suggested in my docs. The problem was with the last two lines 😉

Copy link
Member

Choose a reason for hiding this comment

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

@SSE4 my request was:

Sounds good, lets add a [conf] to opt-out this behavior, I'd suggest something like:

A config required to opt-out. That means that by default it is ON, and you need to do something proactively to opt-out of that feature. If you need to define a conf for activating the feature, it is not an opt-out feature, it is an opt-in feature.

@memsharded memsharded merged commit 8fed505 into conan-io:develop Mar 23, 2021
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.

[feature] Set CMAKE_FIND_PACKAGE_PREFER_CONFIG in conan_toolchain.cmake
4 participants