👩‍🌾 ign_find_package: respect QUIET

[chapulina]

Summary

This is meant to help with gazebosim/gz-rendering#281.

Judging by the documentation, I believe QUIET wasn't being used to suppress warnings from the macro on purpose. But I can't think of a reason not to use that argument 🤔

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

https://github.com/osrf/buildfarmer/issues/181

[mxgrey]

I'm not sure if I would consider this the "right" approach for suppressing warnings about the dependencies of optional components. What if a user wanted to build that component, but cmake quietly fails to configure the component because of a missing dependency?

Instead I think we should make it

    elseif(ign_find_package_REQUIRED_BY)

      foreach(component ${ign_find_package_REQUIRED_BY})
        if(NOT SKIP_${component})
          # Otherwise, if it was only required by some of the components, create
          # a warning about which components will not be available.
          ign_build_warning("Cannot build component [${component}] - ${${PACKAGE_NAME}_msg}")

          # Also create a variable to indicate that we should skip the component
          set(SKIP_${component} true)
        endif()
      endforeach()

    else()
      if(NOT ign_find_package_QUIET)
        ign_build_warning(${${PACKAGE_NAME}_msg})
      endif()
    endif()

Then for gazebosim/gz-rendering#281 we should have the build farm specify -DSKIP_optix in its cmake call. If optix is supposed to be skipped by the build farm, then the build farm should declare that explicitly.

If QUIET is specified and the dependency isn't required by anything, then I suppose it's fine to let it quietly fail.

[mxgrey]

There's a catch with my proposal, though: cmake will only report one missing dependency for a component in each run of the configuration.

To make my proposal really sound, we should create a new set of variables like INTERNAL_SKIP_${component} that only get set internally (and we would no longer set SKIP_${component} internally), then change this line to

if(NOT SKIP_${component} AND NOT INTENRAL_SKIP_${component})

[chapulina]

I think we should separate the 2 things here:

1. How the QUIET keyword should behave
2. How to handle optional components

For 1.: is there a reason to have a different behaviour from find_package? When I do find_package(QUIET), I don't want any warnings on failure to find. I'd expect ign_find_package to behave the same way, that's all. I should be able to choose that as a user and this is really all this pull request is about. No opinions about CI, components, optional engines. Just the principle of respecting what the user requested.

For 2.:

What if a user wanted to build that component, but cmake quietly fails to configure the component because of a missing dependency?

We need to support the idea that not all components are required. For example, ign-plugin now has a dependency on ign-tools for command line tools, and it's expected that users will use ign-plugin without that component. We don't even ship debs for it.

I know that there isn't a consensus about the role of warnings, but in general, we've been warning users when something non critical needs to be fixed. When it comes to optional dependencies or components, nothing needs to be fixed, so nothing needs to be warned. The user will get the message that the dependency was not found, it just won't be a warning.

In any case, I believe the conversation about 2. is tangential to this pull request because of 1..

Then for gazebosim/gz-rendering#281 we should have the build farm specify -DSKIP_optix in its cmake call.

There's a mechanism at the CMake level to skip support for specific engines if the dependency is not available. That works across all platforms, for all users, without hardcoding anything into our CI.

We encourage our users to use Ignition Rendering with any of the 3 officially supported rendering engines, or none of them, and these are first-class use-cases. I'd prefer we don't bother users with warnings when they aren't doing anything wrong.

I recommend we continue this conversation at the ign-rendering PR.

[scpeters]

I made a relevant commentin gazebosim/gz-rendering#309 (comment)

[chapulina]

Leaving QUIET quiet for now