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 parameter library export #448

Merged
merged 2 commits into from
Oct 11, 2022
Merged

Conversation

tylerjw
Copy link
Contributor

@tylerjw tylerjw commented Oct 10, 2022

@fmauch

This PR is directed towards fixing the issue experienced by ur_controllers in this PR: #446

In my local testing the changes in #446 did not cuase the ur_controllers code to complete building, it did get it further than the error experienced in the buildfarm.

Instead of using the parameters.xml to export the transitive dependency of parameter_traits from the parmaeter library this updates the cmake to export the generated pareamters library and export a dependency on generate_parameter_library which exports the dependency on parameter_traits in addition to a handful of other dependencies of the parameters interface.

I tested this by building the following workspace on Rolling.

repositories:
  Universal_Robots_ROS2_Driver:
    type: git
    url: https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver.git
    version: main
  control_msgs:
    type: git
    url: https://github.com/ros-controls/control_msgs.git
    version: humble
  cpp_polyfills:
    type: git
    url: https://github.com/PickNikRobotics/cpp_polyfills.git
    version: main
  generate_parameter_library:
    type: git
    url: https://github.com/PickNikRobotics/generate_parameter_library.git
    version: main
  realtime_tools:
    type: git
    url: https://github.com/ros-controls/realtime_tools.git
    version: master
  ros2_control:
    type: git
    url: https://github.com/ros-controls/ros2_control.git
    version: master
  ros2_controllers:
    type: git
    url: https://github.com/tylerjw/ros2_controllers.git
    version: fix_gpl_export

For reference on the cmake changes for exporting libraries I used this: https://docs.ros.org/en/rolling/How-To-Guides/Ament-CMake-Documentation.html#building-a-library

Recently I went through fixing a bunch of my other library code and had to learn how all this works again.

@fmauch
Copy link
Contributor

fmauch commented Oct 10, 2022

This looks quite sane, but I'm unsure how to actually test that locally, since the problem only occurred on the buildfarm when the JTC was installed as binary package. As the dependencies would have been installed already, if we built ros2_controllers from source on the same machine, this would not be quite the same test.

@tylerjw
Copy link
Contributor Author

tylerjw commented Oct 10, 2022

I was able to build from source locally and that produced the same failure as in the logs in the buildfarm. I built using colcon with this command:

colcon build --mixin ccache rel-with-deb-info lld shared compile-commands

I believe the way colcon builds each package with an isolated build is really similar to how the buildfarm builds debian packages. I could be wrong about this, maybe others know a better way of testing this?

@fmauch
Copy link
Contributor

fmauch commented Oct 10, 2022

I build my workspace locally and it builds fine also without the change introduced here. However, we experienced problems with ccache also in our CI pipeline when migrating the API to the parameter interface, see this commit and this failing pipeline before disabling the cache.

That's why I am rather certain that it should not be a matter of building everything from source, but the problem is actually the dependency simply not being installed, as it was only a build dependency. As this PR adds generate_parameter_library as a full dependency to the JTC package, I am almost 100% certain that this should fix the issue on the buildfarm, as well.

@tylerjw
Copy link
Contributor Author

tylerjw commented Oct 10, 2022

As this PR adds generate_parameter_library as a full dependency to the JTC package

I wanted to do it this way because the library generated has more dependencies than parameter_traits and adding parameter_traits as a dependency only gets the build a little further and it breaks on the next dependency.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Thanks for updating this!

@destogl destogl enabled auto-merge (squash) October 10, 2022 21:29
@destogl
Copy link
Member

destogl commented Oct 10, 2022

@tylerjw It's OK that some builds are failing (unfortunately). We have to get kinematics_interface package merged.

@destogl destogl merged commit eecb7f7 into ros-controls:master Oct 11, 2022
@tylerjw tylerjw deleted the fix_gpl_export branch October 11, 2022 21:45
@bmagyar bmagyar mentioned this pull request Oct 11, 2022
4 tasks
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