nfcpy / nfcpy

A Python module to read/write NFC tags or communicate with another NFC device.
European Union Public License 1.1
576 stars 146 forks source link

Support Python 3 #47

Closed PanderMusubi closed 5 years ago

PanderMusubi commented 8 years ago

Please, support also Python 3.

rejhgadellaa commented 8 years ago

+1

nehpetsde commented 8 years ago

It's planned.

PanderMusubi commented 8 years ago

Great! You can contact me to do some testing.

PanderMusubi commented 7 years ago

@nehpetsde, do you think September is a good month for planning this? :wink:

Skylled commented 7 years ago

To phrase it differently, what can the community do to help move this project forward? Are there any major challenges to consider?

PanderMusubi commented 7 years ago

Perhaps some PRs are also good to do, see https://github.com/nfcpy/nfcpy/network

svvitale commented 7 years ago

I'd also be happy to help with the Python 3 changes. Is there a list of what needs to be done?

henrycjc commented 7 years ago

Hi there, I would like to put my hand up to help out in anyway with Py3 support. If there is no plan of action I will be forking this evening.

svvitale commented 7 years ago

I'm hoping it won't come to forking. @nehpetsde have you started a python 3 branch? Perhaps some of the interested helpers could collaborate on that branch so we're not duplicating work?

henrycjc commented 7 years ago

@svvitale @nehpetsde I would gladly put my hand up to help with Py3 support.

nehpetsde commented 7 years ago

Thanks for all the offers to help with Python 3 compatibility, it's pretty much appreciated. Actually the transition to Py3 is already started but the way I plan to go is through (I) move things out of nfcpy that don't necessarily belong there and (ii) gradually adapt smaller parts through normal releases. Under the first category fall NDEF message decoding and encoding (which will be replaced by nfcpy/ndeflib) and some tools that have grown to much in the examples section. The main reason for (ii) is that testing is by far the hardest part and can't be much automated. Any move to a separate branch for Py3 would lead to larger maintenance problems and I think the world is not yet ready for Py3 only.

Some things that would probably help to move faster:

henrycjc commented 7 years ago

@nehpetsde Thank you very much for getting in touch. I will endeavour to address your dot points in whatever way I can over the coming weeks.

svvitale commented 7 years ago

Thanks @nehpetsde, this is very helpful. I think there are a number of us that can start helping with this vision. I agree that maintaining APIs and compatibility with Python 2 are of the utmost importance.

For the testing challenges, perhaps we can set up individual mocks for each supported hardware type. I can experiment with this approach using the PN532 readers I have. From there, hopefully the approach can be extended to additional hardware.

nehpetsde commented 7 years ago

For the testing part, it's maybe a good start to gather information of who has what hardware (reader/tags/phones) and would be able to test regularly. A Wiki usually appears to be good place for such information. So there's now an - initially empty - page at https://github.com/nfcpy/nfcpy/wiki and it's freely editable (hopefully there isn't much trolling on GitHub).

PanderMusubi commented 7 years ago

I added my device and made a start to build inventory for testing. Change it as you see fit. Preferable list all the device you have so multiple people can test those same devices.

svvitale commented 7 years ago

I've started looking into the mock approach. @nehpetsde what's the current status of the test suite? If I install nose and run the tests, should I expect them to all pass? I have an ACR122U hooked up by USB currently.

nehpetsde commented 7 years ago

The tests are not passing for now. I want to move them to py.test and will have a first file (llcp pdu test) ready tomorrow or a day later.

If you want to have a look into tag or reader parts that'd be great. I actually hope there's some better mockup possibilities with py.test.

nehpetsde commented 7 years ago

I have completed conversion of all previously existing test cases to pytest and mock to a test coverage of now 37 percent. Coverage for Type 1/2/3 Tag is almost 100% which means the implementation code can be tested with Python 3 and modified to pass. I've made the imports working for Py3 but most of the tests still fail. I will not work on that part further but focus on test coverage for other units. If anyone feels encouraged to make the existing tests pass with Py3, don't hesitate to step up.

nehpetsde commented 7 years ago

A short update: Test coverage has now progressed to about 57%, there's still some work to. Along with the tests I've sometimes fixed obvious things like to use the byte literal for byte strings and Python 3 compatible import statements. Some parts (like SnepServer) do import the (obsolete) nfc.ndef submodule and must be temporarily removed from import for testing with Python 3.

As said before, I will continue work on test coverage before ever turning to Python 3.

PanderMusubi commented 6 years ago

@nehpetsde Do you have an update on the progress? As you can see, many of us appreciate your work. Think many of us are wondering how it is going at the moment (and give you more thumbs up ;-) ).

nehpetsde commented 6 years ago

So here is another update: As of July 21st code coverage on master reached 94% and hit the point where API changes became necessary. Since then all commits go to a develop branch where the old ndef library got removed, the Snep client/server adapted and the Handover client/server is underway. The handover submodule is the last missing piece for the core stack before getting to the examples. As a further note, I'm planning the develop branch to finally become nfcpy version 1.0.

Kelladros commented 6 years ago

Update on this? Also anyone know where I could get some help getting this running, fairly new to coding and can't figure out what I'm doing wrong. Thanks.

puhitaku commented 6 years ago

+1

PanderMusubi commented 6 years ago

Looking forward to full and official support for Python 3.

PanderMusubi commented 5 years ago

Any progress with this?

miili commented 5 years ago

It's time, the clock is ticking!

blacklight commented 5 years ago

Any progress/ETA with the migration to Python 3, or is there any way we can help?

robert-alfaro commented 5 years ago

Another helping hand here...

Any status update @nehpetsde? Not saying it's easy, but it's been two, mostly silent, years.

Our team uses python3 for everything except NFC because python2 req. We're going to start to migrate NFC to python3 because it solves many problems for us, especially maintaining code compatibility for heavy bytes operations and asyncio.

blacklight commented 5 years ago

Poking again, not sure if any developer is listening to the requests on this thread.

Could anyone at least provide an ETA? Or if there's no intention to migrate to Python 3 at least state it clearly so someone can fork this project and do the migration? Being negligent against the requests of the community for such a long time doesn't help this project and doesn't help open source in general.

I've started a couple of days ago to test nfcpy against Python 3 and fix what breaks whenever it comes, and I understand that it's not a trivial task; there's plenty of implicit conversions from string to binary that won't work in Python 3. But if nobody starts fixing it then the project will never work. And before doing any further work on the migration I'd like to know if anyone else is also working on this (if so, we need to synchronize so we won't duplicate work) or if nobody else is doing it (if so, I'll fork nfcpy and work on the migration myself).

henrycjc commented 5 years ago

@BlackLight I won't presume to speak for @nehpetsde but check my PR into master https://github.com/nfcpy/nfcpy/pull/114/

I can't actually run the tests because I develop on Windows and termios support is a hassle I haven't worked around (using something like WSL etc.). This Christmas break I was intending on pulling master into develop (thus applying the py3 compat fixes there) and working through the remaining compatibility issues.

Stephen is quick to respond to PRs so feel free to beat me to the mark - at this point I've changed jobs so my interest in NFC is purely for fun.

Anyway, once I've got some stable py3 changes going in develop, I'm sure we can prompt to do a compatibility release :)

blacklight commented 5 years ago

@henrycjc thanks for your reply! I've indeed started building on top of your merge. You've fixed many compatibility issues with Python 3 but there's still a lot of work to do around the buffers managements - there's plenty of .decode() and .encode() to add to ensure that binary data is handled properly. In Python 2 you could get away with chr() on things get could be either strings or binary buffers, but Python 3 is stricter when it comes to distinction between (unicode) strings and (binary) byte arrays.

I've started patching the files but I just wanted to know if anyone else was working on it, so that we don't duplicate what's indeed a lot of work.

And it'd also be helpful if someone could test the new code on different readers. I've got an ACR122U NFC scanner but it'd really be appreciated if users/contributors that use other drivers could test the changes as well.

robert-alfaro commented 5 years ago

@BlackLight I'll be glad to test. I have an ACR122U (uses PN532) and a dongle (forgot the model) that uses PN533.

D37R4C7 commented 5 years ago

Hey guys, what's the current status on this issue? @BlackLight, @henrycjc are you planning a python 3 fork? I would be glad to test, too.

blacklight commented 5 years ago

@D37R4C7 not yet: I had started working on it last month, but there were so many things breaking wherever the code treats strings and bytes as the same type that it took me a lot to get it working with my ACR122U, only through a lot of workarounds and only fixing the piece of calls stack that I could see.

Migrating all the available drivers plus the core codebase is quite some work to do, and before adventuring into it I'd really like to know from the maintainers of this repo:

Until I get these pieces of information from anyone maintaining nfcpy, I don't feel like going forward.

However, I'm also aware that the "Python 3 support is on its way" statement was made nearly 3 years ago, and nothing has happened since then. So I'd also like to give maintainers a heads up: either you update us on what's your plan with Python 3 within a couple of weeks, or I'll just proceed, fork nfcpy and work on an independent Python 3 version.

readysteadywhoa commented 5 years ago

Python 2.7 EOL is 8 months away. It appears that the maintainer has moved onto other things. @BlackLight are you still interested in forking this project?

mofe23 commented 5 years ago

I hacked a night long a few weeks ago to port some functionality to python3. see my fork here: https://github.com/mofe23/nfcpy/tree/feature/py3 I focused on the areas i need myself, which are tag type 2 tags and a sony rs380 reader, so expect that to work - but also ported some other stuff.

I did not set up testing for both python2 and 3 as i have never done this before. IMO the next major todo would be to implement and document testing both python2 and 3

The creator did a gread job on writing unittests, so porting was quite fun work :)

To try if my fork works for your setup do: To use f-strings, but still compatible to 2.7 i used the future-fstrings package, so be shure to run pip install future-fstrings before installing my fork using pip install git+https://github.com/mofe23/nfcpy.git@feature/py3

RafalSkolasinski commented 5 years ago

Does anyone have successfully run nfcpy on Python 3 with ACR122U for reading / witting? Happy to test!

