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

Export platform build flags to dependent catkin projects #190

Closed

Conversation

jdlangs
Copy link
Member

@jdlangs jdlangs commented Feb 21, 2018

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
Copy link
Member

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

I'll submit a review shortly with some comments.

@gavanderhoorn
Copy link
Member

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 simple_message: possibility for name clashes with defines (INT32, INT64, ..) #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.

# certain functions and headers. The default flags in the file included
# here enable compiling for a ROS node. This file is also exported to
# dependent packages via the `catkin_package` commmand.
include(cmake/platform_build_flags.cmake)
Copy link
Member

Choose a reason for hiding this comment

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

I think documenting this as a subsection in the README might be helpful

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the wiki page would be best but it may be out-of-date (last edit ~3 years ago)

@jdlangs jdlangs force-pushed the export_compiler_flags branch 2 times, most recently from e879f68 to f4a6df9 Compare February 28, 2018 01:44
@jdlangs
Copy link
Member Author

jdlangs commented Feb 28, 2018

@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 jdlangs force-pushed the export_compiler_flags branch 2 times, most recently from a64dde4 to fa731af Compare February 28, 2018 02:22
@jdlangs jdlangs force-pushed the export_compiler_flags branch 2 times, most recently from ae34da8 to 6f49a74 Compare July 2, 2019 16:18
jdlangs added 2 commits July 2, 2019 11:22
Replaced with definitions prefixed with the package name. The old definitions are still present to
avoid breaking any user code that checks them.
@jdlangs jdlangs force-pushed the export_compiler_flags branch from 6f49a74 to 2e3329f Compare July 2, 2019 16:22
@jdlangs
Copy link
Member Author

jdlangs commented Jul 2, 2019

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

@jdlangs
Copy link
Member Author

jdlangs commented Jul 2, 2019

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

@gavanderhoorn gavanderhoorn changed the base branch from kinetic-devel to melodic-devel June 21, 2021 12:55
@gavanderhoorn
Copy link
Member

Replacement/follow-up in #262.

thanks for the initial PR @jdlangs 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants