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 generate IDL files and remove unused code #2

Merged
merged 2 commits into from
Aug 9, 2018

Conversation

mikaelarguedas
Copy link
Member

This PR removes what I think is a leftover from copying an existing typesupport that rely on the generation of IDL files and invocation of third party generators.

@MiguelCompany Would you mind having a look and letting us know if this code is necessary ?

First attempt at CI:

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

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Aug 7, 2018
@mikaelarguedas
Copy link
Member Author

New round of CI with the dependency removed:

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

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 8, 2018
Copy link
Member

@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

@mikaelarguedas
Copy link
Member Author

Merging this as it doesnt look like it's used and has been approved.
@MiguelCompany feel free to provide feedback post-merge and we can make a follow-up PR if necessary

@mikaelarguedas mikaelarguedas merged commit 0d58cf8 into master Aug 9, 2018
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Aug 9, 2018
@mikaelarguedas mikaelarguedas deleted the remove_unused_code branch August 9, 2018 22:45
get_filename_component(_name "${_idl_file}" NAME_WE)
set(_abs_idl_file "${${_pkg_name}_DIR}/../${_parent_folder}/dds_fastrtps/${_name}_.idl")
normalize_path(_abs_idl_file "${_abs_idl_file}")
list(APPEND _dependency_files "${_abs_idl_file}")
Copy link
Member

Choose a reason for hiding this comment

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

The removal of this line change breaks the automatic regeneration of the FastRTPS typesupport files when a dependent message file is changed. A following build doesn't re-build the generated code which leaves the library with outdated content.

There is another bug in the CMake which also doesn't re-expand the templates when message files in the processed package fail. I am not sure if that was working before this patch though.

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.

3 participants