FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-32705: Current Android does not have posix_spawn by yan12125 · Pull Request #5413 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-32705: Current Android does not have posix_spawn - #5413

Merged
vstinner merged 1 commit into
python:masterfrom
yan12125:android-no-posix-spawn
Jan 29, 2018
ConversationCommits (1)ChecksFiles changed
Merged

bpo-32705: Current Android does not have posix_spawn#5413
vstinner merged 1 commit into
python:masterfrom
yan12125:android-no-posix-spawn

Conversation

yan12125 commented Jan 29, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown

yan12125 commented Jan 29, 2018

Copy link
Copy Markdown
Author

I think a NEWS entry is not necessary as this patch is a fix to a new feature landed today (6c6ddf9). From https://devguide.python.org/committing/#what-s-new-and-news-entries:

If a change is a fix (or other adjustment) to an earlier unreleased change and the original news entry remains valid, then no additional entry is needed.

gpshead added the type-bug An unexpected behavior, bug, or error label Jan 29, 2018
gpshead self-assigned this Jan 29, 2018
gpshead reviewed Jan 29, 2018
View reviewed changes
Comment thread Modules/posixmodule.c Outdated

gpshead Jan 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't there a way to check the Android API level at build time? Per your comments on the issue, when running on P (API 28) it should be available.

If not, at least mention that P (API 28) contains the API in a comment here so someone can revisit this in the future.

yan12125 Jan 29, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Technically the macro __ANDROID_API__ can be used, while I prefer to wait until Android P is publicly available so that test_posix_spawn can be run before enabling it.

vstinner Jan 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm fine with always skipping, and "revisit" the code once API 28 is released.

vstinner approved these changes Jan 29, 2018
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

vstinner commented Jan 29, 2018

Copy link
Copy Markdown
Member

A better fix would be to check for posix_spawn() in configure, but it can be done later.

yan12125 commented Jan 29, 2018

Copy link
Copy Markdown
Author

A better fix would be to check for posix_spawn() in configure, but it can be done later

I found there's already one: 6c6ddf9#diff-67e997bcfdac55191033d57a16d1408aR3434. Maybe the HAVE_POSIX_SPAWN define in posixmodule.c is not necessary?

vstinner merged commit 8997f9c into python:master Jan 29, 2018

bedevere-bot commented Jan 29, 2018

Copy link
Copy Markdown

@vstinner: Please replace # with GH- in the commit message next time. Thanks!

vstinner commented Jan 29, 2018

Copy link
Copy Markdown
Member

I found there's already one: 6c6ddf9#diff-67e997bcfdac55191033d57a16d1408aR3434. Maybe the HAVE_POSIX_SPAWN define in posixmodule.c is not necessary?

configure checks for posix_spawn() availability but then posixmodule.c defines "#define HAVE_POSIX_SPAWN 1"? I'm not sure that "#define HAVE_POSIX_SPAWN 1" makes sense. Would you like to propose a patch to remove this #define?

I merged your PR to get a working Python 3.7beta1 on Android. I suggest to wait after the beta1 to revisit (remove) the #define.

pablogsal commented Jan 29, 2018

Copy link
Copy Markdown
Member

@vstinner The removal for "#define HAVE_POSIX_SPAWN 1" is being done in #5418

yan12125 deleted the android-no-posix-spawn branch January 29, 2018 12:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gpshead gpshead approved these changes
vstinner vstinner approved these changes

Assignees

gpshead

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL