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

Adding namespaces #195

Closed
wants to merge 1 commit into from
Closed

Adding namespaces #195

wants to merge 1 commit into from

Conversation

richiware
Copy link
Contributor

We removed in Fast-RTPS a using eprosima::fastrtps and a using eprosima::fastrtps::rtps located in a header file. Some users have had problem with this. This PR resolves compilation errors before upload our commit to Github.

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Apr 13, 2018
@mikaelarguedas
Copy link
Member

Thanks @richiware for the headsup.

We removed in Fast-RTPS a using eprosima::fastrtps and a using eprosima::fastrtps::rtps located in a header file. Some users have had problem with this. This PR resolves compilation errors before upload our commit to Github.

Is this commit available somewhere on the Fast-RTPS Github repository for us to test?

This PR currently violates our styleguide and will fail the tests. I have a similar set of commit on this branch that I think adress the same problem and I'd like to test against your namespace removal.

@mikaelarguedas mikaelarguedas added more-information-needed Further information is required requires-changes labels Apr 13, 2018
@richiware
Copy link
Contributor Author

@mikaelarguedas Three months ago a fellow worker made changes in our internal develop branch to solve a problem with namespaces. This week we decided to merge this changes in our internal master branch. I don't want to upload it to Github before be sure not break ROS2.

Today I tested our internal master branch (with the fixing in namespaces) with ROS2 and rmw_fastrtps didn't compile. I was not aware about your changes and decided to upload a PR that solves the problem. But you can deny it.

I've pushed to Github our internal master branch in a new branch for helping you to test.

@mikaelarguedas mikaelarguedas mentioned this pull request Apr 13, 2018
@mikaelarguedas
Copy link
Member

Thanks @richiware for being proactive on this! and for pushing a branch with the changes for us to test 👍. Unfortunately it currently doesnt compile.
I opened #196 with similar changes, I will comment here as soon as #196 is ready to be merged so that you can roll out your changes in Fast-RTPS.

@mikaelarguedas
Copy link
Member

@richiware #196 has been merged so rmw_fastrtps_cpp should work with the new namespace changes.

Thanks!

@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Apr 16, 2018
@richiware
Copy link
Contributor Author

I've updated our master branch.

@mikaelarguedas
Copy link
Member

thanks @richiware for the follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required requires-changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants