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

CMakeDeps: heed <PackageName>_FIND_QUIETLY #12967

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

praetorian20
Copy link
Contributor

@praetorian20 praetorian20 commented Jan 24, 2023

Changelog: Feature: Conan packages using the CMakeDeps generator will now stop printing status messages if the QUIET argument is passed to the respective find_package() CMake call.

Docs: Omit

Fixes #9959
Fixes #10857

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor Author

@praetorian20 praetorian20 left a comment

Choose a reason for hiding this comment

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

@memsharded this PR is related to our discussion here. Please let me know if I'm on the right track with this change.

@@ -45,6 +45,12 @@ def template(self):
message(FATAL_ERROR "The 'CMakeDeps' generator only works with CMake >= 3.15")
endif()

if({{ file_name }}_FIND_QUIETLY)
set({{ file_name }}_MESSAGE_MODE VERBOSE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I unset {{ file_name }}_MESSAGE_MODE at the end of either/both files?

Copy link
Member

Choose a reason for hiding this comment

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

I think it might not be necessary, as the scope is well defined, shouldn't be an issue.

@@ -58,6 +58,10 @@ def template(self):
get_filename_component(_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
file(GLOB DATA_FILES "{{data_pattern}}")

if(NOT DEFINED {{ file_name }}_MESSAGE_MODE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it OK to assume the code generated by config..py is included before this? Or should I replicate the if({{ file_name }}_FIND_QUIETLY) check here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is safe, this file will always be included from xxx-config.cmake, if that is what you mean, so {{ file_name }}_MESSAGE_MODE should already be defined at this point.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I think this is looking good.
But the CI is broken, it seems some missing bracket or something for jinja rendering, please check https://ci.conan.io/blue/organizations/jenkins/ConanTestSuite/detail/PR-12967/1/pipeline

Also, it would be necessary to add a unittest or at least a check in one existing test.

@@ -45,6 +45,12 @@ def template(self):
message(FATAL_ERROR "The 'CMakeDeps' generator only works with CMake >= 3.15")
endif()

if({{ file_name }}_FIND_QUIETLY)
set({{ file_name }}_MESSAGE_MODE VERBOSE)
Copy link
Member

Choose a reason for hiding this comment

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

I think it might not be necessary, as the scope is well defined, shouldn't be an issue.

@@ -58,6 +58,10 @@ def template(self):
get_filename_component(_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
file(GLOB DATA_FILES "{{data_pattern}}")

if(NOT DEFINED {{ file_name }}_MESSAGE_MODE)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is safe, this file will always be included from xxx-config.cmake, if that is what you mean, so {{ file_name }}_MESSAGE_MODE should already be defined at this point.

Conan packages using the CMakeDeps generator will now stop printing
status messages if the QUIET argument is passed to the respective
find_package() CMake call.

Fixes conan-io#9959
Fixes conan-io#10857
@praetorian20 praetorian20 force-pushed the feature/quiet_find_package branch from 7e15bfd to aaaf8cc Compare January 25, 2023 05:47
@praetorian20
Copy link
Contributor Author

Sorry about the broken template code, I think it's fixed in the latest patchset. I'll look into adding unit tests but I haven't been able to run them successfully yet. I keep running into this error with a lot of the cmakedeps tests even though I have cmake 3.22 installed - Failed: Required 'cmake' tool version 'Any' is not available

I'm following the instructions for running tests using pytest and executing this command - venv/bin/python -m pytest . -k cmakedeps. I'm using WSL Ubuntu 22.04, will try tomorrow on a RHEL 8 machine in case it's a WSL issue.

@memsharded
Copy link
Member

Sorry about the broken template code, I think it's fixed in the latest patchset. I'll look into adding unit tests but I haven't been able to run them successfully yet. I keep running into this error with a lot of the cmakedeps tests even though I have cmake 3.22 installed - Failed: Required 'cmake' tool version 'Any' is not available

No worries! Yes, we have some dicts defining tools and versions in conftest.py and it can be overriden in conftest_user.py, to default cmake to a different version and location, it might be necessary so Conan uses your own cmake

@memsharded
Copy link
Member

I have added a unit-test

@praetorian20
Copy link
Contributor Author

Thank you for adding the unit test and for the pointer to conftest_user, now I'm able to get the cmakedeps unit tests to pass.

@memsharded memsharded added this to the 1.58 milestone Jan 26, 2023
@czoido czoido merged commit a33acaa into conan-io:develop Jan 26, 2023
@praetorian20 praetorian20 deleted the feature/quiet_find_package branch January 26, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants