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

Enable SDK for ROS2 on Scarthgap #1241

Open
wants to merge 19 commits into
base: scarthgap-next
Choose a base branch
from

Conversation

sgstreet
Copy link

@sgstreet sgstreet commented Oct 24, 2024

This PR enables OpenEmbedded SDK building and cross-compilation for Rolling, Jazzy and Humble. This PR is based on the work done in #988, #1203 and #1215. See these for more discussion

These changes allow demos, examples and examples_interfaces packages to be built using the resulting SDK. To build the SDK use:

$ bitbake ros2-image-sdktest -c do_populate_sdk

Install the resulting SDK, setup a ROS2 workspace containing demos, examples and example_interfaces and build the workspace using the follow script:

export ROS_VERSION=2
export ROS_PYTHON_VERSION=3
export ROS_DISTRO=${ROS_DISTRO:-rolling}
export PYTHONPATH=$OECORE_NATIVE_SYSROOT/usr/lib/python3.12/site-packages:$OECORE_TARGET_SYSROOT/opt/ros/rolling/lib/python3.12/site-packages
export AMENT_PREFIX_PATH=$OECORE_TARGET_SYSROOT/opt/ros/rolling
usr/share:$OECORE_TARGET_SYSROOT/opt/ros/rolling/share:$OECORE_TARGET_SYSROOT/opt/ros/rolling/lib/cmake"
export PYTHON_SOABI=cpython-312-aarch64-linux-gnu.so

colcon build \
--merge-install \
--install-base $PWD/install/arm64 \
--cmake-args \
" --no-warn-unused-cli" \
" -DCMAKE_TOOLCHAIN_FILE=$CMAKE_TOOLCHAIN_FILE" \
" -DCMAKE_STAGING_PREFIX=$PWD/install/arm64" \
" -DPYTHON_SOABI=$(basename -s so $PYTHON_SOABI)" \
" -DPython3_NumPy_INCLUDE_DIR=$OECORE_TARGET_SYSROOT/usr/lib/python3.12/site-packages/numpy/core/include" \
" -DBUILD_TESTING=OFF"

Install the $PWD/install/arm64 on the target device. Setup the device workspace:

$ . /opt/ros/rolling/setup.bash
$ . ./arm64/local_setup.bash

Run any of the examples to demonstrate the valid cross-compilation.

Co-developed-by: Romain Gimenez [email protected]
Co-developed-by: Jiaxing Shi [email protected]

@sgstreet
Copy link
Author

@jiaxshi @robwoolley can you review this? It has been tested on a Raspberry Pi5 and is functional.

@robwoolley
Copy link
Collaborator

Thanks, I was at ROSCon this week. I will take a look at it when I return home.

@sgstreet sgstreet marked this pull request as draft October 25, 2024 19:40
@sgstreet sgstreet marked this pull request as ready for review October 25, 2024 20:57
set(BOOST_ROOT "${PCL_ROOT}/3rdParty/Boost")
elseif(NOT BOOST_INCLUDEDIR)
- set(BOOST_INCLUDEDIR "@Boost_INCLUDE_DIR@")
+ set(BOOST_INCLUDEDIR "$ENV{OECORE_TARGET_SYSROOT}/usr/include")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @sgstreet
OECORE_TARGET_SYSROOT is defined and used in SDK environment. It should be null in yocto build time.

Copy link
Author

@sgstreet sgstreet Nov 6, 2024

Choose a reason for hiding this comment

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

Yes, I'm aware of this will be empty at yocto build time. My hope, untested at this point, is that non-sdk builds will set BOOST_INCLUDEDIR, EIGEN_ROOT and FLANN_ROOT correctly. Did you test this PR again one of your builds?

Can you suggest an different approach to prevent the leakage of the build directory into the SDK? I can remove this commit and update the sdk colcon build instructions if you want?

Copy link
Author

Choose a reason for hiding this comment

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

Turns out that the building images works even when OECORE_TARGET_SYSROOT is empty but present in the PCLConfig.cmake file. But in the interest of making the least change 9f47588 removes the reference to OECORE_TARGET_SYSROOT. @robwoolley Thanks for the insights!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use ${CMAKE_SYSROOT} here, it's a CMAKE variable and set in both yocto build env and SDK env.

Copy link
Author

Choose a reason for hiding this comment

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

9f47588 removes the references to OECORE_TARGET_SYSROOT and works both when building images/sdk and when using the sdk. Is this sufficient?

@sgstreet
Copy link
Author

sgstreet commented Nov 6, 2024

I rebased onto the current scarthgap-next

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

Hi all, great work on getting ROS2 to be part of the SDK!

It looks like it's almost working on my build, but I do get these two build errors:

rosidl-typesupport-interface.log.do_configure.txt
rcutils.log.do_configure.txt

For rosidl-typesupport-inteface, it looks like it cannot find ament_cmake:

| DEBUG: Executing shell function do_configure
| CMake Error at CMakeLists.txt:5 (find_package):
|   By not providing "Findament_cmake.cmake" in CMAKE_MODULE_PATH this project
|   has asked CMake to find a package configuration file provided by
|   "ament_cmake", but CMake did not find one.
|
|   Could not find a package configuration file provided by "ament_cmake" with
|   any of the following names:
|
|     ament_cmakeConfig.cmake
|     ament_cmake-config.cmake
|
|   Add the installation prefix of "ament_cmake" to CMAKE_PREFIX_PATH or set
|   "ament_cmake_DIR" to a directory containing one of the above files.  If
|   "ament_cmake" provides a separate development package or SDK, be sure it
|   has been installed.
|
|
| -- Configuring incomplete, errors occurred!
| WARNING: exit code 1 from a shell command.
ERROR: Task (virtual:nativesdk:/build/../work/src/ros/meta-ros2-humble/generated-recipes/rosidl/rosidl-typesupport-interface_3.1.5-2.bb:do_configure) failed with exit code '1'

And for rcutils, it's ament-cmake-python:


I wonder if this may be because I'm using a custom ros_prefix (/usr/opt/ros/humble).
| CMake Error at CMakeLists.txt:16 (find_package):
|   By not providing "Findament_cmake_python.cmake" in CMAKE_MODULE_PATH this
|   project has asked CMake to find a package configuration file provided by
|   "ament_cmake_python", but CMake did not find one.
|
|   Could not find a package configuration file provided by
|   "ament_cmake_python" with any of the following names:
|
|     ament_cmake_pythonConfig.cmake
|     ament_cmake_python-config.cmake
|
|   Add the installation prefix of "ament_cmake_python" to CMAKE_PREFIX_PATH or
|   set "ament_cmake_python_DIR" to a directory containing one of the above
|   files.  If "ament_cmake_python" provides a separate development package or
|   SDK, be sure it has been installed.
|
|
| -- Configuring incomplete, errors occurred!
| WARNING: exit code 1 from a shell command.
ERROR: Task (virtual:nativesdk:/build/../work/src/ros/meta-ros2-humble/generated-recipes/rcutils/rcutils_5.1.6-1.bb:do_configure) failed with exit code '1'

I wonder if this is because I'm using a non-standard ros_prefix (/usr/opt/ros/humble)...

@sgstreet
Copy link
Author

@jdiez17 Could you provide the values of PYTHONPATH, AMENT_PREFIX_PATH and the colcon build command you used? All three of these interact to find the ROS2 build system components.

@@ -25,11 +25,13 @@ export AMENT_PREFIX_PATH="${STAGING_DIR_HOST}${prefix};${STAGING_DIR_NATIVE}${pr

inherit cmake python3native

PYTHONPATH:prepend:class-nativesdk = "${STAGING_DIR_NATIVE}/opt/ros/${ROS_DISTRO}/lib/python${PYTHON_BASEVERSION}/site-packages:"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PYTHONPATH:prepend:class-nativesdk = "${STAGING_DIR_NATIVE}/opt/ros/${ROS_DISTRO}/lib/python${PYTHON_BASEVERSION}/site-packages:"
PYTHONPATH:prepend:class-nativesdk = "${STAGING_DIR_NATIVE}${ros_prefix}/lib/python${PYTHON_BASEVERSION}/site-packages:"

