-
Notifications
You must be signed in to change notification settings - Fork 212
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
Upgrade to OGRE 1.10.12 to get OSX fix for Sierra #380
Conversation
Which issue is this? (it currently runs on high sierra) That's definitely been an issue in the past, but I thought it was already fixed. Separately would be to see if a newer version fixes building on Mojave (there are some lingering issues). |
The draft PR feature is new to me, didn't realize it would go to you! I haven't tested this on any platform but OSX yet. I am running on Sierra, (incomplete High Sierra support on internal machines here). All 232 packages successfully built, including rviz2, but I was seeing a consistent segfault crash on startup in the CocoaWindow layer of OGRE
With this update, rviz2 happily runs here now. I'll need to test it on Linux and Windows though before we call this good. |
No worries, I think draft pr only avoids notifications when using "code owners", but I'm not sure. I wasn't going to review until you changed it to a non-draft, I was just curious. |
I am running in the same problems with the segfault on startup. I am running Sierra as well. However, when trying to compile this patch I get a compiler error:
does that tell you anything? EDIT: |
I remember when I did this that the I did some digging and found that the type of the |
590a350
to
53feb03
Compare
@thomas-moulard - please run the following CI job:
|
@emersonknapp do you have an idea on what exactly changed within Sierra that this error occured? I was told the current CI build machines are running Sierra as well and the builds didn't break on their end - saying all the nightlies look good so far. Does the error occur due to an update to one of Ogre's dependencies, such as cocoa? Also, I am not having a Mojave machine around, but do you think you could give this a shot on Mojave to see if this works there as well? |
Well - the build wasn't failing on Sierra, it was only the runtime error. I'm not familiar with the automated testing being done on rviz, does the frontend get launched on those Sierra build machines? I'm not sure if the problem has always existed on Sierra or if it's new - it was my first attempt to use rviz2 on OSX. My macbook at home may be running Mojave, I can check tonight and give it a try if so. |
There are fairly extensive tests for rviz in ROS 2, but I don't know if the gui is launched by default. That's why I asked @Karsten1987 in our triage meeting if he had tried running it on one of the CI machines to see if the problem is there too. I know for a fact that only a few months ago my old laptop running High Sierra worked, though maybe that's not helpful. |
Yeah - from my understanding it has worked consistently on High Sierra. I'm not familiar with Cocoa at all - but for even more context this is where the crash was fixed OGRECave/ogre@8b6fe05#diff-67b8d7c5b917628f037cdff0a91fa136R607. Maybe it has something to do with subtle differences between the Cocoa APIs between the platforms? |
I just tried to launch RViz2 on one of the Sierra CI machines and indeed it's segfaulting as well. It looks like the tests are not fully covering the behavior of opening RViz completely. When opening RViz, it also complains about not finding @wjwwood What do you recommend how we proceed? |
It seems necessary then. So I guess move forward with this pr but manually test that it fixes the issue on the CI machine and on your personal machines. |
@emersonknapp any chance you could test this on either a High Sierra or Mojave machine? I tried to install this patch on a fresh high sierra VM but it still segfaults. But I am also not sure if I just missed something on this VM and would like to have a second opinion on it. |
Will try - I have found a potential machine to use. Hope to have results by the end of the week. |
…ading to 1.11 Signed-off-by: Emerson Knapp <eknapp@amazon.com>
I have built rviz2 on a High Sierra Macbook Pro with the latest master plus this patch and it runs successfully. @wjwwood Is this sufficient data to decide to merge? My force-push below is just a rebase to latest. |
This pr is in @Karsten1987's hands, I don't have time to follow up on it. |
Woohoo, glad we finally got this in! |
On Mojave I found startup problems when launching rviz2 (segfault) using the latest Crystal p4 |
@j-rivero Thanks for confirming on Mojave! Good to know this improved things across the board. |
Upgrade to 1.10.12 from 1.10.11, don't risk breaking any APIs by going to 1.11.x
From 1.10.12 release notes: "Fix crash when embedding OgreOSXCocoaWindow in external window"