Skip to content

Switch CI to use ros-industrial master#21

Merged
Levi-Armstrong merged 2 commits into
tesseract-robotics:masterfrom
mpowelson:fix_ci
Jul 27, 2020
Merged

Switch CI to use ros-industrial master#21
Levi-Armstrong merged 2 commits into
tesseract-robotics:masterfrom
mpowelson:fix_ci

Conversation

@mpowelson
Copy link
Copy Markdown
Contributor

The fix needed in ipa-mdl's branch has since been merged.

@mpowelson
Copy link
Copy Markdown
Contributor Author

@Levi-Armstrong Well using master does not work the same way ipl-mds's branch used to. Trying to decipher the comments it looks like I need to do something to the docker. In the meantime, I switched it to the target workspace instead of upstream. Hopefully that will fix it.

@mpowelson
Copy link
Copy Markdown
Contributor Author

Well tesseract_python is broken. I don't think I am going to spend time fixing this right now.

@mathias-luedtke
Copy link
Copy Markdown

mathias-luedtke commented Jun 8, 2020

@mpowelson: Your Docker image contains outdated versions of colcon/ament/rosdep (not sure, which one was fixed).

If you cannot fix the image, you might try an upgrade: AFTER_INIT: apt-get upgrade -y.

Comment thread .github/workflows/main.yml Outdated
ROS_DISTRO: eloquent,
ROS_REPO: main,
UPSTREAM_WORKSPACE: 'dependencies.rosinstall',
TARGET_WORKSPACE: 'dependencies.rosinstall',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This omits your target repository, UPSTREAM_WORKSPACE should work fine with a newer build system.

@mpowelson mpowelson force-pushed the fix_ci branch 2 times, most recently from 32f1afb to fd537a7 Compare June 8, 2020 19:10
The fix needed in ipa-mdl's branch has since been merged.
@mathias-luedtke
Copy link
Copy Markdown

Something is wrong with the upstream_ws or how it is built. ros2 pkg list does not list any of these packages.

@mathias-luedtke
Copy link
Copy Markdown

Are all tesseract packages built with cmake only? I think they are not added to the ROS2 index.

@mpowelson
Copy link
Copy Markdown
Contributor Author

Yes. They are all ROS agnostic and do not use Ament, so I think that is correct. I don't think they are added to the index unless the ament_index file is manually created like is being done in tesseract_collision

@mathias-luedtke
Copy link
Copy Markdown

I guess you build is failing now because of ros-infrastructure/rosdep#724

@Levi-Armstrong
Copy link
Copy Markdown
Contributor

@schornakj Didn't we add the creation of these for ROS2 support to Tesseract?

@@ -28,14 +29,15 @@ jobs:
UPSTREAM_WORKSPACE: 'dependencies.rosinstall',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
UPSTREAM_WORKSPACE: 'dependencies.rosinstall',
TARGET_WORKSPACE: '. dependencies.rosinstall',

The dot means target repository.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this puts all packages into the targer workspace, but it will build and run all tests as well.

@Levi-Armstrong
Copy link
Copy Markdown
Contributor

It looks like we only do this for tesseract_collision. We should add this to the tesseract_configure_package so all get added.

@Levi-Armstrong
Copy link
Copy Markdown
Contributor

This is what we are doing for pure cmake packages.

# Create an ament_index resource file to allow ROS2 ament_index_cpp to locate the installed path to this package.
# This is a workaround to let the ROS2 version of pluginlib find tesseract_collision's plugins, since tesseract_collision is a non-ROS CMake package.
# ADDITIONAL REQUIREMENT: The installed path must be added to the AMENT_PREFIX_PATH environment variable at runtime, which is outside the scope of CMakeLists.txt.
file(WRITE ${CMAKE_INSTALL_PREFIX}/share/ament_index/resource_index/packages/${PROJECT_NAME} "")

@mathias-luedtke
Copy link
Copy Markdown

mathias-luedtke commented Jun 8, 2020

ADDITIONAL REQUIREMENT: The installed path must be added to the AMENT_PREFIX_PATH environment variable at runtime, which is outside the scope of CMakeLists.txt

That needs to be done somehow
I could come up with a solution like ros-industrial/industrial_ci#496, but it would still be broken for others..

@mathias-luedtke
Copy link
Copy Markdown

I am currently working on a generic fix (ros-industrial/industrial_ci#496).

@mpowelson
Copy link
Copy Markdown
Contributor Author

Thanks. It seems like ultimately this needs to be fixed in rosdep.

@schornakj
Copy link
Copy Markdown
Contributor

@schornakj Didn't we add the creation of these for ROS2 support to Tesseract?

That was originally a solution to the way ROS2 pluginlib finds plugins, which relies on ament_index_cpp to find packages using the ament package index. It was only needed for the tesseract_collision package, since that was the only Tesseract package that contained plugins.

I agree that it's a shortcoming of rosdep, as well as pluginlib.

@mathias-luedtke
Copy link
Copy Markdown

I am currently working on a generic fix (ros-industrial/industrial_ci#496).

I have merged the fix into the master branch.
It is not ideal, but better than a lot of unexpected CI failures.

80affcc should work now without the AFTER_INIT script

@Levi-Armstrong
Copy link
Copy Markdown
Contributor

@schornakj It looks like there may be solution in Eloquent explained here. It looks like we just need to install two new files along with the index file for each package. Would you have time to test this and see if it eliminates the need to modify the AMENT_PREFIX_PATH? @ipa-mdl This may also solve the rosdep issue for pure cmake packages. We will give it a try.

@gavanderhoorn

This comment has been minimized.

@schornakj
Copy link
Copy Markdown
Contributor

schornakj commented Jul 10, 2020

@Levi-Armstrong @mpowelson @gavanderhoorn @ipa-mdl Does anything else need to happen with this PR before we can merge it?

Comment thread .github/workflows/main.yml Outdated
ROS_DISTRO: eloquent,
ROS_REPO: main,
ROSDEP_SKIP_KEYS: "bullet3 fcl ompl",
ROSDEP_SKIP_KEYS: "bullet3 fcl ompl tesseract tesseract_collision tesseract_common tesseract_environment tesseract_geometry tesseract_kinematics tesseract_motion_planners tesseract_process_planners tesseract_scene_graph tesseract_support tesseract_urdf tesseract_visualization",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this required now with the fixes to tesseract updating the prefix path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. It worked without it on mine. I just removed it and added cmake_common_scripts

Comment thread .github/workflows/main.yml
Comment thread .github/workflows/main.yml
@mpowelson
Copy link
Copy Markdown
Contributor Author

@Levi-Armstrong Could we get the foxy build added to the dockerhub, so I could modify this to use that instead?

This is a new dependency of opw_kinematics
@Minipada
Copy link
Copy Markdown

I have the changes ready, will push as soon as it's on dockerhub

@Levi-Armstrong
Copy link
Copy Markdown
Contributor

I have fixed the docker hub hooks in the docker repository for tesseract. Foxy is currently building but the other three have successfully built.

@Levi-Armstrong
Copy link
Copy Markdown
Contributor

Foxy Docker is now available.

@mpowelson
Copy link
Copy Markdown
Contributor Author

I will not be able to mess with this more any time soon to add foxy, but could we merge this so we stop getting nightly emails that ci is broken?

@Levi-Armstrong Levi-Armstrong merged commit de87ff6 into tesseract-robotics:master Jul 27, 2020
@mpowelson
Copy link
Copy Markdown
Contributor Author

Thanks

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.

6 participants