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

consider LOCATION property if IMPORTED_LOCATION is not set #81

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

dirk-thomas
Copy link
Contributor

No description provided.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Dec 8, 2016
@dirk-thomas dirk-thomas self-assigned this Dec 8, 2016
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm.

Is this somehow related to ros2/rmw_connext#126? I see that you're looping over the different configurations, but it's not clear to me that iterating over _imported_configurations is equivalent to non imported library without a mapping. I mention the configuration mapping here ros2/rmw_connext#126 (comment).

@dirk-thomas
Copy link
Contributor Author

I don't think it is related. I created this only to address parts of ros2/rmw_fastrtps#75.

@dirk-thomas dirk-thomas merged commit 5696340 into master Dec 13, 2016
@dirk-thomas dirk-thomas deleted the consider_location_property branch December 13, 2016 00:57
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Dec 13, 2016
@mikaelarguedas
Copy link
Contributor

This broke the windows builds, the linker tries to link .dll rather than .lib and thus fails (cf http://ci.ros2.org/job/ci_windows/2053/consoleFull search for "fatal error LNK1107")

Job pass once this reverted: Build Status

@esteve
Copy link
Contributor

esteve commented Dec 13, 2016

So, should the approach in PR be improved or should the one in ros2/rmw_fastrtps#75 be merged instead? I have to admit I only tested the latter on OSX and Linux, so I don't know if it'll work on Windows, but it follows the same pattern as rmw_opensplice and rmw_connext, so there's a chance it'll build fine.

@dirk-thomas
Copy link
Contributor Author

@mikaelarguedas Sorry for the problem. And thanks for fixing it.

@esteve We will revisit after the beta. The open ticket on rmw fastrtps should be enough to keep track of this.

@esteve
Copy link
Contributor

esteve commented Dec 13, 2016

@dirk-thomas no problem. I've addressed your feedback in ros2/rmw_fastrtps#75, so I wonder what's blocking it from being merged, the changes don't seem to me to be too invasive.

@dirk-thomas
Copy link
Contributor Author

We are already in code freeze for the beta.

@esteve
Copy link
Contributor

esteve commented Dec 13, 2016

@dirk-thomas sorry if I sounded pushy, thanks for the update :-) Good luck 👍 👍 !

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