Closed juanrh closed 4 years ago
@filiperinaldi @lmayencourt @sgermanserrano thought you might be interested in this as well.
Reading the current proposal I see a few issues with the approach:
The initial design goals of colcon
explicitly mentions that dealing with external dependencies (anything outside of the workspace) is explicitly out of scope for the tool. The idea behind this is basically that a tool should have one purpose, focus on that topic and do it well. To me it sounds like a different tool (like rosdep
) should do that job.
While I understand the desire to come up with a single-line-invocation the new verb seems to do two completely separate things: building a sysroot image and building the workspace. I think all the wording around when the sysroot is being build, when it needs to be rebuild and options like --force-sysroot-build
are indicators that it might be better to separate these two parts.
Also I am not sure that colcon
needs a way to use Docker. Instead of coupling colcon
with knowledge about Docker why no just invoke colcon
within Docker - colcon
doesn't need to know about this imo.
Reading the current proposal I see a few issues with the approach:
- The initial design goals of
colcon
explicitly mentions that dealing with external dependencies (anything outside of the workspace) is explicitly out of scope for the tool. The idea behind this is basically that a tool should have one purpose, focus on that topic and do it well. To me it sounds like a different tool (likerosdep
) should do that job.
That seems fair to me. What about having something like rosdep_docker
building Docker sysroot images for us?
We can start by putting this script alongside the colcon plugin and fork it into its own repo if that stuff keeps growing. There is some benefit IMHO into piggy-backing the repo to reduce the maintenance burden (e.g. release management and such).
Otherwise, woud you be OK with us teaching rosdep
how to build Docker images? That'd be another way to do it. We don't need to pull a dependency to Docker for that so it would be a very localized change (probably built on top of rosdep
-s
flag?)
- While I understand the desire to come up with a single-line-invocation the new verb seems to do two completely separate things: building a sysroot image and building the workspace. I think all the wording around when the sysroot is being build, when it needs to be rebuild and options like
--force-sysroot-build
are indicators that it might be better to separate these two parts.
That's fair. I guess rosdep
is not updating the locally installed packages so it could be the same for the sysroot image. If that's becoming a usability problem, we can always revisit this decision down the road so I'm not worried about this.
- Also I am not sure that
colcon
needs a way to use Docker. Instead of couplingcolcon
with knowledge about Docker why no just invokecolcon
within Docker -colcon
doesn't need to know about this imo.
This part is the tricky part: the Docker image we're talking about there not a native image. It could e.g. an ARM Docker image. Meaning that anything inside is running through qemu
with a 10x penalty (relying on binfmt_misc
hooks).
That's what we're proposing is not equivalent to just run colcon
in Docker. If we do that, then development becomes a pain because all of colcon code (Python, etc.) is qemu based. The approach described in this PR is only paying the cost of qemu for the initial build of the workspace as well as every time the image gets updated. Obviously, I don't have number to show you but the hope is that having rosdep + apt-get being slow is not too much of a problem compared to having colcon + gcc/g++ running through qemu.
Otherwise, woud you be OK with us teaching rosdep how to build Docker images?
Wouldn't that have the same problem of adding a new, different function to a tool? rosdep
is for installing dependencies, so I think that your proposal should be reversed somewhat, and that you should provide a script that wraps rosdep
and produces a Docker image.
Otherwise, woud you be OK with us teaching rosdep how to build Docker images?
Wouldn't that have the same problem of adding a new, different function to a tool?
rosdep
is for installing dependencies, so I think that your proposal should be reversed somewhat, and that you should provide a script that wrapsrosdep
and produces a Docker image.
Sure that SGTM.
Introducing a tool (or a verb) to build sysroots sounds like what things like OpenEmbedded do. From the design doc it seems like going that route is not something @juanrh and @thomas-moulard have in mind. Is that correct?
Otherwise the work that LG does/has done with superflore (https://github.com/ros-infrastructure/superflore/pull/168) might be relevant?
Edit: blah, I'd actually read the doc, and it already states this:
However, that tutorial doesn't provide instructions to build other ROS 2 packages, but just for building a whole ROS 2 distribution. Ideally, we should be able to cross-compile a ROS package for ROS 2 [..]
That's indeed not a typical use-case for OpenEmbedded et al (but it can be done afaik).
The design looks good and can be a first step in getting cross compile ROS 2 packages and testing on the target platform.
We follow the following steps at Apex.AI to cross compile and test. Open for suggestions/comments to make it better and contribute it back.
Develop on a x86_64 ubuntu linux system
Cross compile on X86_64 for aarch64. This PR should make it one step process
SCP the build and install folder on to the target aarch64 hardware and do unit testing.
Submit to CI
We follow the above steps for aarch64/linux, x86_64/qnx and aarch64/qnx.
As part of step-3, we will start using the qemu/docker image for aarch64/Linux.
We follow the above steps for application testing, performance testing.
I'm surprised it hasn't been pointed out yet (has the policy changed?) but the format guidelines say one sentence per line and don't word wrap.
I'm surprised it hasn't been pointed out yet (has the policy changed?) but the format guidelines say one sentence per line and don't word wrap.
I didn't focus on stylistic feedback yet since before that the conceptional problems / questions need to be addressed. But yes, the style guide contains the following:
> Each sentence must start on a new line.
Do we have any other comments on the new design with a separate command to setup the sysroot, and a colcon mixin for running the build?
@juanrh When you add/update commits ready for review please comment soon after so we know it's ready for rereview otherwise we don't get a notification and won't know it's been updated. I'll take a look at this shortly.
Any other comments on this? Thanks
Thanks for your comments @jacobperron, I have addressed them in a new commit
Any other comments on this design?
PR moved to https://github.com/ros2/design/pull/249
This pull request has been mentioned on ROS. There might be relevant details there:
https://discourse.ros.org/t/announcing-first-release-of-ros-cross-compiler-tool/12601/6
Design doc for task ros2/ros2#693
Signed-off-by: Juan Rodriguez Hortala hortala@amazon.com
To be reviewed before 2019-04-26