Skip to content

bpo-31904: disable os.popen and impacted test cases on VxWorks #21687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Dec 15, 2020
Merged

bpo-31904: disable os.popen and impacted test cases on VxWorks #21687

merged 9 commits into from
Dec 15, 2020

Conversation

pxinwr
Copy link
Contributor

@pxinwr pxinwr commented Jul 30, 2020 •
edited
Loading

VxWorks has no user space shell provided so it can't support os.popen(). Disable it on VxWorks and impacted test cases.

https://bugs.python.org/issue41442

ZackerySpytz
ZackerySpytz reviewed Sep 1, 2020
View reviewed changes
@pxinwr pxinwr force-pushed the fix-issue-31904-shell branch from bcc5ef8 to e37c536 Compare November 24, 2020 08:11
@pxinwr pxinwr changed the title bpo-41442: add unix_shell requirement checking for test_posix.PosixTester.test_getgroups bpo-31904: disable os.popen and impacted test cases on VxWorks Nov 24, 2020
vstinner
vstinner reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Is this change a temporary workaround until subprocess is fixed on VxWorks, or is this skip supposed to be a temporary workaround?

If it's a temporary workaround, I am not sure if it's a good idea to document the skip.

Lib/os.py Outdated
"popen", "extsep"]
"extsep"]

VXWORKS = (sys.platform == 'vxworks')
Copy link
Member

Choose a reason for hiding this comment

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

This variable is only at a single place: you can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

Lib/os.py Outdated
return None
if name == 'nt':
return returncode
if not VXWORKS:
Copy link
Member

Choose a reason for hiding this comment

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

I suggest: if sys.platform != 'vxworks':.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed accordingly.

@pxinwr
Copy link
Contributor Author

pxinwr commented Dec 10, 2020 •
edited
Loading

Is this change a temporary workaround until subprocess is fixed on VxWorks, or is this skip supposed to be a temporary workaround?

If it's a temporary workaround, I am not sure if it's a good idea to document the skip.

os.popen() runs cmd in shell, i.e. shell=True was set when calling subprocess.Popen(). VxWorks has no user space shell provided so that we can't create a new shell and run commands inside. For the subprocess module, VxWorks can't support features needing shell to run. So when I created this PR, I don't intend to be a temporary workaround.

vstinner
vstinner reviewed Dec 14, 2020
View reviewed changes
@vstinner vstinner merged commit e1e3c2d into python:master Dec 15, 2020
@vstinner
Copy link
Member

Merged, thanks.

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@pxinwr @adorilson
bpo-31904: Disable os.popen and popen test cases on VxWorks (pythonGH…
57a349e 
…-21687)
@pxinwr pxinwr deleted the fix-issue-31904-shell branch May 7, 2021 07:42
@kuhlenough kuhlenough mannequin mentioned this pull request Jan 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pxinwr @vstinner @ZackerySpytz @the-knights-who-say-ni @bedevere-bot