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

Find Ogre 2-2 #157

Merged
merged 5 commits into from
Mar 24, 2021
Merged

Find Ogre 2-2 #157

merged 5 commits into from
Mar 24, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Feb 24, 2021

Signed-off-by: ahcorde [email protected]

🦟 Bug fix

Related with this issue gazebosim/gz-rendering#223

Summary

This PR allows to find OGRE 2-2

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

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from mjcarroll February 24, 2021 13:51
@ahcorde ahcorde self-assigned this Feb 24, 2021
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Feb 24, 2021
Copy link
Contributor

@mjcarroll mjcarroll 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 we're going to have to maintain compatibility with both for a little bit, since we aren't bumping the major version on ign-cmake. This will actually stop OGRE 2.1 from being found.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Mar 17, 2021
@mjcarroll mjcarroll marked this pull request as ready for review March 24, 2021 19:58
@@ -53,6 +53,14 @@ if (${IgnOGRE2_FIND_VERSION_MAJOR})
endif()
endif()

if (${IgnOGRE2_FIND_VERSION_MINOR} VERSION_EQUAL "2")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking on the future and to avoid the assumption that not 2 is 1, could work something like:

message(STATUS "-- Finding OGRE 2.${IgnOGRE2_FIND_VERSION_MINOR}")
set(OGRE2_INSTALL_PATH "OGRE-2.${IgnOGRE2_FIND_VERSION_MINOR}")

instead of the whole if block?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Testing the thing in a colcon workspace with current ogre-2.1 package + rendering4, the thing compiles just fine. I would not expect regressions with current changes.

We probably need to make some changes to make it work on Windows with different package managers/buildsystems. That could be done in a different PR.

I just notice that the ogre-2.2 example can work with ogre-2.1 using current versions of ign-cmake from .debs returning ogre-2.1 paths. We probably need to implement checks on the consumers of this ign-cmake version (i.e: ign-rendering) to pull these changes into the mix to work correctly.

Signed-off-by: Michael Carroll <[email protected]>
@j-rivero j-rivero merged commit d71927a into ign-cmake2 Mar 24, 2021
@j-rivero j-rivero deleted the ahcorde/ogre2-2 branch March 24, 2021 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants