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

Fix installation of dependencies #820

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rhaschke
Copy link
Contributor

ici_install_dependencies shouldn't install dependencies that are available in an underlay. Usually, this is ensure by construction, starting from a clean /opt/ros and then installing dependencies for upstream, target, and downstream in turn. However, there is the UNDERLAY variable, which allows to use another underlay than /opt/ros. In that case, the underlay might provide packages, which would be required for install.

Here is an example using UNDERLAY=/root/ws_moveit/install from a custom docker image. In that case, installation shouldn't care about moveit-* packages, which didn't work before.

Dependencies of the extended workspace are available by construction.
@mathias-luedtke
Copy link
Member

I think both your commits must get mixed. So that we ignore packages in UNDERLAY and extend in all kinds of scenarios.

@rhaschke
Copy link
Contributor Author

Maybe, you are right. We would need the following paths:

  • in upstream: UNDERLAY
  • in target: UNDERLAY + upstream
  • in downstream: UNDERLAY + upstream + target

@rhaschke
Copy link
Contributor Author

rhaschke commented Apr 1, 2023

I have no clue why the installation of colcon, particularly python3-colcon-recursive-crawl, gets stuck in an infinite loop for CI / ici (foxy, rosenv ros2 run industrial_ci run_travis, ros-foxy-ros2run) (which is cancelled by GHA after the timeout of 6h):

  Setting up python3-colcon-recursive-crawl (0.2.1-1) ...
  Error: The operation was canceled.

The changes of this PR become effective much later...
Otherwise this seems to work now.

@@ -173,10 +173,12 @@ function run_source_tests {
ici_step "setup_rosdep" ici_setup_rosdep

extend=${UNDERLAY:?}
export ROSDEP_SOURCE_FOLDERS=("${UNDERLAY:?}") # source folders to be ignored for rosdep install
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go into env.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable is configured only here (incrementally for all stages).
Moving the initialization into another file seems odd.

@@ -386,7 +386,7 @@ function ici_build_workspace {
fi

ici_step "setup_${name}_workspace" ici_prepare_sourcespace "$ws/src" "${sources[@]}"
ici_step "install_${name}_dependencies" ici_install_dependencies "$extend" "$ROSDEP_SKIP_KEYS" "$ws/src"
ici_step "install_${name}_dependencies" ici_install_dependencies "$extend" "$ROSDEP_SKIP_KEYS" "$ws/src" "${ROSDEP_SOURCE_FOLDERS[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

We might even move this into ici_install_dependencies, then it would always use the UNDERLAY by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key is that this variable incrementally changes. Using UNDERLAY always is not intended. I would have made the variable local in run_source_tests but it needs to propagate here.

Copy link
Member

Choose a reason for hiding this comment

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

I meant that it will always start from UNDERLAY, just like you intended.

@mathias-luedtke
Copy link
Member

mathias-luedtke commented Jun 2, 2023

The current version couples run_source_tests and ici_build_workspace, but the latter is used in run_abi_check as well.
The tests pass, because they do not cover this particular case.

Right now I am leaning towards making extend chainable, just like CMAKE_PREFIX_PATH etc.

@mathias-luedtke
Copy link
Member

@rhaschke: Please have a look at #833
The foxy job stalls as well, I will try to fix it.

@mathias-luedtke
Copy link
Member

I think I know why it is stalling..
Both of our fixed set UNDERLAY as source (in order to ignore them), but this forces rosdep install to install all missing dependencies. It might be better to just scan the packages and command rosdep to skip them.

@mathias-luedtke
Copy link
Member

I have been digging a little bit deeper:

ici_install_dependencies shouldn't install dependencies that are available in an underlay.

As far as I can tell, ici_install_dependencies does not install them directly

However, there is the UNDERLAY variable, which allows to use another underlay than /opt/ros. In that case, the underlay might provide packages, which would be required for install.

And those packages will be excluded by rosdep, because they are list in ROS_PACKAGE_PATH.


Your example builds panda_moveit_config in the upstream workspace, where it can use the the packages from the underlay.

Your counter-example (which didn't work before) installs ros-noetic-panda-moveit-config, which forces apt-get to install all dependencies.

Works fine with the current master version:

./industrial_ci/scripts/run_ci DOCKER_IMAGE=moveit/moveit:noetic-source TARGET_WORKSPACE="github:rhaschke/moveit_tutorials#0c883c0a8a8aef9340da3840a318ad48fc36f5af" UNDERLAY=/root/ws_moveit/install UPSTREAM_WORKSPACE=github:ros-planning/panda_moveit_config#noetic-devel

install_target_dependencies
$ ( source /root/upstream_ws/install/setup.bash && rosdep install -q --from-paths /root/target_ws/src --ignore-src -y | grep -E '(executing command)|(Setting up)' ; )
executing command [apt-get install -y -qq ros-noetic-moveit-visual-tools] rosdep
Setting up libqt5x11extras5:amd64 (5.12.8-0ubuntu1) ...
Setting up ros-noetic-graph-msgs (0.1.0-2focal.20220926.190945) ...
Setting up ros-noetic-roslint (0.12.0-1focal.20210423.224843) ...
Setting up ros-noetic-geometric-shapes (0.7.5-1focal.20230412.160132) ...
Setting up ros-noetic-srdfdom (0.6.4-1focal.20230516.061359) ...
Setting up ros-noetic-moveit-msgs (0.11.4-1focal.20230216.003351) ...
Setting up libqt5x11extras5-dev:amd64 (5.12.8-0ubuntu1) ...
Setting up ros-noetic-moveit-core (1.1.12-1focal.20230524.152908) ... apt-get/dpkg
Setting up ros-noetic-rviz-visual-tools (3.9.3-1focal.20230330.132121) ...
Setting up ros-noetic-moveit-ros-occupancy-map-monitor (1.1.12-1focal.20230524.155332) ...
Setting up ros-noetic-moveit-ros-planning (1.1.12-1focal.20230524.155710) ...
Setting up ros-noetic-moveit-visual-tools (3.6.1-1focal.20230524.161213) ...

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.

None yet

2 participants