-
Notifications
You must be signed in to change notification settings - Fork 83
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
support rviz2 for humble on macOS #445
base: develop
Are you sure you want to change the base?
Conversation
Only humble modified and tested by visualizing pointcloud2 played from a ros2bag. Needs test on linux and other macOS versions, also on other ros2 distros. @wentasah @hegde-atri could you help please? |
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.
Some changes will be needed. See the comments below. Also, when I build rviz2
on Linux, I'm getting the following error:
-- Configuring done (8.7s)
CMake Error at Components/Overlay/CMakeLists.txt:42 (add_library):
Cannot find source file:
/build/rviz-release-release-humble-rviz_ogre_vendor-11.2.12-1/build/ogre-v1.12.13-prefix/src/ogre-v1.12.13-build/imgui-1.79/imgui.cpp
this patch is based on release/humble/rviz_ogre_vendor/11.2.12-1 to support rviz2 on darwin-aarch64 for ros2-humble, see details at lopsided98/nix-ros-overlay#445 allow building arm64 ``` @@ -125,7 +125,11 @@ macro(build_ogre) set(OGRE_CXX_FLAGS "${OGRE_CXX_FLAGS} /w /EHsc") elseif(APPLE) set(OGRE_CXX_FLAGS "${OGRE_CXX_FLAGS} -std=c++14 -stdlib=libc++ -w") - list(APPEND extra_cmake_args "-DCMAKE_OSX_ARCHITECTURES='x86_64'") + if (${CMAKE_SYSTEM_PROCESSOR} STREQUAL "arm64") + list(APPEND extra_cmake_args "-DCMAKE_OSX_ARCHITECTURES='arm64'") + else() + list(APPEND extra_cmake_args "-DCMAKE_OSX_ARCHITECTURES='x86_64'") + endif() else() # Linux set(OGRE_C_FLAGS "${OGRE_C_FLAGS} -w") # include Clang -Wno-everything to disable warnings in that build. GCC doesn't mind it ``` solve `error: Objective-C was disabled in PCH file but is currently enabled` for Apple Silicon. `fix-arm64` is merged by upstream since v1.12.9, version bump is in nix-ros-overlay `override.nix`. ``` @@ -185,10 +188,10 @@ macro(build_ogre) -DOGRE_BUILD_COMPONENT_JAVA:BOOL=FALSE -DOGRE_BUILD_COMPONENT_CSHARP:BOOL=FALSE -DOGRE_BUILD_COMPONENT_BITES:BOOL=FALSE + -DOGRE_ENABLE_PRECOMPILED_HEADERS=0 ${extra_cmake_args} -Wno-dev PATCH_COMMAND ${Patch_EXECUTABLE} -p1 -N < ${CMAKE_CURRENT_SOURCE_DIR}/pragma-patch.diff && - ${Patch_EXECUTABLE} -p1 -N < ${CMAKE_CURRENT_SOURCE_DIR}/fix-arm64.diff && ${Patch_EXECUTABLE} -p1 -N < ${CMAKE_CURRENT_SOURCE_DIR}/relocatable.patch COMMAND ```
Thank you @wentasah! patch for However I don't have Linux machine, can you provide more outputs? I think I've encountered this before in other repos, will do my best to help. |
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.
Just a quick note for now. I'll investigate the build failure on Linux later when I have more time. Maybe, it will go away if you use the upstream patches.
distros/humble/overrides.nix
Outdated
patches = patches ++ lib.optionals self.stdenv.isDarwin [ | ||
# Fix build with darwin-aarch64 | ||
(self.fetchpatch { | ||
url = "https://github.com/ros2-gbp/rviz-release/commit/777d3f45f7d6197e3ee3b7e3b9e95f0138ca6266.patch"; |
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.
This is not what I meant. AFAICS the commit you're referring to is only in your repository. If it's needed, you should at least use the URL of your repo. However, it's not needed in this case, because the same fixes are already merged in the upstream repo (e.g. here and here). The preferred way is to use those patches.
Hi there! Any updates during this week? In summary,
I’ll definitely set up a Linux container for future contributions when I have some free time. But for now, I’m afraid I can’t help with this. |
Sorry, I got swamped with other work and forgot about this. I will have a go at tackling the building failure on Linux tomorrow and let you know how I get along. |
Currently getting a failed patch error while building rviz-ogre-vendor on linux. I will have more time to work on this tomorrow. As for your other points (2 & 3), I have no clue 😅 |
@wentasah can you give me any pointers for the issue on linux? Currently its failing while building
|
Hello, PR #442 is merged, so I rebased this branch and fixed #445 (comment) @hegde-atri. However I don't have the issue #445 (review) @wentasah. Tested on macOS 15.1 and ubuntu 24.04 docker container. Could you please take a look at this PR and let me know if you spot any other issues? If everything looks good enough, I’ll go ahead and squash it and then wait for a merge. Thanks a bunch! |
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.
Sorry for neglecting this, but unfortunately I'm not going to merge this for now. My policy is that if upstream does not support macOS, then I don't want to maintain a bunch of hacks to make it work. I have no ability to test on macOS, so complex patches hinder my ability to maintain the overlay for Linux.
It does mainly look like the problems are with aarch64-darwin, so I would be willing to merge a more limited PR that only targets x86_64-darwin.
Hi @lopsided98, @wentasah found a commit in the up-upstream repository here that was a fix for humble but was merged into the rolling release at that time. Do you think it’s okay to use this commit? If so, we can change the file name in the patch to make it work (since |
If there are upstream patches in recent distros that fix the problem, backporting them to humble is fine. I just want to things to start trending toward getting fixed upstream, so I don't have to maintain downstream patches forever. |
I would also appreciate it if you could try to get newer distros working as well. In particular, the macOS dependency overrides probably be moved to |
original patch: ros2/rviz@0ef2b56
@lopsided98 thanks for reaching out, I’ve got a fix but it’s a bit of a hack. Please take a look and let me know if it works for you! |
Sorry, I didn't mean to make you add a bunch of hacks just to avoid making a custom version of the patch. I also didn't look at it closely enough before to realize how similar your original patch was to the version accepted upstream. On the other hand, is it actually okay to just replace Ogre 1.12.1 with 1.12.10? The upstream commit that upgraded Ogre (ros2/rviz@4e976dc) looks to have some non-trivial changes to make it compatible. It might build, but does it run properly? I assume you are stuck on humble. It would be a lot easier if we could just forget about humble and make newer distros work instead... |
I haven’t had a chance to fully test it, but it seems to work on my Mac (MacOS 15.1.1, M3 Max). I’m not sure if it breaks any functionalities I don’t use or Linux, though. It looks like newer distributions have higher versions of all the dependencies, so the patches won’t apply to all of them right away. Since humble is still the latest LTS, I was planning to work on iron after the rviz2 and rqt pull requests are merged. Nevertheless, I can begin working on iron later. I’ll start a new pull request then. |
It looks like both Iron and Jazzy have been upgraded to Ogre 1.12.10 and have ros2/rviz@0ef2b56 applied. What is there to patch other than adding the macOS framework dependencies? |
Also, LTS distros are way harder to support in this overlay because they lack all the fixes required to be compatible with newer dependency versions. nixpkgs keeps moving on, but ROS distros are largely frozen in time with their corresponding Ubuntu release. |
fixes rviz2 issue in #419 (comment), depends on PR #442