I suspect this hardcoded path caused my build error. Currently running a build with this change, seems to get further. Will report back if it builds and I can compile a workspace with the SDK.

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

I didn't get to building a workspace with this yet. That error was from running bitbake <image> -c populate_sdk. I suspect the hardcoded PYTHONPATH in ros_ament_cmake.bbclass was causing the build error(s).

@@ -38,3 +40,8 @@ EXTRA_OECMAKE:prepend:class-native = "\
-DCMAKE_PREFIX_PATH='${ros_prefix}' \
-DCMAKE_INSTALL_PREFIX:PATH='${ros_prefix}' \
"

EXTRA_OECMAKE:prepend:class-nativesdk = "\
-DCMAKE_PREFIX_PATH='${STAGING_DIR_NATIVE}/opt/ros/${ROS_DISTRO};${STAGING_DIR_NATIVE}${ros_prefix};${STAGING_DIR_NATIVE}${prefix}' \
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't ${STAGING_DIR_NATIVE}/opt/ros/${ROS_DISTRO} and ${STAGING_DIR_NATIVE}${ros_prefix} the same? (except, of course, in situations where ros_prefix is customized - like in my build).

Suggested change
-DCMAKE_PREFIX_PATH='${STAGING_DIR_NATIVE}/opt/ros/${ROS_DISTRO};${STAGING_DIR_NATIVE}${ros_prefix};${STAGING_DIR_NATIVE}${prefix}' \
-DCMAKE_PREFIX_PATH='${STAGING_DIR_NATIVE}${ros_prefix};${STAGING_DIR_NATIVE}${prefix}' \

Copy link
Author

Choose a reason for hiding this comment

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

Sure seem like a problem. I will try on my desktop

Copy link
Author

@sgstreet sgstreet Nov 11, 2024

Choose a reason for hiding this comment

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

Does b99fe2b fix it?

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

Unfortunately, using ${STAGING_DIR_NATIVE}${ros_prefix} for CMAKE_PREFIX_PATH and PYTHONPATH does not work. My build only succeeds when I put the hardcoded path, like ${STAGING_DIR_NATIVE}/usr/opt/ros/humble in both paths.

This may be because ${ros_prefix} includes ${base_prefix}. To be continued...

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

Indeed ${ros_prefix} seems incorrect for nativesdk packages due to base_prefix.

$ bitbake-getvar -r nativesdk-rosidl-typesupport-interface ros_prefix
#
# $ros_prefix [2 operations]
#   set /build/../work/src/raccoon/meta-raccoon/conf/distro/raccoon.conf:26
#     "${base_prefix}/usr/opt/ros/${ROS_DISTRO}"
#   set? /build/../work/src/ros/meta-ros-common/classes/ros_opt_prefix.bbclass:7
#     "${base_prefix}/opt/ros/${ROS_DISTRO}"
# pre-expansion value:
#   "${base_prefix}/usr/opt/ros/${ROS_DISTRO}"
ros_prefix="/usr/local/oe-sdk-hardcoded-buildpath/sysroots/x86_64-pokysdk-linux/usr/opt/ros/humble"

For reference:

$ bitbake-getvar -r ros-image-core ros_prefix
#
# $ros_prefix [2 operations]
#   set /build/../work/src/raccoon/meta-raccoon/conf/distro/raccoon.conf:26
#     "${base_prefix}/usr/opt/ros/${ROS_DISTRO}"
#   set? /build/../work/src/ros/meta-ros-common/classes/ros_opt_prefix.bbclass:7
#     "${base_prefix}/opt/ros/${ROS_DISTRO}"
# pre-expansion value:
#   "${base_prefix}/usr/opt/ros/${ROS_DISTRO}"
ros_prefix="/usr/opt/ros/humble"

@sgstreet
Copy link
Author

@jdiez17 Seems a like a bigger problem with the ros_prefix preventing alternative install locations. Maybe we should create a new issue to explore how to fix this?

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

@sgstreet Agreed. As discussed, this should not block merging this PR. The only thing I can say is that the hardcoded paths (as they are in the PR currently) should stay there for now.

@sgstreet
Copy link
Author

sgstreet commented Nov 11, 2024

@jdiez17 Can you try something like this?

diff --git a/meta-ros-common/classes/ros_opt_prefix.bbclass b/meta-ros-common/classes/ros_opt_prefix.bbclass
index 58c05e58d..238949aca 100644
--- a/meta-ros-common/classes/ros_opt_prefix.bbclass
+++ b/meta-ros-common/classes/ros_opt_prefix.bbclass
@@ -4,7 +4,8 @@
 # Copyright (c) Qualcomm Innovation Center, Inc. All rights reserved
 #
 
-ros_prefix ?= "${base_prefix}/opt/ros/${ROS_DISTRO}"
+ros_install_prefix ?= "/opt/ros/${ROS_DISTRO}"
+ros_prefix = "${base_prefix}${ros_install_prefix}"
 
 ros_bindir = "${ros_prefix}/bin"
 ros_sbindir = "${ros_prefix}/sbin"
diff --git a/meta-ros2/classes/ros_ament_cmake.bbclass b/meta-ros2/classes/ros_ament_cmake.bbclass
index f964181c7..ccc7aa1c6 100644
--- a/meta-ros2/classes/ros_ament_cmake.bbclass
+++ b/meta-ros2/classes/ros_ament_cmake.bbclass
@@ -42,6 +42,6 @@ EXTRA_OECMAKE:prepend:class-native = "\
 "
 
 EXTRA_OECMAKE:prepend:class-nativesdk = "\
-    -DCMAKE_PREFIX_PATH='${STAGING_DIR_NATIVE}/opt/ros/${ROS_DISTRO};${STAGING_DIR_NATIVE}${ros_prefix};${STAGING_DIR_NATIVE}${prefix}' \
+    -DCMAKE_PREFIX_PATH='${STAGING_DIR_NATIVE}${ros_install_prefix};${STAGING_DIR_NATIVE}${ros_prefix};${STAGING_DIR_NATIVE}${prefix}' \
     -DCMAKE_INSTALL_PREFIX:PATH='${ros_prefix}' \
 "

And this set ros_install_prefix in your build environment.

I'm pretty sure the SDK ROS2 build tools are assuming some dependencies are present in their final location /opt/ros/${ROS_DISTRO} or in your case /usr/opt/ros/${ROS_DISTRO}

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

@sgstreet I can confirm that it works with your proposed patch (committed in my fork here).

This builds the SDK successfully, but seems numpy is not included:

CMake Error at /opt/raccoon/0.4/sysroots/x86_64-pokysdk-linux/usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found
  version "3.12.4")
Call Stack (most recent call first):
  /opt/raccoon/0.4/sysroots/x86_64-pokysdk-linux/usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  /opt/raccoon/0.4/sysroots/x86_64-pokysdk-linux/usr/share/cmake-3.28/Modules/FindPython/Support.cmake:3867 (find_package_handle_standard_args)
  /opt/raccoon/0.4/sysroots/x86_64-pokysdk-linux/usr/share/cmake-3.28/Modules/FindPython3.cmake:545 (include)
  /opt/raccoon/0.4/sysroots/cortexa53-crypto-poky-linux/usr/opt/ros/humble/share/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake:24 (find_package)
  /opt/raccoon/0.4/sysroots/cortexa53-crypto-poky-linux/usr/opt/ros/humble/share/ament_cmake_core/cmake/core/ament_execute_extensions.cmake:48 (include)
  /opt/raccoon/0.4/sysroots/cortexa53-crypto-poky-linux/usr/opt/ros/humble/share/rosidl_cmake/cmake/rosidl_generate_interfaces.cmake:286 (ament_execute_extensions)
  CMakeLists.txt:17 (rosidl_generate_interfaces)

Not sure why numpy is needed for message generation, but will continue investigating.

@sgstreet
Copy link
Author

sgstreet commented Nov 11, 2024

My bad, I will update the usage instructions, try adding:

-DPython3_NumPy_INCLUDE_DIR=$OECORE_TARGET_SYSROOT/usr/lib/python3.12/site-packages/numpy/core/include

