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
vstinner merged 9 commits into from
Dec 15, 2020
Merged

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

vstinner merged 9 commits into from
Dec 15, 2020

Conversation

@pxinwr
Copy link
Contributor

@pxinwrpxinwr 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
@pxinwrpxinwrforce-pushed the fix-issue-31904-shell branch from bcc5ef8 to e37c536CompareNovember 24, 2020 08:11
@pxinwrpxinwr changed the title bpo-41442: add unix_shell requirement checking for test_posix.PosixTester.test_getgroupsbpo-31904: disable os.popen and impacted test cases on VxWorksNov 24, 2020
vstinner
vstinner reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@vstinnervstinner 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

@vstinnervstinnerDec 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
ContributorAuthor

@pxinwrpxinwrDec 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

@vstinnervstinnerDec 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
ContributorAuthor

@pxinwrpxinwrDec 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
ContributorAuthor

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
@vstinnervstinner merged commit e1e3c2d into python:masterDec 15, 2020
@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)
@pxinwrpxinwr deleted the fix-issue-31904-shell branch May 7, 2021 07:42
@kuhlenoughkuhlenoughmannequin mentioned this pull request Jan 12, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments

+1 more reviewer

@ZackerySpytzZackerySpytzZackerySpytz left review comments

Reviewers whose approvals may not affect merge requirements

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