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

Regression introduced with 1.11.8 #69

Closed
piyushk opened this issue Mar 6, 2017 · 6 comments
Closed

Regression introduced with 1.11.8 #69

piyushk opened this issue Mar 6, 2017 · 6 comments

Comments

@piyushk
Copy link
Contributor

piyushk commented Mar 6, 2017

@dirk-thomas, @mikaelarguedas

The following snippet was released in action_client.py as part of the latest Kinetic sync, and results in a regression in python based actionlib servers. sub_queue_size defaults to None, which is immediately compared with an integer.

https://github.com/ros/actionlib/blame/indigo-devel/src/actionlib/action_client.py#L531

self.sub_queue_size = rospy.get_param('actionlib_client_sub_queue_size', None)
if self.sub_queue_size < 0:
    self.sub_queue_size = None

Results in:

    client = actionlib.SimpleActionClient('move_base_wrapper', MoveBaseAction)
  File "/opt/ros/kinetic/lib/python2.7/dist-packages/actionlib/simple_action_client.py", line 54, in __init__
    self.action_client = ActionClient(ns, ActionSpec)
  File "/opt/ros/kinetic/lib/python2.7/dist-packages/actionlib/action_client.py", line 532, in __init__
    if self.sub_queue_size < 0:
TypeError: unorderable types: NoneType() < int()
@piyushk
Copy link
Contributor Author

piyushk commented Mar 7, 2017

I just noticed the server suffers from the same issue. The PR has been updated.

@mikaelarguedas
Copy link
Member

Thanks @piyushk for reporting it. This arose because python3 is less permissive than python2 in that matter.

mikaelarguedas pushed a commit that referenced this issue Mar 8, 2017
* fixed default value for rosparam. closes #69

* fixed same issue for action server as well.
@piyushk
Copy link
Contributor Author

piyushk commented Mar 9, 2017

@mikaelarguedas: No problem.

I'm not sure how severe the effect of this issue is though, and whether it would require another Kinetic sync. Maybe @dirk-thomas can comment on it.

@mikaelarguedas
Copy link
Member

I don't think it's as severe as what motivated the previous sync. The main reason being that a big part of the ROS stack (starting as low as ros_base packages) doesn't support python3 (dynamic_reconfigure for example), or started to support it very recently. So I wouldn't expect many people to have deployed systems relying on it.

That being said a new release will be made soon with this fix in it and we can see that the next sync will happen in a timely manner to reduce the impact of this bug.

@dirk-thomas How does that sound to you?

@piyushk
Copy link
Contributor Author

piyushk commented Mar 9, 2017

That seems reasonable. We're being forced to use python3 in some ROS scripts due to other dependencies, but I'm not sure how many other people are.

@dirk-thomas
Copy link
Member

Sounds good to me.

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

No branches or pull requests

3 participants