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

Don't COLCON_IGNORE fastcdr, we want to use it for rosbag2 type conversion #534

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

emersonknapp
Copy link
Contributor

@nuclearsandwich let me know what you think about this, we currently have a dependency from rosbag2 converters on rmw_fastrtps_cpp, but only need the CDR implementation and are looking to pull a dependency on it directly. Noticed that in the packaging builds it is ignored if "USE_FASTRTPS_*" is not checked. We're thinking this library should be usable as a dependency even if the middleware itself is not being used.

…ndency for rosbag2 type conversion

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Copy link
Contributor

@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.

That makes sense to me, but I'll let @nuclearsandwich review before merging.

Copy link
Contributor

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

nice

@clalancette
Copy link
Contributor

@emersonknapp Can you please run a CI with this change in place, just to ensure that nominal things still work?

@emersonknapp
Copy link
Contributor Author

emersonknapp commented Jan 15, 2021

@emersonknapp Can you please run a CI with this change in place, just to ensure that nominal things still work?

Given that I do not have write access to this repo and can't push my feature branch to it, I'm not sure that I am able to do so. My understanding is that CI_SCRIPTS_BRANCH parameter is used on builds to achieve this, but that the branch has to be present on this repository, not a fork.

@nuclearsandwich
Copy link
Member

@emersonknapp I pushed your branch to ros2/ci directly so it can be used with CI_SCRIPTS_BRANCH. The branch name is unchanged (emersonknapp/no-ignore-cdr)

@emersonknapp
Copy link
Contributor Author

Thanks! Anything I should run beyond:

  • ci_packaging_linux: Build Status

?

@clalancette
Copy link
Contributor

I guess the other thing test to run is when both Fast-RTPS static and Fast-RTPS dynamic are unchecked. I don't expect problems, really (we're just adding a new package in that case), but we may as well check it.

@emersonknapp
Copy link
Contributor Author

Of course, makes sense

  • Build Status

@clalancette clalancette merged commit 6125ec9 into ros2:master Jan 15, 2021
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.

4 participants