-
Notifications
You must be signed in to change notification settings - Fork 30
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
Fix problems on IgnOGRE when version is not found #175
Conversation
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
list(APPEND OGRE_INCLUDE_DIRS ${dir_include}) | ||
endforeach() | ||
list(APPEND OGRE_INCLUDE_DIRS ${dir_include}) | ||
endforeach() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this block of code is now moved to to the Windows section (instead of if (OGRE_FOUND)
)? When building ign-rendering with this branch, I get error about not being able to find an ogre include header during compilation. How about just protecting it with a separate if (OGRE_FOUND)
check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, good catch! 0b05939
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to test this locally. Any tips for what setup to use to re-produce this behavior?
@adlarkin, I'm testing it with this PR: gazebosim/gz-rendering#376 Without these changes, I get the |
I'm not able to reproduce the error without these changes 🤔 I built a workspace with the |
hmm I just rebuilt my workspace and I can't reproduce it now too.. But I think we may run into this error with no ogre debs installed in the system though. |
I think that the changes in the PR make sense. I'd just like to be able to test locally if possible in order to avoid new breaks that we might not be aware of. But, maybe it's difficult to create a scenario to test this, I'm not sure. |
Signed-off-by: Jose Luis Rivero <[email protected]>
…to jrivero/fix_ogre_not_found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes make sense. We can get this in
@@ -1,6 +1,9 @@ | |||
## Ignition CMake 2.x | |||
|
|||
### Ignition CMake 2.X.X (20XX-XX-XX) | |||
### Ignition CMake 2.X.X (2021-08-23) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to make an ign-cmake
release once this PR is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to since seems to be causing problems on current CI. See gazebosim/gz-rendering#376 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving here by now. Going to merge to solve problems on CI.
* Fix problems on IgnOGRE when version is not found Signed-off-by: Jose Luis Rivero <[email protected]>
🦟 Bug fix
Summary
When Ogre is not found (looking for 1.10 in a 1.9 environment), the builds fail with:
There is a final block starting at line 240 that is not protected by the check of
OGRE_FOUND
leavingOGRE_VERSION
empty and making the whole thing to crash. I did not find any reason not to include it underOGRE_FOUND
.Checklist
Note to maintainers: Remember to use Squash-Merge