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

Fixes and tests for remappings passed to Node actions #137

Merged
merged 7 commits into from
Sep 13, 2018
Merged

Conversation

dhood
Copy link
Member

@dhood dhood commented Sep 13, 2018

Some issues I noticed while working on #135 (kept as separate commits).

Most significant change is that, without this PR, something like:

def generate_launch_description():
    os.environ['TOPIC_NAME'] ='chatter'
    return LaunchDescription([
        launch_ros.actions.Node(
            package='demo_nodes_cpp', node_executable='talker', output='screen',
            remappings=[
                (EnvironmentVariable(name='TOPIC_NAME'), [
                    'new_', EnvironmentVariable(name='TOPIC_NAME')])
            ],
            log_cmd=True,
        ),
    ])

would not remap properly, and would output:

[INFO] [launch]: process[talker-1] details: cmd=[/home/dhood/ros2_ws/install/demo_nodes_cpp/lib/demo_nodes_cpp/talker, <launch.substitutions.environment_variable.EnvironmentVariable object at 0x7f239d09ea20>:=['new_', <launch.substitutions.environment_variable.EnvironmentVariable object at 0x7f239d09e9b0>]], cwd='None', custom_env?=False

whereas now it evaluates the substitutions and gives:

[INFO] [launch]: process[talker-1] details: cmd=[/home/dhood/ros2_ws/install/demo_nodes_cpp/lib/demo_nodes_cpp/talker, chatter:=new_chatter], cwd='None', custom_env?=False

This PR currently targets the branch used for #135 because it builds on top of it; will be retargeted at master once that PR's merged.

@dhood dhood added the bug Something isn't working label Sep 13, 2018
@dhood dhood self-assigned this Sep 13, 2018
@dhood dhood added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 13, 2018
@dhood
Copy link
Member Author

dhood commented Sep 13, 2018

this can be reviewed but there might be duplicate comments from #135 since they have similar approaches; if review comments are added on that PR I will propagate the changes to this one.

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, thanks for fixing that up!

@dhood
Copy link
Member Author

dhood commented Sep 13, 2018

Thanks for the quick review @wjwwood. I added two simple commits since then.

Standard CI (for launch packages):

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

Repeated tests looking for flakiness:

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

@dhood dhood changed the base branch from pass_parameter_files to master September 13, 2018 21:32
@dhood dhood merged commit 8395ddf into master Sep 13, 2018
@dhood dhood deleted the remapping_fixes branch September 13, 2018 21:36
@dhood dhood removed the in review Waiting for review (Kanban column) label Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants