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

Fix build of downstream packages. #407

Merged
merged 2 commits into from Sep 24, 2020

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Sep 24, 2020

Fixed libpointmatcher_ros build in catkin workspace.

@jlblancoc
Copy link
Contributor

LGTM. Thanks! 👍

@peci1
Copy link
Contributor Author

peci1 commented Sep 24, 2020

The pkgconfig files are definitely still wrong as they have no way to expand the generator expressions. But I'll leave that to others as my interest is in the CMake config files.

Could you please require that contributors (or Jenkins) do a more complex test than "hey, this library builds alone" before doing such substantial changes to the project?

@YoshuaNava
Copy link
Contributor

Could you please require that contributors (or Jenkins) do a more complex test than "hey, this library builds alone" before doing such substantial changes to the project?

If I remember well, the Jenkins pipeline passed for the original PR. Another user also tested and said that it solved the issues with libnabo.

Can you provide more info about your setup and how you tested it? Based on that feedback we could improve the tests.

@pomerlef
Copy link
Collaborator

@peci1 I agree that those build modifications are annoying, but we don't do that really often. It's also rare that a library test for downstream dependencies, although I agree that libpointmatcher_ros is maintained by ourselves also.

@pomerlef pomerlef merged commit 95647d3 into norlab-ulaval:master Sep 24, 2020
@pomerlef
Copy link
Collaborator

@peci1 thanks for being quick to solve the dependency issues. We should converge soon ;)

@peci1
Copy link
Contributor Author

peci1 commented Sep 24, 2020

If I remember well, the Jenkins pipeline passed for the original PR. Another user also tested and said that it solved the issues with libnabo.

Yes, but the author of the PR also said that he only tested in build-space. Testing install-space is what's probably missing.

It seems to me that a good way to test the integration would be to have some kind of integration builds which would build all the things down to e.g. ethzasl_icp_mapper (actually doing make install for each repo). This way, you could be sure that changes in the lower-level libraries do not break downstream (or at least some downstream :) ). However, I can imagine such build could also fail when downstream packages get problems, so it probably shouldn't be a hard requirement for accepting PRs to libnabo/pm... Or you could create dummy test packages which would just try to find libnabo/pm via CMake and via pkgconfig and link some super-simple programs utilizing these libraries (ideally requiring the transitive dependency on libnabo via pm).

@peci1
Copy link
Contributor Author

peci1 commented Oct 3, 2020

Could you also have a look at https://github.com/ethz-asl/laser_slam/tree/master/sensor_drivers/velodyne_assembler ? I get the following error:

/usr/bin/ld: /media/data/subt/mapper/devel/lib/libpointmatcher.a(OctreeGrid.cpp.o): undefined reference to symbol 'pthread_create@@GLIBC_2.2.5'
/lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
tradr-laser-slam/sensor_drivers/velodyne_assembler/CMakeFiles/velodyne_assembler_node.dir/build.make:157: recipe for target '/media/data/subt/ws/devel/lib/velodyne_assembler/velodyne_assembler_node' failed
make[2]: *** [/media/data/subt/ws/devel/lib/velodyne_assembler/velodyne_assembler_node] Error 1
CMakeFiles/Makefile2:32852: recipe for target 'tradr-laser-slam/sensor_drivers/velodyne_assembler/CMakeFiles/velodyne_assembler_node.dir/all' failed
make[1]: *** [tradr-laser-slam/sensor_drivers/velodyne_assembler/CMakeFiles/velodyne_assembler_node.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

@YoshuaNava
Copy link
Contributor

@peci1 The above issue has been pointed out before by other colleagues. The Octree Grid filter might be missing a pthread depend, which can be fixed easily by linking against pthread.

@peci1
Copy link
Contributor Author

peci1 commented Oct 29, 2020

Okay. Is that something you're about to fix in libpm?

@YoshuaNava
Copy link
Contributor

@peci1 I'll take a look during the day and tag you in the PR.

Out of curiosity: what is your build chain?

@peci1
Copy link
Contributor Author

peci1 commented Oct 29, 2020

We use ROS melodic and build ethzasl_icp_mapper with catkin tools.

@YoshuaNava
Copy link
Contributor

YoshuaNava commented Oct 29, 2020

Additional questions:

  • Which version of gcc/clang?
  • Target platform x86?

Pull request: #417

@peci1
Copy link
Contributor Author

peci1 commented Oct 29, 2020

gcc 7.5.0 (I think bionic default), building for amd64 (but soon we'll also need arm64)

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

4 participants