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

Add support for parsing middle module name from type #128

Merged
merged 5 commits into from
Jun 11, 2019
Merged

Add support for parsing middle module name from type #128

merged 5 commits into from
Jun 11, 2019

Conversation

davidhodo
Copy link
Contributor

Allows support for message types generated from both msg and idl files. Attempts to solve #127. I'm making the assumption that all type names are of the form package_name/middle_module (i.e. idl or msg)/type.

Allows support for message types generated from both msg and idl files.

Signed-off-by: David Hodo <[email protected]>
@Karsten1987
Copy link
Collaborator

Thanks for the patch. I think that probably definitely has to be addressed.

However, when running colcon test --packages-select rosbag2 on your branch, I get a lot of test failures. Could you verify that the tests all run successfully on your end?

@Karsten1987
Copy link
Collaborator

I just opened https://github.com/davidhodo/rosbag2/pull/1 to address the concerns of failing tests.

I am not sure however how easily we can backport this to dashing, because as is it's currently breaking API.

@davidhodo
Copy link
Contributor Author

Thanks for those fixes. It works fine for me with your changes. I merged the pull request. Understood on the API change. It would be nice to get it in the next patch release since we're working with some outsides groups that also need the fixes. I'll see if I can figure out a way to do it without changing the API, but I'm not sure it's possible.

@Karsten1987
Copy link
Collaborator

Karsten1987 commented Jun 10, 2019

Running a first round of CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

I think in order to remain API compatible we should maintain the function extract_type_and_package as is, but deprecate this function. Then add a new function e.g. extract_type_identifier (we can come up with a better function name), which extracts the type correctly with a middle module. We can then further decide that the first function calls the new functions and simply discards the middle module in order to avoid code duplication.
Does this make sense?

In order to bring this to a patch release, we should ideally merge this PR today as we ship the first patch release tomorrow.

@davidhodo
Copy link
Contributor Author

It does. I'll take a stab at it shortly.

@davidhodo
Copy link
Contributor Author

See what you think of those changes. On OSX I had to build with colcon build --cmake-args -DCMAKE_CXX_FLAGS="-Wno-error=deprecated-declarations" to prevent the deprecated functions calls in the tests from showing up as errors. I assume we should keep tests in there for the legacy versions until they are removed.

@Karsten1987 Karsten1987 self-assigned this Jun 10, 2019
@Karsten1987
Copy link
Collaborator

Karsten1987 commented Jun 10, 2019

This is great. I like it. I took the liberty to push on your branch a small patch which adds a pragma to suppress the deprecation warning in order to trigger another CI build.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@davidhodo
Copy link
Contributor Author

Not a problem. Thanks for adding that. I wasn't sure the best way to fix it. Also, I noticed you added a default value of msg if there are only two parts. Do you know of a case where that is possible or are you just being cautious? I'm trying to fix a similar issue in ros2cli and am assuming that types always have 3 parts.

@Karsten1987
Copy link
Collaborator

Not sure I completely understood. Whenever trying to access the typesupport only by specifying e.g. test_msgs/BasicTypes the middle element will be empty. In order to get the correct symbol from the library, it makes sense to default to msg then, because before dashing that was the case.

@davidhodo
Copy link
Contributor Author

Got it. Thanks.

@Karsten1987 Karsten1987 merged commit 2ad74a4 into ros2:master Jun 11, 2019
clalancette pushed a commit that referenced this pull request Jun 12, 2019
* Add support for parsing middle module name from type
Allows support for message types generated from both msg and idl files.

Signed-off-by: David Hodo <[email protected]>

* test fixups and default behavior

Signed-off-by: Karsten Knese <[email protected]>

* deprecate legacy type extraction and add new

Signed-off-by: David Hodo <[email protected]>

* use pragma to avoid deprecation in test

Signed-off-by: Karsten Knese <[email protected]>
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.

2 participants