search

ros2 / ros1_bridge

ROS 2 package that provides bidirectional communication between ROS 1 and ROS 2
Apache License 2.0
436 stars 283 forks source link

remove f-strings for dashing support #306

Closed potentialdiffer closed 3 years ago

potentialdiffer commented 3 years ago

This fixes compilation issues on systems with Python versions older than 3.6 by removing f-Strings. https://github.com/ros2/ros1_bridge/issues/305

clalancette commented 3 years ago

CI:

clalancette commented 3 years ago

@potentialdiffer It looks like the build is not happy with the changes. Can you take a look?

fujitatomoya commented 3 years ago

@clalancette

It looks like the build is not happy with the changes. Can you take a look?

isn't that because CI was kicked against focal:rolling? i think this one is supposed to be for bionic:dashing.

potentialdiffer commented 3 years ago

It failed due to a missing symbol. I fixed it. Anyway @fujitatomoya has a point, it should be bionic:dashing.

fujitatomoya commented 3 years ago

I started CI to rebuild https://ci.ros2.org/job/ci_packaging_linux/449/ with bionic:dashing. it tells me Waiting for next available executor on and pending. (build number is 450)

clalancette commented 3 years ago

isn't that because CI was kicked against focal:rolling? i think this one is supposed to be for bionic:dashing.

You are right; I made that mistake, thanks for pointing it out. However, the issues were in Python, so something else is going on. @potentialdiffer you may need a \ at the end of the string line continuations; I'm not sure.

potentialdiffer commented 3 years ago

Can you have a look at the new error? I cannot see see how the new errors are related to the changes I did. https://ci.ros2.org/job/ci_packaging_linux/450/

clalancette commented 3 years ago

Can you have a look at the new error? I cannot see see how the new errors are related to the changes I did. https://ci.ros2.org/job/ci_packaging_linux/450/

No, it's my mistake again; that ros2.repos file is bogus for Dashing. Sorry, I wasn't thinking straight when I ran this yesterday. Here is another try:

clalancette commented 3 years ago

The flake8 errors are unrelated to this PR. Going ahead and merging, thanks for fixing this.