mofe23 commented 5 years ago

@nehpetsde: First: It has been absolutely painless for me to get it up and running on both osx and windows and it was fun to dig into the code. Thanks for that awesome code, docs and tests.

Today i started to configure tox to run test on both python2 and 3 and im confident that im able to finish the test setup and porting the project to python3, but i have some questions for you before i continue:

  1. What minimum version of python3 should i target?
  2. Are you ok to add the external dependency on future_fstrings?
  3. Sphinx dropped support for python2 in v2.0, we need to pin it to <=v1.85 to build docs on python2.7 or build the docs in a python3 env (would prefer that)
blacklight commented 5 years ago

@mofe23 first of all thanks for your work. I had started a while ago migrating the library for ACR122U but eventually gave up due to lack of momentum and interest from the maintainers. So far my solution is to wrap the nfcpy functionalities in a Python 2 script that communicates over messages infrastructure with another Python 3 project.

My two cents on your questions:

  1. The minimum Python 3 version I've seen installed on most of the systems I've been working on lately is 3.5. It's still the one installed by default on Raspbian Jessie, which was released almost 4 years ago. I think it's a reasonable bottom line for the required Python 3 version. I know that some MacOS installations still ship Python 3.4 by default, maybe it's also worth to include it unless it's too much of a pain.

  2. Do we really need the future_fstrings dependency? Even though I love the format string features introduced in Python 3.6, I also recognize that it's mostly a (developer-side) cosmetic feature. I personally wouldn't add a dependency only to make a developer's life slightly easier when it's still possible to offer the same functionality using the built-in formatting features.

  3. As more and more projects are dropping Python 2 support, I'd stick to the newer versions, even if it requires to run some portions of the code in a Python 3 env: sticking to old and unmaintained dependency versions so we can stick to our old and unmaintained versions too is never ever a good decision IMHO.

nehpetsde commented 5 years ago

@mofe23 Many thanks for your help. To the questions:

  1. Ubuntu 18.04 ships with Python 3.6, so that seems like an appropriate minimum version.
  2. Thanks for pointing out the future_fstrings module. Looks well done and f-strings are cool - so yes.
  3. If possible, I'd also prefer to have the docs built with Python3. Could be a separate PR if that makes it easier to progress.

As a notice, just recently @msnoigrs submitted PR #127 but now closed it to fix some tests that failed.

blacklight commented 5 years ago

@nehpetsde I'd suggest to include Python 3.5 as well please: being the version installed on Jessie it's still the default Python version on most of the Raspberry Pis and clones out there.

nehpetsde commented 5 years ago

@BlackLight Thanks for reminding of Raspberry Pi support. That's a clear go for Python 3.5 as the minimum version.

Regarding f-strings, they are a perfect candidate for something that can be deferred for a later time, regardless of their usefulness. They are not used in the current code base and wouldn't need to be introduced for the purpose of porting to Python3.

mofe23 commented 5 years ago

Ok, i removed future_fstrings and will target python3.5.

mofe23 commented 5 years ago

Spent another sunday yesterday and i guess i finished the python3 porting. Tox & travis are set up, docs build, tests/doctests pass and flake is happy.

Furthermore i fixed a lot of depreciation warnings and some small stuff (like comparing booleans with ==)

@nehpetsde @BlackLight @readysteadywhoa @RafalSkolasinski @D37R4C7: Are you guys still interested in testing? Try #130

nehpetsde commented 5 years ago

@mofe23 Many thanks for the PR. I'll try to check this week.

RafalSkolasinski commented 5 years ago

@mofe23 I just tried your branch feature/py3. It seems to work fine! I only had to make small adjustment in my code but it was due to Python 3 vs Python3:

I had to change

tag_id = tag.identifier.encode("hex").upper()

into

tag_id = tag.identifier.hex().upper()

And I successfully installed your fork with just

pip install git+https://github.com/mofe23/nfcpy.git@feature/py3
mofe23 commented 5 years ago

@RafalSkolasinski Thank you for checking, seems like i overlooked some .encode("hex") lines, mostly in logging calls, gonna fix them soon.

Somehow i dont find the specific loc you mentioned, could you give me a hint where it is?

RafalSkolasinski commented 5 years ago

@mofe23 Sorry, I meant in my own code that uses nfcpy ;-) It was more a general comment about adjusting nfcpy-based codes to Python 3 as these .encode('hex') I think I took from the tutorial / example.

nehpetsde commented 5 years ago

@mofe23 I've pulled your PR to branch mofe23-feature/py3 for testing and further updates. Unfortunately it is not yet passing Python 3.6 and 3.7 targets (I've added them to .travis.yaml configuration). I'll merge as soon as I get the tests pass (they fail on some exotic things like PEP 479 becoming default in Python 3.7).

mofe23 commented 5 years ago

@nehpetsde Hey, thanks for checking!

I will have a look at the tests for the newer versions this weekend, i saw a warning by pytest that StopIteration will soon be depreciated but did not notice it already happened in 3.7.

A question: Whats the current state of ndeflib/nfc.ndef? is nfcpy ready to remove the ndef stuff and depend on ndeflib?