Skip to content

Conversation

@bernhardkaindl
Copy link
Collaborator

@bernhardkaindl bernhardkaindl commented Nov 24, 2023

Fix the 1st commit of #17 which replaced commands.getstatusoutput() with
subprocess.getstatusoutput() without taking the change of the status code into account:

python2 -c 'import commands  ; print(  commands.getstatusoutput("false"))'
(256, '')
python3 -c 'import subprocess; print(subprocess.getstatusoutput("false"))'
(1, '')

Unfortunately, the Python3 developers broke the compatibility: The return code changes!!!

With commands.getstatusoutput(), you had to use this to get the actual exit code:

status = os.WEXITSTATUS(status)

These calls have to be removed because now they just shift away the error code:

As shown at benjaminp/six#207, the operation is just status >> 8

Luckily, the status code is checked to against 0 at most places, so there is no change
for these checks. There is only one location where a bit is checked. Fix this location too.

Also, that commit did not take into account that subprocess.get*output do not exist
in Python2, which goes against the directive by Andrew in PR #16 where he requires
that we keep Python2 working:

The current master branch works neither for Python2 nor Python3.

Fix this breakage in the 2nd commit:


Fix the code base to work at least with one Python version(Python2)

After the previous commit updated the code base to work with the exit status
returned by Python3 getstatusoutput(), add a compatible wrapper for
commands.getstatusoutput() to have one Python version functional.

…usoutput()

Fix the 1st commit of #17 which replaced commands.getstatusoutput() with
subprocess.getstatusoutput() without taking the change of the status code into account:

Unfortunately, the Python3 developers broke the compatibility: The return code changes:

python2 -c 'import commands  ; print(  commands.getstatusoutput("false"))'
(256, '')
python3 -c 'import subprocess; print(subprocess.getstatusoutput("false"))'
(1, '')

With commands.getstatusoutput(), you had to use this to get the actual exit code:

status = os.WEXITSTATUS(status)

These calls have to be removed because now they just shift away the error code:

As shown at benjaminp/six#207, the operation is just `status >> 8`

Luckily, the status code is checked to against 0 at most places, so there is no change
for these checks. There is only one location where a bit is checked. Fix this location too.

Also, that commit did not take into account that subprocess.get*output do not exist
in Python2, which goes against the directive by Andrew in PR #16 where he requires
that we keep Python2 working:

The current master branch works neither for Python2, nor Python3 - fix this breakage
in the 2nd commit.

Signed-off-by: Bernhard Kaindl <[email protected]>
Fix the code base to work at least with one Python version(Python2)

After the previous commit updated the code base to work with the exit status
returned by Python3 getstatusoutput(), add a compatible wrapper for
commands.getstatusoutput() to have one Python version functional.

Signed-off-by: Bernhard Kaindl <[email protected]>
@bernhardkaindl bernhardkaindl force-pushed the private/bernhardk/fix-getstatusoutput-and-py2 branch from 42b5980 to d436071 Compare November 24, 2023 17:11
Copy link
Collaborator

@liulinC liulinC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the purpose of py2 compatible code,
But we have agreed master branch is target py3 only, for simple and clean (As both xs8 stream and xs9 have py3).

We are targeting to remove py2 code.
For Yangtze (will be EoL in 2024) which is py2 only, we will branch out for support if necessary.

@bernhardkaindl bernhardkaindl merged commit 67a114b into master Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants