-
Notifications
You must be signed in to change notification settings - Fork 137
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
REP-2001: Propose c++-only minimal ros2 variant #231
REP-2001: Propose c++-only minimal ros2 variant #231
Conversation
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
It would be nice, but probably not likely given the packaging situation on Windows. At least on Ubuntu, they should generally pass through to the underlying system packages.
My opinion is that it would be better to call out these dependencies specifically, especially if someone is going to try to port the |
rep-2001.rst
Outdated
The intention is that this variant can target cross-compiling for custom OS sysroots that may not have build tools or Python available that run on the target platform. | ||
It may not contain any GUI or Python dependencies. | ||
It may not have any dependencies outside a source workspace of its members with an exception of ubiquitous Linux libraries. | ||
Note that this variant explicitly does not include an implementation of either RMW or rcl_logging, users will need to choose their own that handle the dependencies that come with them. |
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 we need to at least have one RMW in order to be able to build this variant - it can be just the default (FastRTPS) - or if that requires undesired dependencies any other RMW impl.
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.
The attempt to avoid FastRTPS was because of its dependency on libasio-dev. I'm more inclined toward Cyclone because it has no external rosdeps, but I know it's not the default RMW - what do you think?
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 agree that I think that it should stand alone and be buildable in the default configuration without modifications. Otherwise the out of the box experience will be quite poor. And we won't be able to test it easily as all tests would need to have logic to extend it with an implementation.
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 included rcl_spdlog
and rmw_cyclonedds
in the latest revision
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 there needs to be a paragraph describing the rational why CycloneDDS was chosen over the default RMW FastRTPS.
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.
FWIW, and I'm not advocating for FastRTPS, I've been able to cross compile to an embedded ARM target using FastRTPS via --cmake-flags -DTHIRDPARTY=ON
which forces FastRTPS to source it's own copy of asio (among other things).
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.
That's good information - perhaps we can target this at "one of" like the current core does
What if someone would like an even more minimal variant which e.g. only uses C (no C++)? In that case I would argue "minimal" might not be a good name since it very much depends on your subjective perspective. |
I would propose |
Of those two I prefer |
"light" sounds good to me, but I'm wondering if we should go for more verbose and name it something like |
@emersonknapp wrote:
|
+1. This sounds more clear to me what the variant goes to include. |
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.
For the naming I would suggest that we use something that actually mentions cross compiling as that's what this is semantically used for and selected for. Other proxies for minimal or small or light are actually side effects of the main purpose.
rep-2001.rst
Outdated
The intention is that this variant can target cross-compiling for custom OS sysroots that may not have build tools or Python available that run on the target platform. | ||
It may not contain any GUI or Python dependencies. | ||
It may not have any dependencies outside a source workspace of its members with an exception of ubiquitous Linux libraries. | ||
Note that this variant explicitly does not include an implementation of either RMW or rcl_logging, users will need to choose their own that handle the dependencies that come with them. |
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 agree that I think that it should stand alone and be buildable in the default configuration without modifications. Otherwise the out of the box experience will be quite poor. And we won't be able to test it easily as all tests would need to have logic to extend it with an implementation.
That is only one way this variant can be used. So I think coupling the name with that semantic is wrong since it can equally be useful in other context. |
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.
Great initiative!
RE: naming
cpp_core
sounds nice, minimal_cpp
, ros_bare_cpp
could work too.
One concern with the use of core
is the implied coupling with ros_core
and managing expectations accordingly: does cpp-core
provide all the cpp features of ros_core
(e.g. pluginlib)?
And in the future would py_core
provide all the ROS 2 python commandline tools included in ros_core
? or just a similar feature-set as cpp-core
but in Python?
Decoupling the name from ros_core
would allow for both to evolve independently
|
||
The `ros_minimal` variant is a highly restricted set of ROS2 functionality. | ||
It targets resource- and tool-constrained environments where package managers may be limited or unavailable. | ||
The intention is that this variant can target cross-compiling for custom OS sysroots that may not have build tools or Python available that run on the target platform. |
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.
If it is an explicit goal for this variant to be able to be used on a target system that doesn't have Python available, a different building + environment setup process should be highlighted as the currently recommended one (colcon build + source a setup file) does not fulfill this requirement (colcon/colcon-core#73)
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.
The goal isn't to have this be built on a target system that doesn't have Python available. The intention is to be built for a target system that doesn't have Python available. I want to make a strong distinction between the build-time environment and the run-time environment. While these are often the same in classic ROS workflows, It shouldn't be the assumption. Do you think I should add language around that here? I fully expect this to be built with colcon and linted with cpplint, for example.
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.
The build also needs Python to run the code generators for messages / services / actions.
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.
But that's not needed at runtime, right?
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.
The intention is to be built for a target system that doesn't have Python available.
That's what I meant as well but it was not clear.
Currently AFAICT it is not possible to source a colcon setup file (top-level setup.bash file generated by colcon) without Python: colcon/colcon-core#73.
So for such a variant to be usable on a system that doesn't have Python available, we need a different workflow that allow to setup the environment on the target even if the target doesn't have Python. It can be modifying colcon to generate Python-agnostic setup files, or a different mechanism.
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.
Oh - I see what you mean. The way I have used it is to
- use
--merged-install
export LD_LIBRARY_PATH=install/lib
- invoke binaries by full path
Now that I'm thinking about that, yes, it seems we'll need a non-python way to set up the proper environment variables. Would it be possible to get away with creating a fairly simple pure-shell utiility? Something that would
- Set
LD_LIBRARY_PATH
- Add all package
lib
folders toPATH
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.
That simple approach will only work for a subset. A complete shell-based solution must replicate the same logic as it is implemented in Python. That includes collecting all packages in the prefix, getting their dependencies (available in the filesystem, doesn't need to parse the manifests), order them topologically, and then source the setup files of each package.
If the shell-based solution doesn't implement the logic around .dsv
files that would also imply a much longer time to setup the environment.
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 these details just necessary to handle the different types of installs, or is it deeper than that? Perhaps we could impose limitations on the usage of this variant so as to avoid more complexity in the environment setup.
source the setup files of each package.
Do these require python, or just the top level script does?
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 these details just necessary to handle the different types of installs, or is it deeper than that? Perhaps we could impose limitations on the usage of this variant so as to avoid more complexity in the environment setup.
That has nothing do to with the different types of installs.
The importance is the order. One package might rely on an environment change caused by another package it depends on.
source the setup files of each package.
Do these require python, or just the top level script does?
No, they don't.
rep-2001.rst
Outdated
rcl_logging, rclcpp, rcpputils, rcutils, rmw, | ||
rmw_implementation, ros_tracing, rosidl, rosidl_defaults, | ||
rosidl_typesupport, spdlog_vendor, test_interface_files, | ||
tinydir_vendor, unique_identifier_msgs] |
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.
historically despite the name "packages", this represent a list of repositories that one can clone and build as is (without the REP needing to explicitly list all packages in all the listed repos).
However some of these repositories contain python packages (e.g. ament_index
repository includes ament_index_cpp
and ament_index_py
or ament_cmake
includes many python packages). Should they be split out to keep this list "without any Python dependency"? or switch to listing explicitly packages and a recommended way to clone only those packages?
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.
That's a good point - I am definitely hoping to build a subset of packages within the involved repositories. I'll add a list of repos and the packages from them that will be built. Since the variants are described by a meta-package, though, if you build --packages-up-to VARIANT
that's probably more correct than checking things out and building everything.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Thanks everyone for the feedback! I've pushed a new revision taking into account some of the comments. A few notes on this one:
We will want to test this variant, of course. But we probably don't want to require the test dependency tree to meet the criteria for being part of this variant (launch-based testing is useful) There could be a split point where it needs to be possible to have Python in a superset test environment on top of the target minimal environment...? |
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Additionally I have reverted the changes to |
@dirk-thomas @mikaelarguedas I'd like to determine next steps - what I see is
|
I'll defer to @dirk-thomas as I haven't been following the colcon development for a long time. IIRC before the use of Python, a simplistic approach was being used. colcon knows the topological order when building, so it just hard-coded a list of prefix files to source in order at build time. This would be less efficient and less modular than the current approach (as pointed out above by Dirk) but could be an intermediate approach if porting all of the python utilities to native shells proves to be too challenging. |
|
||
* desktop (recommended) | ||
* ros_base | ||
* ros_core | ||
* ros_cpp_core (advanced) |
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.
Somewhat pedantic, but with ros_core
, I think ros_core_cpp
would follow the naming convention we generally use (see all packages below).
Also the "advanced" tag is a little unclear (what's advanced about it? Isn't it more basic than ros_core
? etc). I might change it to barebones
if you'd like to include some qualifier, which I'm not sure is necessary. The name itself is very telling.
Yes, that is correct. The same modifications will also be needed in ament_package which contains similar logic used when e.g. in ros_workspace when building Debian packages.
That approach is not being used since it prevents installing packages into the same prefix incrementally (the package of a single build invocation might not be the only ones in the install prefix).
It will indeed be challenging to implement the logic in e.g. plain |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros2-tooling-wg-next-meeting/12545/39 |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-10-15/16849/1 |
I don't anticipate this project going anywhere - will close it to clear up stale issues. |
Propose a minimal C++-only ROS2 variant that is smaller than
ros_core
and excludes Python and the CLI.See https://discourse.ros.org/t/c-c-minimal-source-tree-only-ros2-variant/11760/4 for reference.
This proposal has unresolved issues, but I would like to start the conversation and get feedback.
Open questions:
*cmake*
packages as wellzlib
andcurl
, which we depend on but basically everybody can build so it's not a big deal, as opposed to something like OpenCV which is nontrivialvendor_
packages at some point? If so, that makes this more difficult, because those vendor packages support this use case today.Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com