Skip to content

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

Merged
vstinner merged 1 commit intopython:masterfrom
yan12125:android-no-posix-spawn
Jan 29, 2018
Merged

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

Conversation

@yan12125
Copy link
Copy Markdown
Contributor

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

@yan12125
Copy link
Copy Markdown
Contributor Author

yan12125 commented Jan 29, 2018

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 gpshead added the type-bug An unexpected behavior, bug, or error label Jan 29, 2018
@gpshead gpshead self-assigned this Jan 29, 2018
gpshead
gpshead reviewed Jan 29, 2018
View reviewed changes
Comment thread Modules/posixmodule.c Outdated
Copy link
Copy Markdown
Member

@gpshead gpshead Jan 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@yan12125 yan12125 Jan 29, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@vstinner vstinner Jan 29, 2018

Choose a reason for hiding this comment

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

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

vstinner
vstinner approved these changes Jan 29, 2018
View reviewed changes
Copy link
Copy Markdown
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.

LGTM.

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Jan 29, 2018

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

@yan12125
Copy link
Copy Markdown
Contributor Author

yan12125 commented Jan 29, 2018

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 vstinner merged commit 8997f9c into python:master Jan 29, 2018
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jan 29, 2018

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

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Jan 29, 2018

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
Copy link
Copy Markdown
Member

pablogsal commented Jan 29, 2018

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

@yan12125 yan12125 deleted the android-no-posix-spawn branch January 29, 2018 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes

@vstinner vstinner vstinner approved these changes

Assignees

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

@yan12125 @vstinner @bedevere-bot @pablogsal @gpshead @the-knights-who-say-ni