search

uwreact / uwreact_robot

Software behind our fully autonomous FIRST robots
BSD 3-Clause "New" or "Revised" License
8 stars 6 forks source link

Add ZED dependencies #58

Closed wraftus closed 5 years ago

wraftus commented 5 years ago

Pull Request

Contribution Checklist

Change Checklist

wmmc88 commented 5 years ago

BUILD FAILED. Git gud

wraftus commented 5 years ago

oof, it needs CUDA and the ZED SDK to build and I'd imagine travis doesn't have that

wmmc88 commented 5 years ago

Pretty sure it only needs CUDA and the sdk at runtime

wraftus commented 5 years ago

well catkin build fails unless you have them, unless that's something else

wmmc88 commented 5 years ago

Oof ye nvm u need the sdk to build

ghost commented 5 years ago

I'd look into the travis docs and talk to @Matthew-Reynolds who set up the ROS CI to see if you can add the dependencies you need. If travis isn't enough, https://circleci.com will definitely be enough and we may have to look into transitioning sooner.

ghost commented 5 years ago

But most importantly, this should not be merged until CI works, no exceptions

matthew-reynolds commented 5 years ago

tl;dr: Blocked by frc_control PR https://github.com/uwreact/frc_control/pull/31 and blocked by waiting for ros-melodic-depthimage-to-laserscan to be added to apt.

Travis vs Circle doesn't matter here at all. In both cases, we're running in a custom docker container. Since we're in normal docker, and not nvidia's special cuda-compatible nvidia-docker, trying to actually use CUDA will fail, but since we're only linking against CUDA and the ZED sdk, we're good to go.

ZED provides docker images for various configurations that already have CUDA and the ZED sdk installed. Doing some local testing, industrial-ci seems to have no issues building the zed ros code in their provided zed:ubuntu1804-cuda10.0-zed2.7-ros docker image. However, there are a few things blocking this PR.

1) The zed-ros-wrapper is written for Kinetic. Some of the examples require depthimage_to_laserscan, which until 6 days ago, wasn't released in Melodic. It has since been added to the release index, so the next time the packages get updated, we should be in the clear for this. It is currently in the ros-shadow-fixed distribution, but not the main ros distribution. These get synced biweekly, but I don't know where in that 2 week cycle we are right now. But we are blocked until this package is available for install with apt. Workarounds for now are to either add depthimage_to_laserscan to our .rosinstall, or to only build with ROS_REPO=ros-shadow-fixed (We normally build with both ros and ros-shadow-fixed).

2) Using ubuntu18 for our CI breaks the build because frc_control hasn't merged its big update to 18 yet. However, frc_control's update is ready to be merged (https://github.com/uwreact/frc_control/pull/31), so this won't block us for long.

3) industrial-ci expects the user in the docker image to be root so that it can install packages and such. Zed's docker images, unlike the ones industrial-ci provides, don't have root as the default user. By manually editing the industrial-ci scripts, we can get around this, but this clearly isn't a viable workaround. I need to dig a bit deeper and figure out how to specify a user for the docker container through our .travis.yaml file.

matthew-reynolds commented 5 years ago

~Side note, we should add a blocked tag~ On Hold has been renamed Blocked :)

matthew-reynolds commented 5 years ago

Update on my prev comment: We are right at the beginning of the 2 week cycle. Last sync was Feb 11, next will be ~Feb 25. I suggest our action for now is to remove our ros travis job, and only leave the ros-shadow-fixed job for the next 2 weeks, and then revert that change.

matthew-reynolds commented 5 years ago

Status update:

This means the only thing blocking this PR is uwreact/frc_control#31 (see also #55). Once that has been merged, we can validate that things look good here, and then proceed with reviews.

matthew-reynolds commented 5 years ago

uwreact/frc_control#31 is merged, rerunning CI.

matthew-reynolds commented 5 years ago

CI successful - PR is unblocked. @wmmc88 @michaelwm @wraftus Let's proceed with review.

wraftus commented 5 years ago

Okay, finally got around to doing those documentation changes. Let me know if it's inline with what you guys want

matthew-reynolds commented 5 years ago

@wmmc88 Please review so this PR can be merged.

matthew-reynolds commented 5 years ago

@wraftus I have changed this status to urgent since it is now blocking #64. Please acknowledge Melvin's comment and merge this PR ASAP.