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

Replace trollius by asyncio #574

Merged
merged 2 commits into from
Oct 31, 2019
Merged

Replace trollius by asyncio #574

merged 2 commits into from
Oct 31, 2019

Conversation

timonegk
Copy link
Member

@timonegk timonegk commented Oct 23, 2019

Since trollius is a python2 package that is not necessary for python3 because the functionality has been replaced by the python asyncio module, this pull request replaces trollius by asyncio. It closes #520 and would close #558 because the dependency to trollius could be dropped.
To be python3 compatible, #565 should also be merged.

Obviously, this pull request would drop python2 support, which is currently probably not realistic. Would it be possible to merge this to a separate branch to release a different version for python3?

@v4hn
Copy link
Contributor

v4hn commented Oct 24, 2019

CI failed as your patch is of course not python2 compatible.
Any chance of getting this implemented in a version-agnostic way, factorizing the implementation?

@timonegk
Copy link
Member Author

A version-agnostic implementation unfortunately does not seem to be possible because the new from() function is invalid syntax in python2. But there there might be a way of circumventing this function that I am not aware of.

@v4hn
Copy link
Contributor

v4hn commented Oct 24, 2019

@mikepurvis How do you want to handle this version-incompatibility here then?
Any chance of having a separate python3 branch?

@mikepurvis
Copy link
Member

mikepurvis commented Oct 24, 2019

Thanks for doing this work! Looks like the changes are pretty straight forward.

Per @wjwwood's comment on #558 (comment), I'd be fine to just merge this and release it as 0.5.0, supporting Python 3.5+ and Ubuntu 16.04+ only. Trusty is five years old at this point, and ROS Indigo was EOL in April 2019.

At the end of the day, catkin_tools is a CLI utility, not a library, so short of weirdness with ROS workspace PYTHONPATH values, it shouldn't really impact people that much to have the tool suddenly be running under Python 3 rather than 2 (and if the PYTHONPATH does turn out to be a problem, we could consider adding logic to the entry_point that sanitizes the sys.path before importing anything external).

@v4hn
Copy link
Contributor

v4hn commented Oct 25, 2019

@mikepurvis Sounds wonderful!

Could you patch CI to remove the python2.7 checks then so CI succeeds here and this gets merged?

@mikepurvis
Copy link
Member

You should be able to make the CI updates part of this change! Just update the travis config here:

https://github.com/catkin/catkin_tools/blob/master/.travis.yml

And trollius should be dropped as a dep from the requirements.txt and setup.py files as well.

@wolfv
Copy link

wolfv commented Oct 26, 2019

This would be great, as this would make catkin_tools work with newer Python releases in the conda package manager! I just realized after I published the package that trollius and asyncio are not exactly drop-in replaceable.

@wolfv
Copy link

wolfv commented Oct 26, 2019

I wonder if you could use exec(...) or eval(...) to work around the missing keyword problem.

@mikepurvis
Copy link
Member

Given that the target OS will be 16.04+, it probably makes sense to have it testing against Python 3.5, but either way this is fine for now. Thanks for the contribution!

Going to merge and then myself and others can use it from source for a bit before recommending a new release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants