NordicSemiconductor / pc-ble-driver-py

Python bindings for the ble-driver library
Other
126 stars 115 forks source link

python 3 support #27

Closed samstagern closed 5 years ago

samstagern commented 7 years ago

Dear NordicSemiconductor,

Thanks for creating the python ble driver! Is a version for python 3 already planned? Is there already work done on Python 3 and are there known difficulties in porting?

Greetings, Samstagern

n6hpa commented 7 years ago

Please please implement this! I really don't want to have to do this myself

samstagern commented 7 years ago

Before I start porting

Looking through the code it seems reasonably simple to port it, so that it supports both 2.7 and 3.

samstagern commented 7 years ago

There seems to be already ongoing effort at jorgenmk/pc-ble-driver-py. Do you have contact with that developer?

samstagern commented 7 years ago

I have a draft version for preview here: https://github.com/samstagern/pc-ble-driver-py.git. This should support both 2.7 and 3 and still has some raw edges (like CMakeListst.txt) and is untested.

bihanssen commented 7 years ago

Hi @samstagern, sorry for the late reply, and thanks for sharing your code. Our policy has been that since we have internal teams requiring python2, we haven't started adding python3 support. See nrfutil comment.

However, as you have already discovered, @jorgenmk has also been looking into creating py3 version of pc-ble-driver-py. The job with adding py3 support seems to be relatively small. Before potentially releasing this we would need to find a good solution for how to make both versions available in pip.

LJBD commented 7 years ago

@bihanssen AFAIK to make both versions available in pip, you can upload wheels for multiple Python versions (see this). A good, although a bit extreme example of that would be PyMongo which has a ton of wheels built for each version of the package itself.

I'd recommend setting up a Travis job that would build the wheels for Python 2.7 and 3.x.

samstagern commented 7 years ago

@LJBD that seems indeed a good way to go. In the fork I did the code is both python 2 and 3 compatible at source level. It is a bit more effort, but I think it makes sense to keep a single source for this package. If someone can help review and test the fork I will create a pull request.

yoonlee95 commented 6 years ago

Hello, I am also interested in getting pc-ble-driver to work on python3.

If anybody was successful, Can I get some pointers on how to do so? @samstagern

tjstone14 commented 6 years ago

This library is great, but it is getting to be difficult to support python 2 as other libraries are dropping support. I see there is a pull request for python 3 that hasn't yet been merged. Is the plan to merge in the pull request or is there some other activity in the works for python 3?

bihanssen commented 6 years ago

There is still a desire to from Nordic to get support for Python 3 in pc-ble-driver-py. The challenge with the current PR (py3 SD API v5) is that is pulls in many changes at once, and the work of reviewing and completing that job has not been prioritized.

It could be that by focusing on the changes required for Python 3 (that is 2 + 3), and not pulling in SD API-upgrade, it would be easier to get the changes merged.

Python 2+3 support would have to include not just py source code changes, but also a solution for building and bundling binaries (swig compilation) for both py 2 and 3. If someone would do a PR on this (py 2+3), it would most likely speed things up.

dalgr commented 6 years ago

@bihanssen please review https://github.com/NordicSemiconductor/pc-ble-driver-py/pull/50

cooperlees commented 6 years ago

Hi,

What's my best way to run the current test suite?

I've prepared a diff to plumb up the current unit tests to setup.py but I want to ensure Python 2 all passes before I start making changes for Python 2/3 support. Diff: https://github.com/cooperlees/pc-ble-driver-py/commit/15c50a052959395bf52916ab73e81d2f63f7c444

I've been trying after creating a 2.7.15 virtualenv:

/tmp/test_ble_py2/bin/python setup.py test

I get RuntimeError: Connectivity IC identifier __conn_ic_id__ is not set - Full Output: https://pastebin.com/Nx7wQCHs

But in the tests I do see:

config.__conn_ic_id__ = 'NRF52'

If we could get a little tutorial on how to test today, I'll try make the unit tests more self contained so they should just run when you have a virtualenv with all the dependencies installed.

bihanssen commented 5 years ago

For reference: A new release of pc-ble-driver-py with python 3 support is currently in development (currently branch feature/sd132v5_py3). @cooperlees, sorry for late response. If you are still having issues or need assistance please report in a separate ticket.

martijnthe commented 5 years ago

@bihanssen any ETA for when sd132v5_py3 will be completed / released? Thanks much!

bihanssen commented 5 years ago

@martijnthe, thanks for your interest. No ETA, but the release is progressing well and will be out soon.

goldwake commented 5 years ago

This can be closed. v0.12.0 supports python 3

bihanssen commented 5 years ago

@goldwake thanks for the reminder. Closing this issue.