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

Add multi-release support, bump SDK to 2.11.3.121 for xenial and 2.9.3.43 for trusty #127

Merged
merged 10 commits into from
Jul 18, 2017
Merged

Conversation

Baycken
Copy link
Contributor

@Baycken Baycken commented Jul 6, 2017

  • Fixed all catkin lint -W2 errors and warnings.
    Only significant change is change the how download_flycap is run from cmake: Execution bit of the script is removed and the cmake invoke python to run the script instead of running the script as executable. This is done to resolve warning about not installed executable.

  • Use cmake magic to choose which version on flycap to use for xenial and trusty thus allow both release to be used.

  • Bump the SDK version for xenial to 2.3.11.121 and for trusty to 2.3.9.43.

@@ -100,6 +101,5 @@ if (CATKIN_ENABLE_TESTING)
roslaunch_add_file_check(launch/camera.launch)
roslaunch_add_file_check(launch/stereo.launch)

find_package(roslint REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

This line needs to remain, or the code linter macro won't work.

Copy link
Contributor Author

@Baycken Baycken Jul 6, 2017

Choose a reason for hiding this comment

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

I removed the roslint find_package for catkin and added the line back

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that roslint is only a test_depend, so it needs to only be looked for inside the if(CATKIN_ENABLE_TESTING) block.

@mikepurvis
Copy link
Member

mikepurvis commented Jul 6, 2017

Thanks very much for looking at this— I know there have been a number of reports around 16.04, particularly in #73, a previous PR along similar lines.

It looks like you have a bunch of __cxx11 linker errors (indicating an STL ABI mismatch) occurring in the Travis environment, which is Ubuntu Trusty + GCC 4.8.4. Since we do get a clean build from the buildfarm (running Xenial with GCC 5.3.1), I think our options are:

  • Drop support for Indigo, at least in the master branch. Either eliminate the Travis job, or switch it to use a containerized Xenial. Create an indigo-devel branch as required to manage further maintenance-only changes targeting Indigo.
  • Make the build able to detect which STL is present, and download the correct version of FlyCapture, thus maintaining some semblance of Trusty/Indigo support on the master branch.

Thoughts? @konradb3 @ironmig @tedchina

@kev-the-dev
Copy link
Contributor

I'd like to see the indigo compatibility maintained as there are still several years left in indigo support and I'm sure there's a lot of robots out there running indigo with pointgrey cameras still (my lab has at least one). Perhaps create an indigo-devel branch like many big ROS project have adopted and occasionally backport changes if they build (while maintaining this diff in the SDK version of the cmake). What do you think about this?

@Baycken
Copy link
Contributor Author

Baycken commented Jul 7, 2017

I added version selection for Ubuntu releases by using code name, I think this way both indigo/trusty and kinetic/xenial can be supported

@Baycken Baycken changed the title catkin lint -W2 fix and bump SDK to 2.11.3.121 Add multi-release support, bump SDK to 2.11.3.121 for xenial and 2.9.3.43 for trusty Jul 8, 2017
@Baycken
Copy link
Contributor Author

Baycken commented Jul 18, 2017

@mikepurvis, how does this look now?

@mikepurvis
Copy link
Member

Looks terrific. Thanks for your patience and persistence in getting this through.

@mikepurvis mikepurvis merged commit ef74de6 into ros-drivers:master Jul 18, 2017
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.

4 participants