8000 Fixes and tests for remappings passed to Node actions by dhood · Pull Request #137 · ros2/launch · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

Sign up for GitHub

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

Jump to bottom

Fixes and tests for remappings passed to Node actions #137

Merged
merged 7 commits into from
Sep 13, 2018
Merged

Fixes and tests for remappings passed to Node actions #137

merged 7 commits into from
Sep 13, 2018

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.

wjwwood
wjwwood approved these changes Sep 13, 2018
View reviewed changes
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 mentioned this pull request Sep 13, 2018
Merged
@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
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees

@dhood dhood

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhood @wjwwood
0