Skip to content
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.

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

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

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

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 changes
Lib/test/test_posix.py Outdated Show resolved Hide resolved
@pxinwr pxinwr force-pushed the fix-issue-31904-shell branch from bcc5ef8 to e37c536 Compare Nov 24, 2020
@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 changes
Copy link
Member

@vstinner vstinner left a comment

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

@vstinner vstinner Dec 8, 2020

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

@pxinwr pxinwr Dec 9, 2020

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

@vstinner vstinner Dec 8, 2020

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

@pxinwr pxinwr Dec 9, 2020

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

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 changes
Lib/test/test_popen.py Outdated Show resolved Hide resolved
Lib/os.py Show resolved Hide resolved
pxinwr and others added 2 commits Dec 15, 2020
@vstinner vstinner merged commit e1e3c2d into python:master Dec 15, 2020
3 checks passed
@vstinner
Copy link
Member

vstinner commented Dec 15, 2020

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner

@ZackerySpytz ZackerySpytz

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@pxinwr @vstinner @ZackerySpytz @the-knights-who-say-ni @bedevere-bot