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

Include noetic build in CI #67

Open
wants to merge 9 commits into
base: kinetic-devel
Choose a base branch
from

Conversation

stwirth
Copy link
Contributor

@stwirth stwirth commented Feb 12, 2021

  • Makes CI build for noetic as well
  • Adds some missing #includes
  • Simplifies the download_spinnaker script and makes it download a newer version of the SDK for noetic
  • Removes the "install" step from the build: I would suggest everyone to install the official Spinnaker SDK when using this driver, so this auto-downloading feature is just used for CI and development workspaces.

@stwirth
Copy link
Contributor Author

stwirth commented Feb 12, 2021

Seems the CI doesn't like

#!/usr/bin/env python

Maybe the script should be ported to python3? Should still work fine for kinetic/xenial.

@stwirth
Copy link
Contributor Author

stwirth commented Feb 12, 2021

Clearpath seems to host the 2.2.0.48 version of the Spinnaker SDK on https://packages.clearpathrobotics.com/stable/flir/Spinnaker/Ubuntu20.04/. The download_spinnaker script would need to be adapted to pull from there. The version specified there (1.27.0.48) does not seem to be published by FLIR for Ubuntu 20.04.

@stwirth
Copy link
Contributor Author

stwirth commented Feb 12, 2021

Still a little more work needed to match download_spinnaker and DownloadSpinnaker.cmake.

@stwirth stwirth changed the title Include noetic build in CI [WIP] Include noetic build in CI Feb 12, 2021
@stwirth
Copy link
Contributor Author

stwirth commented Feb 25, 2021

Last noetic build went through but kinetic and melodic failed with:

make[2]: *** No rule to make target 'spinnaker-sdk/usr/lib/libSpinnaker.so', needed by '/root/target_ws/devel/.private/spinnaker_camera_driver/lib/libSpinnakerCameraLib.so'.  Stop.

Took me almost a day to find out that this is caused by a broken symbolic link in the spinnaker debians version 1.27.0.48. 🤯

@stwirth stwirth marked this pull request as draft February 25, 2021 15:05
@stwirth stwirth closed this Mar 8, 2021
@stwirth stwirth reopened this Mar 8, 2021
@stwirth stwirth changed the title [WIP] Include noetic build in CI Include noetic build in CI Mar 8, 2021
@stwirth stwirth marked this pull request as ready for review March 8, 2021 16:39
@stwirth
Copy link
Contributor Author

stwirth commented Mar 8, 2021

@mhosmar-cpr this is ready for review.

@stwirth
Copy link
Contributor Author

stwirth commented Mar 23, 2021

@mhosmar-cpr ping

@Jaeyoung-Lim
Copy link

@stwirth @mhosmar-cpr Any reason that this is not getting in?

@stwirth
Copy link
Contributor Author

stwirth commented Apr 2, 2021

@stwirth @mhosmar-cpr Any reason that this is not getting in?

Not from my side, this is waiting for comment/merge from a maintainer.

# Redistributing the flycapture .so file is permitted by the SDK EULA:
# http://www.ptgrey.com/support/kb/data/PGR-FlyCap-SDK-LA.pdf
if(EXISTS ${CMAKE_CURRENT_BINARY_DIR}/usr/lib)
install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/usr/lib/ DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we're not bundling the sdk anymore. Do we not need this for the deb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that license agreement is outdated (there is no flycapture / pointgrey anymore, it's Spinnaker / FLIR now) and I think it is better to let the users install the SDK by themselves. We could try to make a separate ROS package that just catkinizes the Spinnaker SDK but I won't have time for that. The official way of installing the SDK also includes an explicit manual confirmation of the license.

So my suggestion is to use the download script just for CI and encourage the user to use this package from source.
Any better ideas?

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.

3 participants