👩‍🌾 Find rendering engines quietly, it's ok if they're not present

[chapulina]

🦟 Bug fix

Fixes #281

Summary

Requires gazebosim/gz-cmake#164

All rendering engines are optional to Ignition Rendering, so I think that we shouldn't throw warnings when they can't be found.

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

[codecov]

Codecov Report

Merging #309 (e03f729) into ign-rendering3 (e3d56ad) will increase coverage by 9.31%.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##           ign-rendering3     #309      +/-   ##
==================================================
+ Coverage           50.59%   59.90%   +9.31%     
==================================================
  Files                 129      159      +30     
  Lines               11887    19356    +7469     
==================================================
+ Hits                 6014    11596    +5582     
- Misses               5873     7760    +1887     

Impacted Files	Coverage Δ
include/ignition/rendering/base/BaseVisual.hh	81.05% <0.00%> (-10.61%)	⬇️
ogre2/src/Ogre2Material.cc	93.66% <0.00%> (-1.82%)	⬇️
include/ignition/rendering/base/BaseMesh.hh	76.47% <0.00%> (-1.18%)	⬇️
ogre2/src/Ogre2GaussianNoisePass.cc	88.88% <0.00%> (-1.12%)	⬇️
ogre2/src/Ogre2DepthCamera.cc	93.32% <0.00%> (-0.32%)	⬇️
ogre/src/OgreMesh.cc	0.00% <0.00%> (ø)
ogre/src/OgreLight.cc	0.00% <0.00%> (ø)
ogre/src/OgreCamera.cc	0.00% <0.00%> (ø)
ogre/src/OgreMarker.cc	0.00% <0.00%> (ø)
ogre/src/OgreRenderTarget.cc	0.00% <0.00%> (ø)
... and 78 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3d56ad...e03f729. Read the comment docs.

[scpeters]

I see that this cleans up warnings in common use cases, but it feels like we are potentially hiding information in other cases. If you build ign-rendering up through ign-gazebo from source without any version of OGRE installed, with this change you will get no configuration warnings but instead have problems at runtime (I think). That sounds like a bad user experience.

If we decide that's an edge case and take a calculated decision to prioritize less noise for most users over helpful information in an edge case, then we may need to add some troubleshooting documentation to aid in diagnosing runtime errors when there are no rendering plugins available

[mxgrey]

I'll be responding to this comment from another thread in here since @chapulina requested that the conversation be held here.

is there a reason to have a different behaviour from find_package?

Yes, because ign_find_package does much more than find_package. CMake's find_package is meant to be a (relatively) low-level function, and in the broader context of worldwide cmake usage there are plenty of cases where someone might call find_package multiple times in different ways to choose one potential dependency over another or add/remove features based on what dependency is available. As a general-purpose build configuration tool, cmake has a responsibility to directly support all those different possible usages and doesn't try to do anything more for the user.

ign_find_package is an intentionally higher-level tool, and its job is to enforce a more narrow, consistent, and structured way of configuring a package and its components. Part of that job is tying together optional components and the dependencies that they need. When you use REQUIRED_BY in ign_find_package, then that invocation of ign_find_package will determine whether or not certain components will be configured. When you use REQUIRED, then that invocation of ign_find_package will determine whether anything will be configured.

