-
Notifications
You must be signed in to change notification settings - Fork 0
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
Switch to python 3 #583
Switch to python 3 #583
Conversation
I tried this out, and it worked! I ended up having two problems:
Otherwise, I ran all of the demos and they all seemed to work pretty well. This is awesome @basicNew ! |
@clalancette thanks for checking this so quickly! Indeed, I forgot to mention I nuked |
3bf330b
to
1d0a8db
Compare
Alright, CI is passing! @hidmic would you mind testing this one (check @clalancette 's comments)? If everything goes ok I think we could merge this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of small nits in the code that is here.
However, I think we need to change a bit more of the CI code. In particular I think we need to remove https://github.com/ToyotaResearchInstitute/delphyne-gui/blob/master/tools/install_prereqs.sh#L43, change https://github.com/ToyotaResearchInstitute/delphyne-gui/blob/master/tools/install_prereqs.sh#L80 to python3, etc. Also, I'm surprised that we didn't have to change the name of the tarball that we fetch: https://github.com/ToyotaResearchInstitute/delphyne/blob/master/tools/continuous_integration/jenkins/build#L20 ; is python3 bindings automatically included in the "default" nightly tarballs now?
python/delphyne/launcher.py
Outdated
@@ -62,12 +62,14 @@ def launch(self, command, label=None, cwd=None, environ=None): | |||
stdin=self.devnull, | |||
stdout=subprocess.PIPE, | |||
stderr=subprocess.STDOUT, | |||
universal_newlines=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about adding this, but I'm surprised you needed it, given that we only support Linux at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, I was surprised require this too, but otherwise the characters used from ignition logging (spaces and colors) were all escaped and output was a mess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Good reason to use it then; I'll just suggest that you put a comment in there saying that we need it for that reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments added!
@@ -38,7 +38,7 @@ libignition-cmake-dev | |||
mercurial | |||
pkg-config | |||
pycodestyle | |||
pylint | |||
pylint3 | |||
python-pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that you didn't have to change this to python3-pip
suggests that we don't actually need pip anymore; I'd suggest removing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I'll take a shot at it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working fine on my end, LGTM pending @clalancette comments' resolution!
1d0a8db
to
78022f7
Compare
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Done!
AFAIK nightly include python3 support, see relevant comment here. |
Let's see what CI has to say. |
@clalancette @basicNew looks like CI is happy :) |
This requires latest Drake, so goes hand-in-hand with maliput/delphyne_gui#179
Steps to migrate:
--config=python3
flag):$PYTHONPATH
points to a 3.6 install and not a 2.7All the demos should run and the tests should pass.
@clalancette @hidmic haven't yet checked on CI, wanted to push this as soon as I got it so you could confirm this is working locally too