-
Notifications
You must be signed in to change notification settings - Fork 640
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
Fixes for building on OS X and against pcl-1.7 #8
Conversation
I'm not sure why this works on Linux? data is not a static member of VelodynePacket, it should not be accessed with an instance.
pcl_conversions is explicitly not a run_depend nor a transitive dependency as it is only used in the cc files.
@@ -13,7 +13,7 @@ set(${PROJECT_NAME}_CATKIN_DEPS | |||
velodyne_msgs) | |||
|
|||
find_package(catkin REQUIRED COMPONENTS | |||
${${PROJECT_NAME}_CATKIN_DEPS} roslaunch rostest) | |||
${${PROJECT_NAME}_CATKIN_DEPS} pcl_conversions roslaunch rostest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add pcl_conversions
to ${PROJECT_NAME}_CATKIN_DEPS
?
Is it because it's only a build dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is explicitly not a transitive dependency, because it is not used in any header files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I mentioned that in the commit message:
https://github.com/wjwwood/velodyne/commit/6b1437f3a618a7cc2369bd2cf3993d7efd94b5ee
:)
Fixes for building on OS X and against pcl-1.7
What do I have to check out to build and test this? It does not work with my current Hydro development workspace:
|
You would have to use the building repo or build pcl and pcl_ros from source in order to test this, shadow fix is catching up in a day or so. I built everything from source to test this, but I already had stuff from source in order to test pcl and pcl_ros so it wasn't too bad for me. |
I'm based on shadow-fixed. Explicitly installing I had some more problems, then explicitly installed After that, it built cleanly and unit tests run successfully. I'll release soon. Thanks for the patch, William! |
BTW, shouldn't |
I forgot to mention this:
The ros-hydro-pcl in shadow-fixed is not providing any CMake find_package module. |
@jack-oquin Yeah, you're right ros-perception/pcl_conversions#1 I haven't been able to reproduce the missing PCLConfig.cmake file, that might be new with 1.7.0-9 (pcl-1.7.0rc2)... Can you grep for PCLConfig.cmake in the pcl-1.7.0-9 deb and see if it is listed? (I don't have a ubuntu machine handy) It may be that it is getting installed into a place which CMake is not looking? |
Yep, it's somewhere else:
|
I just ran a pre-release test, which failed with this error I'd not seen on my development system:
|
@jack-oquin It must be the wrong branch or something, my changes fix that here: |
@jack-oquin Can you try this:
|
The above works on my machine, even with the odd locations. It might also be a change in cmake, what version are you using? I am using:
|
cmake 2.8.7 The pre-release build had the same warning. For the previous message:
I don't know what's going on, but that seemed to work. |
Re: the version checked out by the pre-release:
That tree does have your fixes on github. |
I'm running another pre-release, just to see. |
That one failed the same way. I have no idea what's going on. Probably I am just doing something stupid. I need to get ready for a music gig, no more time to work on this today. Thanks for all the help. I'll probably look at it again with fresher eyes tomorrow morning. |
Ok. |
Fixes PCL 1.7 compatibility problems (ros-drivers/velodyne#8).
I never did figure out what that pre-release problem was, but it seems to work now. |
The changes in 6b1437f are specific to hydro.