search

ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
156 stars 181 forks source link

Export platform build flags to dependent catkin projects #190

Closed jdlangs closed 3 years ago

jdlangs commented 6 years ago

This fixes the problem from #188 by using the CFG_EXTRAS option of the catkin_package command to export default compiler definitions to all projects which depend on simple_message. It also addresses a few of the names mentioned in #65 by adding a SIMPLE_MESSAGE_ prefix to the definitions.

The "documentation" of these definitions for package users is right now only in CMakeLists.txt and the new platform_build_flags.cmake file. Let me know if there is anywhere else it would be appropriate to note this behavior.

gavanderhoorn commented 6 years ago

Thanks for the PR, this looks like a useful change.

I'll submit a review shortly with some comments.

gavanderhoorn commented 6 years ago

As it's just a few questions/comments really, let's not start a review yet.

  1. it seems your commit isn't attributed to your account, you may want to fix that
  2. did you remove the CFG_EXTRAS entry from industrial_robot_client on purpose? Is it no longer needed?
  3. addressing #65 is nice, but we have to be careful with this change so as to not introduce any breaking changes in Kinetic (which is an LTS). Could I ask you to refactor the changes that introduce the prefix into a separate commit? We should then also keep the old defines in Kinetic - and provide the new ones as well - add a deprecation notice and then in Lunar we can remove the old ones.
jdlangs commented 6 years ago

@gavanderhoorn sorry it took me a little while to come back to this. Responding to your comments:

  1. Fixed
  2. It does not appear to be needed. I tested building against the industrial_robot_client library and it only breaks if I remove the issue46_workaround file from the CFG_EXTRAS in simple_message.
  3. I've put the definition renaming in a separate commit. Is there a better deprecation strategy than just continuing to provide the old defines with a note to remove them later? I couldn't see a way to keep them and emit a warning in the user's code.
jdlangs commented 5 years ago

Refreshed this PR for ROS-I day. This is unchanged from last year but I believe everything brought up had been addressed.

jdlangs commented 5 years ago

@jrgnicho would you want to take another look since you commented the first time around?

gavanderhoorn commented 3 years ago

Replacement/follow-up in #262.

thanks for the initial PR @jdlangs :+1: