Working Python 2.7 support under Microsoft Windows #2
Conversation
Tested with:
Python 2.7.10 (default, May 23 2015, 09:40:32) [MSC v.1500 32 bit (Intel)] on win32
|
This was enough so I could get logs out of the running watchface/app on my phone. I think pyreadline is OK to include, https://pypi.org/project/pyreadline/ claims to support Python3. https://pypi.org/project/anyreadline/ might be another alternative but I only have experience with pyreadline hence using it. |
|
@Katharine anything else this would need to be considered mergeable? You did the last commit so picked you to ping :-p |
|
@ishotjr do you have time and permission to merge this? There are a few other PRs that would be nice to get consolidated As always thanks to everyone on the Rebble time, you all rock! |
requirements.txt
Outdated
| #libpebble2==0.0.28 | ||
| git+https://github.com/pebble-dev/libpebble2.git@685cd9841e647b48609ce904a87540019390b3be#egg=libpebble2 |
There was a problem hiding this comment.
The libpebble2 on pypi isn't unmaintained - I'd rather use that.
There was a problem hiding this comment.
Ahhh, that's not clear from the pypi page. All the links are to the old pebble project and CI. Thanks for making that clear :-). I was wondering maybe I can open a PR to clean that up, but the readme at https://github.com/pebble-dev/libpebble2/blob/master/README.rst doesn't match the (long) description over at https://pypi.org/project/libpebble2/ - I'll get a bug opened over there but notes (mostly to myself)
https://pypi.org/project/libpebble2/ points to:
- https://github.com/pebble/libpebble2 (multiple places) - instead of https://github.com/pebble-dev/libpebble2
- https://travis-ci.org/pebble/libpebble2 - which also points to https://github.com/pebble/libpebble2
I don't remember why I felt the direct link was necessary, I'm reasonably confident I was ending up with the wrong/old version and it didn't work without it.
There was a problem hiding this comment.
Issues are not enabled on https://github.com/pebble-dev/libpebble2 - https://pypi.org/project/libpebble2/ looks abandoned (readme/description and links are not the current readme in https://github.com/pebble-dev/libpebble2). This may be a pypi issue?
There was a problem hiding this comment.
...
I don't remember why I felt the direct link was necessary, I'm reasonably confident I was ending up with the wrong/old version and it didn't work without it.
lol, I missed the setup.py requirements - that's probably why :-D
| @@ -16,3 +17,4 @@ websocket-client==0.32.0 | |||
| wheel==0.24.0 | |||
| colorama==0.3.3 | |||
| packaging==16.7 | |||
| pyreadline==2.1 | |||
There was a problem hiding this comment.
A quick look suggests that pyreadline has been unmaintained for longer than this tool, and does not work under the latest version of Python. pyreadline3 might be a better choice, though it's very obscure.
Does installing this without importing it do anything?
There was a problem hiding this comment.
I'll need to get back to you on this, been a while since I looked at it.
There was a problem hiding this comment.
A quick look suggests that pyreadline has been unmaintained for longer than this tool, and does not work under the latest version of Python. pyreadline3 might be a better choice, though it's very obscure.
Do you mean https://pypi.org/project/pyreadline3 ? That doesn't look appropriate, it does NOT support Python 2.7 :-(, only some 3.x
Does installing this without importing it do anything?
Are you asking if the install runs some code in the background I'd need to look at the setup.py to know? I'm not 100% sure what you are asking. It's defintely being imported by commands/repl.py
There was a problem hiding this comment.
Supporting Python 2.7 is not really a goal at this point, given that Python 2.7 has been dead for years. That said, it is still necessary given that while this runs (or should run) fine under Python 3, the build chain does not. (For that matter, supporting Windows outside WSL is also a non-goal...)
Is editing requirements.txt even the right thing to be doing? Selectively installing things might be better. What, if anything, happens if this is installed on Linux or macOS?
There was a problem hiding this comment.
Hmmm, maybe that's the way to tackle this. Scratch this PR, and I open a new one that's ONLY a readme change with a note for py2.7 (and Windows) so it's a manual step (or a separate requirements file) so the current known platforms work the way they always have.
Let me try that out real quick.
There was a problem hiding this comment.
It should be possible to selectively install things in setup.py, if one uses that mechanism instead of requirements.txt.
There was a problem hiding this comment.
I'm definitely down for that :-)
I just posted a hopefully easier to accept PR #4 to fix things on all platforms, with Windows to follow. I already tested out requirements windows file and it works great, but a conditional in setup.py would be great too. My main goal is to get something merged so it's in a central place, for a clean working version.
README.md
Outdated
| @@ -0,0 +1,8 @@ | |||
| # Pebble build tool | |||
There was a problem hiding this comment.
It's the Pebble Tool, not the Pebble Build Tool.
There was a problem hiding this comment.
I'll fix that now get it done. If things look good I can squash changes into a single commit.
Thanks for taking the time for review!
Not sure if this is a reasonable answer but building a native binary (pebble.exe) ;-) ... and I don't have to get to grips with WSL ;-p the different versions originally confused me. Seems to have settled down with WSL2. |
This commit is untested.
Match versions in setup.py with (working) requirements.txt. When ran from setup.py httplib2 would pull in pyparsing 3.1.0 rather than 2.4.7. This does not happen with pip install of requirements.txt, manually added pyparsing with pinned version number.
|
@Katharine and @ishotjr I think I'm ready for round 2 :-) Thanks again for you help with this. |
I was writing this when #2 (comment) was posted, hold fire on review. New approach being tested. |
Tested with:
Ready for 2nd review 2023-07-22 (12:11 Pacific time)