ros-tooling / cross_compile

A tool to build ROS and ROS2 workspaces for various targets

Apache License 2.0

188 stars 60 forks source link

Fix workspace builder script when ROS actually installed, #143

Closed emersonknapp closed 4 years ago

emersonknapp commented 4 years ago

The build_workspace.sh script caused an error upon sourcing setup.bash because of unset variables, if ROS was actually installed.

To fix:

remove the set -u option for that script
add explicit checking for required variables
update e2e test dummy package to require ROS2

After updating the package but before updating the script, the e2e test failed. Now it succeeds and is performing a more thorough test.

Part of the replacement for #139

Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com

codecov[bot] commented 4 years ago

Codecov Report

Merging #143 into master will not change coverage by %. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #143   +/-   ##
=======================================
  Coverage   87.11%   87.11%           
=======================================
  Files           6        6           
  Lines         225      225           
=======================================
  Hits          196      196           
  Misses         29       29

Flag	Coverage Δ
#unittests	`87.11% <ø> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a7f38fa...2fe7ec3. Read the comment docs.

prajakta-gokhale commented 4 years ago

What is the reason behind making dummy_pkg a ROS 2 package? (Apologies if I'm missing context from earlier PRs)

emersonknapp commented 4 years ago

Is your question "as opposed to a ROS 1 package"? - arbitrary choice, the bug here affects either version

Otherwise if question is "as opposed to what it currently was" - If you read my description for the PR, having it not be a ROS package was causing a breaking bug to successfully pass our e2e tests

prajakta-gokhale commented 4 years ago

Otherwise if question is "as opposed to what it currently was" - If you read my description for the PR, having it not be a ROS package was causing a breaking bug to successfully pass our e2e tests

Yes my question was "as opposed to what it previously was". It makes sense now, I see that it is made a ROS 2 package so that it always requires ros2 to be installed and sourced.

emersonknapp commented 4 years ago

The outcome of last e2e test note:

it tried to build the ROS2 package for the ROS1 platforms, that failure is fine and I will fix it
ROS2 distributions only target debian as a Tier3 platform, so we can't resolve rosdep keys for ROS2 packages themselves for targeting Debian. We can resolve rosdep keys for external dependencies. This means that for targeting Debian, a user will need to perform a full source build of the ROS2 core. I'm not sure exactly how I want to handle this case...

I think I will split out 3 cases:

empty dummy_pkg like previously, for ROS2+Debian cases
ROS 1 dummy for ROS1 cases
use this modified dummy for ROS2+Ubuntu targets That should increase our surface area. I'll also update the README.md to explain the situation

thomas-moulard commented 4 years ago

Seems like this will require more work, so I'm not reviewing it now. But your approach LGTM. Let's make sure we add plenty of comments around that rationale.

emersonknapp commented 4 years ago

@thomas-moulard requesting re-review. I have updated with a ROS2 application and some documentation. This is a strict improvement over the current situation, and solves the identified bug for both ROS 1 and 2 workspaces, since the colcon setup.bash file is the same for either.