To your colcon build command -cmake-args. I was unable to track down this problem, it seem like it is an issue with the numpy cmake module, but I could no track it down. You were trying to use the SDK to build a workspace correct?

@sgstreet
Copy link
Author

@jdiez17 See 4f60fc1 for the fix to relocating the ROS install. I changed the name ros_base_prefix to kind of match the existing naming convention.

@jdiez17
Copy link
Contributor

jdiez17 commented Nov 11, 2024

@sgstreet Thanks, telling colcon/cmake about the location of the numpy include dir made the generated SDK be able to succesfully compile my interface (msg/action) packages. Winner, winner! 🎉

For reference, this is my cross compilation script.

Your commit 4f60fc1 LGTM.

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

{common} Add native sdk class extentions

This looks like the correct, simple solution for now. In the longer term, we may want to replace this with a variable in ros-distro.inc that automatically extends the recipes to include nativesdk if the recipe name is in the list.

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

{rolling} Patch fastrtps to use find TinyXML2

I'm fine with the changes, but it is unclear what problem this solves.

Can you provide more detail on why this was necessary to enable the SDK?

Newer versions of Bitbake also require the Upstream-Status field to be included in the header of the patch. Is this a change that would be applicable upstream or is it oe-specific? (https://docs.yoctoproject.org/contributor-guide/recipe-style-guide.html#patch-upstream-status)

Thanks!

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

{common}{ros2} Ensure EXTRA_OECMAKE overrides correctly handle multiple cmake -D options

The change looks good. Thanks for tracking this down!

I don't think I understand the comment about making sure it handles multiple cmake -D options, could you elaborate on what the problem was and how this fixes that?

I also need to double-check whether STAGING_DIR_HOST is used and not STAGING_DIR_TARGET.

Do we need to set CMAKE_MODULE_PATH for native and nativesdk? Without it I worry we may be linking against Python modules provided by the host instead of the -native Python modules.

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

meta-ros-common/recipes-core/images/ros2-image-sdktest.bb:

I think having this alongside ros-image-sdktest.bb makes sense. We may want to rename the other image recipe ros1-image-sdktest.bb to avoid confusion for ROS 2 users.

I see that it includes some heavy hitters like OpenCV, Python, NumPy, Boost, etc. It looks like a good regression test for making sure that the SDK is working. Can we also document the steps needed to exercise the SDK?

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

meta-ros-common/recipes-devtools/colcon/python3-colcon-core/0003-Force-shebang-to-usr-bin-env-python3.patch:

Nice find, looks good.

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

{humble} Use find package for yaml to prevent sdk build failures

I wonder if we should be using yaml_vendor instead?

Copy link
Collaborator

@robwoolley robwoolley left a comment

Choose a reason for hiding this comment

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

{infrastructure} Stop chatter about newline in package descriptions

Would it be possible to add an Upstream-Status line to the patch? https://docs.yoctoproject.org/contributor-guide/recipe-style-guide.html#patch-upstream-status

What is the impact of the warning? Does it flood the log or cause a build failure?

How does this change match upstream? Is it something that should be submitted upstream?

@sgstreet
Copy link
Author

{common} Add native sdk class extentions

This looks like the correct, simple solution for now. In the longer term, we may want to replace this with a variable in ros-distro.inc that automatically extends the recipes to include nativesdk if the recipe name is in the list.

I created issue #1255 to track this.

@sgstreet
Copy link
Author

Can you provide more detail on why this was necessary to enable the SDK?

This error is generated without this patch:

--- stderr: demo_nodes_cpp_native
CMake Warning at CMakeLists.txt:29 (add_library):
  Cannot generate a safe runtime search path for target talker_native because
  files in some directories may conflict with libraries in implicit
  directories:

    runtime library [libssl.so.3] in /home/stephen/Workspace/ros2-dev/sdk/sysroots/cortexa76-oe-linux/usr/lib may be hidden by files in:
      /home/stephen/Workspace/meta-ros-dev/scarthgap-next/build-rolling/tmp-glibc/work/cortexa76-oe-linux/fastrtps/2.14.3-1/recipe-sysroot/usr/lib
    runtime library [libcrypto.so.3] in /home/stephen/Workspace/ros2-dev/sdk/sysroots/cortexa76-oe-linux/usr/lib may be hidden by files in:
      /home/stephen/Workspace/meta-ros-dev/scarthgap-next/build-rolling/tmp-glibc/work/cortexa76-oe-linux/fastrtps/2.14.3-1/recipe-sysroot/usr/lib

  Some of these libraries may not be found correctly.

Newer versions of Bitbake also require the Upstream-Status field to be included in the header of the patch. Is this a change that would be applicable upstream or is it oe-specific? (https://docs.yoctoproject.org/contributor-guide/recipe-style-guide.html#patch-upstream-status)

I'm not sure how to assess the upstream fix. I copied the patch from f0b47b0 in PR #988.

@sgstreet
Copy link
Author

I don't think I understand the comment about making sure it handles multiple cmake -D options, could you elaborate on what the problem was and how this fixes that?

The original code at

EXTRA_OECMAKE:prepend = "\
is broken. The EXTRA_OECMAKE:prepend here adds items to the EXTRA_OECMAKE. The final value of EXTRA_OECMAKE will contain two -DCMAKE_PREFIX_PATH and two -DCMAKE_INSTALL_PREFIX:PATH values. When cmake gets the --cmake-args containing these duplicate defines, cmake selects only the last value to use leading to incorrect values. The patch correctly assembles the needed EXTRA_OECMAKE value.

Do we need to set CMAKE_MODULE_PATH for native and nativesdk? Without it I worry we may be linking against Python modules provided by the host instead of the -native Python modules.

This was in the orignal commit faa4e9b so I left it alone as required. I think, but do not know that this required so that the native versions of the rosidl can be built.

@sgstreet
Copy link
Author

I think having this alongside ros-image-sdktest.bb makes sense. We may want to rename the other image recipe ros1-image-sdktest.bb to avoid confusion for ROS 2 users.

Should I add to this PR?

I see that it includes some heavy hitters like OpenCV, Python, NumPy, Boost, etc. It looks like a good regression test for making sure that the SDK is working. Can we also document the steps needed to exercise the SDK?

These are only required to use the SDK to build the perception variant. Maybe I should remove them for this PR. Yep, we need SDK documentation, wiki? For now the PR description would allow someone to build demos, examples and example_interfaces. Is this good enough?

@sgstreet
Copy link
Author

sgstreet commented Nov 12, 2024

I wonder if we should be using yaml_vendor instead?

What I found is using

find_package(PkgConfig REQUIRED)
pkg_check_modules(yaml REQUIRED yaml-0.1)

caused the SDK to include build directory paths in the cmake yaml modules, regardless of yaml_vendor usage while using find_package did not. I did not root cause the problem. Maybe I got this wrong?

@sgstreet
Copy link
Author

sgstreet commented Nov 12, 2024

Would it be possible to add an Upstream-Status line to the patch? https://docs.yoctoproject.org/contributor-guide/recipe-style-guide.html#patch-upstream-status

I think the upstream package is https://github.com/ros-infrastructure/catkin_pkg/blob/master/src/catkin_pkg/package.py which does contain the offending newline check. The meta-ros recipe at https://github.com/ros/meta-ros/blob/master/meta-ros-common/recipes-infrastructure/python/python-catkin-pkg.inc gets it's source from some pypi package repo that I'm could not untangle. I'm completely unclear on how these upstream locations are interacting. Maybe we should be using the ros-infrastructure version?

What is the impact of the warning? Does it flood the log or cause a build failure?

Lot's of annoying messages that add no value.

How does this change match upstream? Is it something that should be submitted upstream?

As far as I can tell (see above) the up stream does not have this issue.

Signed-off-by: Stephen Street <[email protected]>
Signed-off-by: Stephen Street <[email protected]>
Signed-off-by: Stephen Street <[email protected]>
Signed-off-by: Stephen Street <[email protected]>
@sgstreet
Copy link
Author

I rebased onto the tip of scarthgap-next.

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

Successfully merging this pull request may close these issues.

4 participants