search

ARMmbed / mbed-os-tools

The tools to test and work with Mbed OS
Apache License 2.0
33 stars 67 forks source link

Use is_alive in favour of isAlive for Python 3.9 compatibility. #248

Closed tirkarthi closed 3 years ago

tirkarthi commented 3 years ago

Description

Use is_alive in favour of isAlive for Python 3.9 compatibility. is_alive is present in both Python 2 and 3. Fixes #247

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

LDong-Arm commented 3 years ago

Hello, is there any chance of getting this merged? It works for Python 3.6 and above. Python 3.5 and below are not actively supported anymore: https://www.python.org/downloads/ and Python 2.x reached the end of support in 2020: https://www.python.org/doc/sunset-python-2/

Patater commented 3 years ago

@LDong-Arm Sorry, no. In this repo, we'd need a solution that still worked for 2.7 because these are our legacy Mbed tools. The new Mbed tools don't have this restriction. As for when we start shipping a Greentea that doesn't support Python 2, we don't know yet, but it'll be from a standalone Greentea repo (not this repo).

LDong-Arm commented 3 years ago

@LDong-Arm Sorry, no. In this repo, we'd need a solution that still worked for 2.7 because these are our legacy Mbed tools. The new Mbed tools don't have this restriction. As for when we start shipping a Greentea that doesn't support Python 2, we don't know yet, but it'll be from a standalone Greentea repo (not this repo).

Fair enough, it makes sense for legacy tools to prioritise backward compatibility.

Patater commented 3 years ago

@LDong-Arm Sorry, no. In this repo, we'd need a solution that still worked for 2.7 because these are our legacy Mbed tools. The new Mbed tools don't have this restriction. As for when we start shipping a Greentea that doesn't support Python 2, we don't know yet, but it'll be from a standalone Greentea repo (not this repo).

Fair enough, it makes sense for legacy tools to prioritise backward compatibility.

If we can add the support here without breaking 2.7 and 3.5, that'd work.

rwalton-arm commented 3 years ago

You could probably check if the attribute exists first.

if hasattr(t, "isAlive"):
    t.isAlive()
else:
    t.is_alive()
LDong-Arm commented 3 years ago

Good suggestion. And we could check the "preferred" (up-to-date) one first which is is_alive.

tirkarthi commented 3 years ago

is_alive is present in both python 2 and 3 so the PR is already backwards compatible. isAlive used to be a deprecated alias to is_alive and it was removed.

harmut01 commented 3 years ago

@tirkarthi, @LDong-Arm what is the status on this PR? I'm still unable to run greentea tests with python 3.9.

LDong-Arm commented 3 years ago

@tirkarthi, @LDong-Arm what is the status on this PR? I'm still unable to run greentea tests with python 3.9.

I locally make the change on my machine, but it'd be really good to have the fix in. Python 3.9 has been available for a while now.

@tirkarthi has a good point - from the Python documentation, even the legacy Python 2.7 and 3.5 should support is_alive(). @Patater @rwalton-arm Is it possible our CI does not have the latest patch releases of both Python versions?

LDong-Arm commented 3 years ago

I think we should try to give this PR a push. Still facing the error every time my python packages are updated.

LDong-Arm commented 3 years ago

Could do it in https://github.com/ARMmbed/greentea once that's available for use

rwalton-arm commented 3 years ago

I strongly suggest we merge this PR. We need to continue supporting these tools and they are currently completely broken on python 3.9>.

One of the reasons CI was failing for python 3.5 was because of an issue with "prettytable" (since fixed, in fact we dropped 3.5 from the CI matrix completely in https://github.com/ARMmbed/mbed-os-tools/commit/3c6400e29388b5a884dfbeaa7df354abec75c020).

@tirkarthi Sorry for the extremely long delay in reviewing this PR, but could you please rebase on the latest master so we can re-run the CI (and finally get this fix merged). Thanks!

tirkarthi commented 3 years ago

@rwalton-arm Sure, updated my PR with master.

tirkarthi commented 3 years ago

@rwalton-arm Ok, I just used github U for updating branch which uses merge. I reverted my merge and did a rebase now.