But even putting aside the differences between find_package and ign_find_package, consider this: When you use regular find_package with both QUIET and REQUIRED, you will get a completely failed configuration if the package cannot be found. For example, try putting this line in a CMakeLists.txt (assuming you don't have any package installed called fake):

find_package(fake REQUIRED QUIET)

When you try to configure, cmake will very loudly print the following to stderr, and return with a failed exit code:

CMake Error at CMakeLists.txt:9 (find_package):
  By not providing "Findfake.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "fake", but
  CMake did not find one.

  Could not find a package configuration file provided by "fake" with any of
  the following names:

    fakeConfig.cmake
    fake-config.cmake

  Add the installation prefix of "fake" to CMAKE_PREFIX_PATH or set
  "fake_DIR" to a directory containing one of the above files.  If "fake"
  provides a separate development package or SDK, be sure it has been
  installed.

This is the behavior of find_package when both QUIET and REQUIRED are used together. If anything, it would be more of a deviation from standard cmake behavior if we have ign_find_package completely suppress any feedback to the user when a REQUIRED or REQUIRED_BY flag is passed to ign_find_package.

As a side note, we intentionally never pass the REQUIRED flag from ign_find_package down to find_package because using find_package( ... REQUIRED) will cause cmake to immediately quit when the package isn't found, but we prefer to identify all the missing packages first before quitting the configuration so we can report all missing dependencies to the user at once.

In my comment here I said

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

which I'd say is more consistent with the native behavior of find_package.

We need to support the idea that not all components are required.

This is already supported. We issue a configuration warning when a component cannot be built, not an error, and cmake returns with a successful exit code.

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.

To say that an issue is "non critical" but also "needs" to be fixed seems self-contradictory to me. I'll assume you mean "should be fixed" rather than "needs to be fixed", like a compilation warning. So the state of the configuration is undesirable but can continue to function. I'm having a hard time thinking of a case where a configuration could be in such a state. I think build configurations are usually either good (required dependencies are found, versions are acceptable, whatever necessary flags are specified) or messed up (a required dependency wasn't found, a version doesn't match the requirements, or some flag needed from the user wasn't set).

But if we are dead set on this "non-critical error" interpretation of warning, then I guess we could issue a STATUS message when a component's dependency couldn't be found, instead of issuing a warning. I would really discourage this, because it will reduce the visibility of this important information (in ordinary use of colcon, STATUS messages won't be visible anywhere besides the build log), but if I can't persuade people to reconsider the "non-critical error" definition of a configuration warning, then I guess it would be a fair compromise. However I'd say we absolutely must issue a configuration error for REQUIRED, or else we'd really be going off the rails in terms of how a build configuration tool should behave.

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.

I'm perplexed about why the HAVE_<component> was put in. If you remove those lines and just always specify

ign_configure_build(QUIT_IF_BUILD_ERRORS COMPONENTS ogre optix ogre2)

everything should always work exactly the same way. ign_find_package will automatically make sure that components with missing dependencies do not get built.

I'd prefer we don't bother users with warnings when they aren't doing anything wrong.

Like @scpeters pointed out, if we say nothing when the dependencies for all the plugins are missing, then users will be very bothered when they have none of the dependencies installed and don't know why ign-rendering isn't working for them at all. I certainly know that I would personally be very irritated if a package can be successfully configured and built with no notice or warning issued to me, but doesn't actually function at all when I go to run it.

[mxgrey]

Here's a suggestion to chew on: What if we leave it as a warning for the sake of visibility, but make the message friendlier, like Skipping component [${component}] - ${${PACKAGE_NAME}_msg}. That way users don't get the impression that something is broken, but they get clearly notified that a component will be automatically skipped due to a missing dependency.

[adlarkin]

I'm joining this discussion late, but here's how I see/understand it:

We have packages with optional dependencies (like rendering engines for ign-rendering). For the case of ign-rendering, we don't want users to think that something is wrong if they don't have all rendering engines configured, but we also don't want them to be completely clueless if they have no rendering engines configured at all. Currently, we let users know what is/is not configured via cmake warnings, but this is resulting in ign-rendering CI to appear as unstable due to a warning of optix not being configured.

I agree with @mxgrey that warnings aren't necessarily "bad" (they're just providing information to the user - if the contents of a warning message are bad or fatal, then this should be an error message instead), and that we shouldn't suppress them or use something like a STATUS message, or else users may face the scenario pointed out by @scpeters where if no rendering engines are configured, configuration/build succeeds, but using the package results in unexpected behavior.

So, the question is, how do we provide users with information about missing optional dependencies that can be suppressed for cases like CI? I believe that @mxgrey has proposed a nice solution in gazebosim/gz-cmake#165. If we were to use this approach, we wouldn't need to change how packages are found/configured for ign-rendering. Instead, we should just be able to pass -DSKIP_optix=true to the cmake invocation in CI to suppress the missing optix warning. This would also allow for ign-rendering users to be notified of which rendering engines they do/do not have configured when configuring ign-rendering to avoid unexpected runtime behavior, and they can always suppress this information if they don't need it.

[chapulina]

When you use regular find_package with both QUIET and REQUIRED

I see, I didn't think of REQUIRED_BY as similar to REQUIRED, see gazebosim/gz-cmake#165 (review)

I guess we could issue a STATUS message when a component's dependency couldn't be found

I believe this is already being done by the underlying find_package when it says -- Looking for OptiX - not found. In fact, that's also what ign-cmake did until version 2.6.2, before gazebosim/gz-cmake#159.

If you build ign-rendering up through ign-gazebo from source without any version of OGRE installed, with this change you will get no configuration warnings but instead have problems at runtime (I think).

That's not entirely true. If you compile with ogre2, but not ogre1, then ign gazebo lights.sdf will fail to load the rendering engine, because it has ogre hardcoded. Most worlds, as well as the default configuration, have ogre2 hardcoded. When you don't have the requested rendering engine, you'll see runtime errors explaining what's missing.

The key thing is that there may be ign-gazebo users who aren't using any of these engines. And that's fine, supported and encouraged.

warnings aren't necessarily "bad"

Yup, warnings are not errors. But they shouldn't be ignored either. Most developers will either want to fix them or explicitly disable them. So it prompts an action, and that's my main concern here.

missing optional dependencies that can be suppressed for cases like CI?

Our CI is not the only place where Ignition libraries are built from source. I think that saying "you can just explicitly skip anything you don't want" to all downstream users is adding an unnecessary burden into workflows that aren't doing anything wrong. For example, from the moment Bullet support is added to ign-physics3, I wouldn't want to introduce warnings to existing users that they suddenly need to deal with.

---

It's a balancing act to not make our users pay for what they don't use, while still keeping beginner friendly default behaviour.

Requiring that users explicitly set SKIP_ variables at configuration time feels like an unnecessary burden to me. And for the beginners, if we always warn, they will probably always see some warning and not know for sure if they're missing something. Maybe one compromise is making only the non-default engines QUIET. More thoughts on this in gazebosim/gz-cmake#165 (comment).

[chapulina]

We're putting the burden of skipping what they don't need on downstream users for now. We can revisit if this becomes too much for too many users to handle.