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

Use target_compile_defs with exported defs from industrial_core #12

Merged

Conversation

gavanderhoorn
Copy link
Member

Since ros-industrial/industrial_core#262, industrial_core exports its compiler definitions.

abb_driver depends on industrial_robot_client and simple_message, and should use those exported definitions, instead of hard-coding them in its own CMakeLists.txt.

The changes in this PR do that.

Note: this PR will fail CI until industrial_core sees a new release, as the CI configuration installs industrial_core using the released packages. As the most recent release of industrial_core does not include ros-industrial/industrial_core#262, the variable referenced in this PR does not exist and the build will fail.

industrial_robot_client now exports the exact compile definitions used to build the library we use, so use them.
@gavanderhoorn
Copy link
Member Author

Rebased to use CI.

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This seems reasonable enough to me with the recent changes to industrial_core. @Levi-Armstrong , you are listed as the maintainer here, could you take a look and potentially merge? This is currently causing us to not be able to sync Melodic.

@gavanderhoorn
Copy link
Member Author

Actually, we might want to revert ros-industrial/industrial_core#262 in melodic-devel.

We unfortunately released melodic before that change, so we now have breaking builds -- also in packages not under our control.

We'd probably want/need to branch industrial_core and create a noetic-devel.

@clalancette
Copy link

Actually, we might want to revert ros-industrial/industrial_core#262 in melodic-devel.

All right, that's another option. Should I revert ros/rosdistro#30043 for now, and then we can update it as needed later on? That will unblock the Melodic sync that I would like to start getting ready for.

@gavanderhoorn
Copy link
Member Author

If you can wait 5 minutes I'll do a new release of industrial_core.

If not, you could revert.

@clalancette
Copy link

If you can wait 5 minutes I'll do a new release of industrial_core.

Waiting is fine, I just wasn't sure how much work it would be :).

@gavanderhoorn
Copy link
Member Author

I've submitted ros/rosdistro#30228 instead.

@gavanderhoorn
Copy link
Member Author

I'm going to close this here for now, and retarget and re-open it later.

@gavanderhoorn
Copy link
Member Author

I'm actually not going to revert ros-industrial/industrial_core#262. See ros-industrial/industrial_core#274 (comment).

That doesn't change anything wrt the 0.7.2 release. We already reverted that in ros/rosdistro.

@gavanderhoorn gavanderhoorn reopened this Jul 15, 2021
@gavanderhoorn
Copy link
Member Author

Re-opening, as migrating to industrial_core_DEFINITIONS would still be better, even if industrial_core gets some bw-compatibility.

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.

2 participants