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

Correct rosdistro keys #32

Merged
merged 3 commits into from May 16, 2020
Merged

Correct rosdistro keys #32

merged 3 commits into from May 16, 2020

Conversation

ruffsl
Copy link
Contributor

@ruffsl ruffsl commented Feb 13, 2020

This corrects the rosdistro key names so rosdep can properly resolve and install missing dependencies, as well fix some issues in finding exported headers from upstream projects.

See official list of rosdistro keys here:
https://github.com/ros/rosdistro/blob/master/rosdep/base.yaml

Related:

@ToniRV
Copy link
Collaborator

ToniRV commented Mar 11, 2020

So my understanding is that all the depend names in package.xml should reflect the ones that we want to depend on, which should be the ones we install in the workspace (+/- the ones that are already in ROS core packages).
So namings should follow:
https://github.com/MIT-SPARK/Kimera-VIO-ROS/blob/master/install/kimera_vio_ros_ssh.rosinstall

package.xml Show resolved Hide resolved
<depend>dbow2</depend>
<depend>gtsam</depend>
<depend>kimera_rpgo</depend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not ok, as I think it is KimeraRPGO, we should make a PR to the RPGO repo updating the name there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<depend>gtsam</depend>
<depend>kimera_rpgo</depend>
<depend>libgflags-dev</depend>
<depend>libgoogle-glog-dev</depend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the two above are irrelevant actually because we use cmake files to find glog/gflags as I had tons of issues with these two in multiple systems...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use cmake files to find glog/gflags

Not sure being explicit about this external dependency the manifest would change what the CMake does to find the glog/gflags installed in the cmake path. Are you saying that the system install of glog/gflags in the latest ubuntu LTS is still an issue?

<depend>kimera_rpgo</depend>
<depend>libgflags-dev</depend>
<depend>libgoogle-glog-dev</depend>
<depend>libopencv-dev</depend>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, should be opencv or rather opencv3_catkin if we use ETH's catkinized opencv3

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use libopencv-dev, what are we assuming? That ppl installed opencv how?

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 is the correct/official rosdep key name for the opencv3 library:

https://github.com/ros/rosdistro/blob/4bfca115e18c733686b3b39524fbf1e227177574/rosdep/base.yaml#L2815

By declaring the same key name as the dependency, users with other packages depending on opencv3 don't have to monkey patch there manifest to use the same key to avoid rosdep from installing libopencv-dev from apt repo if the kimera workspace it builds from already includes a source build of opencv.

Note how this is used in the Dockerfile that builds the underlay for the ros wrapper:
https://github.com/MIT-SPARK/Kimera-VIO-ROS/pull/32/files#diff-3254677a7917c6c01f55212f86c57fbfR71-R95

CMakeLists.txt Outdated Show resolved Hide resolved
@ruffsl ruffsl mentioned this pull request May 5, 2020
@ruffsl ruffsl changed the title Correct rosdistro keys and patch cmake build Correct rosdistro keys May 13, 2020
@ToniRV
Copy link
Collaborator

ToniRV commented May 13, 2020

@ruffsl if you want we can merge this one after updating to master and if Jenkins is ok with it 👍

@ruffsl
Copy link
Contributor Author

ruffsl commented May 13, 2020

@ruffsl if you want we can merge this one after updating to master and if Jenkins is ok with it +1

@ToniRV I've rebased this PR onto master. I think the 18.04 test might be flaky, as it passed before rebasing, and the error dosn't seem to pertain to ros-dep keys. The builds seems green though!
Could you also merge this PR as well: MIT-SPARK/Kimera-RPGO#51

@ToniRV ToniRV merged commit 480a70a into MIT-SPARK:master May 16, 2020
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