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

support rviz2 for humble on macOS #445

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

stevalkr
Copy link
Contributor

@stevalkr stevalkr commented Aug 3, 2024

fixes rviz2 issue in #419 (comment), depends on PR #442

@stevalkr
Copy link
Contributor Author

stevalkr commented Aug 3, 2024

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?

Copy link
Contributor

@wentasah wentasah left a 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

distros/humble/overrides.nix Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/default.nix Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/default.nix Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/rviz-ogre-vendor.patch Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/rviz-ogre-vendor.patch Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/rviz-ogre-vendor.patch Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/rviz-ogre-vendor.patch Outdated Show resolved Hide resolved
distros/humble/rviz-ogre-vendor/rviz-ogre-vendor.patch Outdated Show resolved Hide resolved
distros/humble/rviz-rendering/default.nix Outdated Show resolved Hide resolved
examples/ros2-basic.nix Outdated Show resolved Hide resolved
stevalkr added a commit to stevalkr/rviz-release that referenced this pull request Aug 4, 2024
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
```
@stevalkr
Copy link
Contributor Author

stevalkr commented Aug 4, 2024

Thank you @wentasah! patch for darwin-aarch64 is pushed to ros2-gbp/rviz-release@777d3f4, version bump in done by substituteInPlace.

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.

Copy link
Contributor

@wentasah wentasah left a 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.

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";
Copy link
Contributor

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.

@stevalkr
Copy link
Contributor Author

Hi there! Any updates during this week?

In summary,

  • the building failure on Linux. Is there any progress?
  • are pugixml and glew needed on other platforms?
  • should I change the URL to my repo or try to find a upstream patch that can be used directly?

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.

@hegde-atri
Copy link

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.

@hegde-atri
Copy link

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 😅

@hegde-atri
Copy link

@wentasah can you give me any pointers for the issue on linux? Currently its failing while building rviz-ogre-vendor.

Hunk #2 succeeded at 112 with fuzz 2 (offset -1 lines).
(Stripping trailing CRs from patch; use --binary to disable.)
patching file PlugIns/OctreeZone/CMakeLists.txt
(Stripping trailing CRs from patch; use --binary to disable.)
patching file OgreMain/include/OgreTimer.h
patching file OgreMain/src/OgrePlatformInformation.cpp
Reversed (or previously applied) patch detected!  Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file OgreMain/src/OgrePlatformInformation.cpp.rej
make[2]: *** [CMakeFiles/ogre-v1.12.13.dir/build.make:115: ogre-v1.12.13-prefix/src/ogre-v1.12.13-stamp/ogre-v1.12.13-patch] Error 1
make[1]: *** [CMakeFiles/Makefile2:137: CMakeFiles/ogre-v1.12.13.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

@stevalkr
Copy link
Contributor Author

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!

Copy link
Owner

@lopsided98 lopsided98 left a 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.

@stevalkr
Copy link
Contributor Author

stevalkr commented Nov 5, 2024

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 rviz_ogre_vendor is a subfolder of ros2/rviz). Alternatively, we could also make iron compile and function on aarch64-darwin. The patches have already been merged into the release repository. Which solution do you prefer (or none of them)?

@lopsided98
Copy link
Owner

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.

@lopsided98
Copy link
Owner

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 ros2-overlay.nix.

@stevalkr
Copy link
Contributor Author

stevalkr commented Dec 2, 2024

@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!

@lopsided98
Copy link
Owner

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...

@stevalkr
Copy link
Contributor Author

stevalkr commented Dec 3, 2024

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.

@lopsided98
Copy link
Owner

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?

@lopsided98
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants