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

Skip parsing argv[0] for remap rules #249

Closed
wants to merge 1 commit into from
Closed

Conversation

dhood
Copy link
Member

@dhood dhood commented Jun 2, 2018

I turned on debug logging investigating #248 and was surprised that argv[0] was being parsed.

$ ros2 run demo_nodes_cpp talker
[WARN] [rcl]: arg 0 (/home/dhood/ros2_ws/install_isolated/demo_nodes_cpp/lib/demo_nodes_cpp/talker) error 'Expected lexeme type (19) not found, at /home/dhood/ros2_ws/src/ros2/rcl/rcl/src/rcl/lexer_lookahead.c:227'
[INFO] [talker]: Publishing: 'Hello World: 1'

(error message format using changes in #248)

Is it safe to assume that argv[0] is never going to have remapping arguments? (I am not 100% sure)

@dhood dhood added the in review Waiting for review (Kanban column) label Jun 2, 2018
@dhood dhood self-assigned this Jun 2, 2018
@dhood
Copy link
Member Author

dhood commented Jun 4, 2018

I've found that there are times when this assumption (that argv[0] won't have remapping arguments) doesn't hold: when they are being passed in manually.

E.g. this rclpy test:

self = <test.test_node.TestCreateNode testMethod=test_node_arguments>

    def test_node_arguments(self):
        rclpy.init()
        try:
            node = rclpy.create_node('my_node', namespace='/my_ns', cli_args=['__ns:=/foo/bar'])
>           self.assertEqual('/foo/bar', node.get_namespace())
E           AssertionError: '/foo/bar' != '/my_ns'
E           - /foo/bar
E           + /my_ns

That is a test, yes (and there are a number of other tests relying on the existing behaviour), but I think it's reasonable usage that someone else might want to do.

Unless we think that manual passing of cli_args should always include the process name as the first in the list, I will close this PR.

@dhood dhood closed this Jun 4, 2018
@dhood dhood deleted the dont_parse_argv0 branch June 4, 2018 21:39
@dhood dhood removed the in review Waiting for review (Kanban column) label Jun 4, 2018
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