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

[rosidl_generator_py] Fixes srv only generation #198

Merged
merged 4 commits into from
Jan 5, 2017

Conversation

mikaelarguedas
Copy link
Member

@mikaelarguedas mikaelarguedas commented Jan 3, 2017

fixes #193
Connects to #193
Reason of the failure: the generated file with all python extension symbols was not added to the library when no .msg files were present
What this PR does:

  • generated the extension files in the package folder rather than the msg subfolder
  • store extension files' paths in a msg agnostic cmake variable and use it for custom command and shared library

CI:
Linux: Build Status
OSX: Build Status
Windows: Build Status

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Jan 3, 2017
@mikaelarguedas mikaelarguedas self-assigned this Jan 3, 2017
@@ -121,7 +121,8 @@ def generate_py(generator_arguments_file, typesupport_impls):
}
data.update(functions)
generated_file = os.path.join(
args['output_dir'], 'msg', generated_filename % args['package_name'])
args['output_dir'], generated_filename % args['package_name'])
print('***generated file: %s' % generated_file)
Copy link
Member

Choose a reason for hiding this comment

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

You might want to remove this debug output.

list_append_unique(_generated_msg_c_files "${_output_path}/${_parent_folder}/_${PROJECT_NAME}_s.ep.${_typesupport_impl}.c")
list_append_unique(_generated_msg_c_ts_${_typesupport_impl}_files "${_output_path}/${_parent_folder}/_${PROJECT_NAME}_s.ep.${_typesupport_impl}.c")
list_append_unique(_generated_extension_files "${_output_path}/_${PROJECT_NAME}_s.ep.${_typesupport_impl}.c")
list_append_unique(_generated_extension_${_typesupport_impl}_files "${_output_path}/_${PROJECT_NAME}_s.ep.${_typesupport_impl}.c")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of repeating the whole value twice (and again twice for services) the variable _generated_extension_files could be updated by appending _generated_extension_${_typesupport_impl}_files at the end of each foreach cycle.

You could even move this out of the loop iterating over the msg / srv files and only set it once if there are any msg or srv files. I think that would be even clearer since these variable only ever contain a single value per typesupport.

if(NOT _generated_msg_c_files STREQUAL "" OR NOT _generated_srv_c_files STREQUAL "")
foreach(_typesupport_impl ${_typesupport_impls})
list_append_unique(_generated_extension_${_typesupport_impl}_files "${_output_path}/_${PROJECT_NAME}_s.ep.${_typesupport_impl}.c")
list_append_unique(_generated_extension_files "${_generated_extension_${_typesupport_impl}_files}")
Copy link
Member

Choose a reason for hiding this comment

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

While using list_append_unique works for both lines I would suggest using list(APPEND instead since there are by design no duplicates ever.

@mikaelarguedas
Copy link
Member Author

ok I think this is ready for review now
reran CI just in case:
http://ci.ros2.org/job/ci_linux/2198/
http://ci.ros2.org/job/ci_windows/2191/

@@ -270,8 +269,8 @@ endforeach()
if(BUILD_TESTING AND rosidl_generate_interfaces_ADD_LINTER_TESTS)
if(
NOT _generated_msg_py_files STREQUAL "" OR
NOT _generated_msg_extension_files STREQUAL "" OR
Copy link
Member

Choose a reason for hiding this comment

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

Where is this variable coming from?

Copy link
Member Author

Choose a reason for hiding this comment

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

bad search/replace apparently 😖

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Looks good to me. I assume you have tried it locally together with ros2/examples#163

@mikaelarguedas
Copy link
Member Author

Yes tried locally using the previously failing service examples

@mikaelarguedas mikaelarguedas merged commit 1e87192 into master Jan 5, 2017
@mikaelarguedas mikaelarguedas deleted the python_extension_fixup branch January 5, 2017 18:35
@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Jan 5, 2017
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.

[rosidl_generator_py] Support packages with only services but no messages
2 participants