Fix check for ogre when using USE_UNOFFICIAL_OGRE_VERSIONS

[j-rivero]

🦟 Bug fix

Fixes #376 (comment)

Summary

The use of USE_UNOFFICIAL_OGRE_VERSIONS totally missed the check for Ogre. The PR should make it unconditional.

[codecov]

Codecov Report

Merging #453 (5ea5506) into ign-rendering6 (f93c796) will not change coverage.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           ign-rendering6     #453   +/-   ##
===============================================
  Coverage           53.49%   53.49%           
===============================================
  Files                 192      192           
  Lines               19560    19560           
===============================================
  Hits                10464    10464           
  Misses               9096     9096           

---

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 f93c796...5ea5506. Read the comment docs.

[iche033]

should we target this at ign-rendering3 / citadel then forward port the changes?

[traversaro]

This comment may be a bit misleading, as OGRE_FOUND is false either if ogre 1.10 or greater was not found, or also if USE_UNOFFICIAL_OGRE_VERSIONS is explicitly set to TRUE .

[traversaro]

Hi @j-rivero, while debugging some failed conda builds in conda-forge/libignition-rendering4-feedstock#21, I am afraid I found something problematic in how this code is structured. Quoting conda-forge/libignition-rendering4-feedstock#21 (comment) :

The problem is that at the moment the CMake code is calling ign_find_package(IgnOGRE VERSION 1.10 QUIET), and after that ign_find_package(IgnOGRE VERSION 1.10 COMPONENTS ${ign_ogre_components} REQUIRED_BY ogre PRIVATE_FOR ogre). The problem is that the IgnOgre::IgnOgre target created by the first call (that does not has any component properly set in its IMPORTED_LINK_INTERFACE_LIBRARIES property) is not changed at all by the second ign_find_package call, as ign_import_target silently does nothing if a target with the same name already exists (see https://github.com/ignitionrobotics/ign-cmake/blob/ignition-cmake2_2.9.0/cmake/IgnImportTarget.cmake#L67).

In my specific case, this created a problem that could be solved by adding COMPONENTS ${ign_ogre_components} to the first ign_find_package. However, I wonder if to reduce the problems like this in the future if it could make sense to have a single call:

ign_find_package(IgnOGRE VERSION 1.9.0
      COMPONENTS ${ign_ogre_components}
      REQUIRED_BY ogre
      PRIVATE_FOR ogre)

and then just print a warning depending on the Ogre version found and the value of the USE_UNOFFICIAL_OGRE_VERSIONS variable?

[j-rivero]

Hi @j-rivero, while debugging some failed conda builds in conda-forge/libignition-rendering4-feedstock#21, I am afraid I found something problematic in how this code is structured. Quoting conda-forge/libignition-rendering4-feedstock#21 (comment) :

I see, sorry for the inconvenience.

However, I wonder if to reduce the problems like this in the future if it could make sense to have a single call:

and then just print a warning depending on the Ogre version found and the value of the USE_UNOFFICIAL_OGRE_VERSIONS variable?

Sounds like the proper solution to the whole mess. Would you mind sending the PR against ign-rendering3? If you don't have time, no worries, I'll take it, just trying to minimize the persons in the loop #383

Thanks Silvio!

[j-rivero]

Sounds like the proper solution to the whole mess. Would you mind sending the PR against ign-rendering3? If you don't have time, no worries, I'll take it, just trying to minimize the persons in the loop #383

Let's follow in #465

[traversaro]

Hi @j-rivero, while debugging some failed conda builds in conda-forge/libignition-rendering4-feedstock#21, I am afraid I found something problematic in how this code is structured. Quoting conda-forge/libignition-rendering4-feedstock#21 (comment) :

I see, sorry for the inconvenience.

However, I wonder if to reduce the problems like this in the future if it could make sense to have a single call:

and then just print a warning depending on the Ogre version found and the value of the USE_UNOFFICIAL_OGRE_VERSIONS variable?

Sounds like the proper solution to the whole mess. Would you mind sending the PR against ign-rendering3? If you don't have time, no worries, I'll take it, just trying to minimize the persons in the loop #383

Thanks Silvio!

Sorry, I had missed this message!

[j-rivero]

Sorry, I had missed this message!

no worries!