classification
Title: Compilation failure against Android NDK r14 beta 2
Type: compile error Stage: resolved
Components: Build Versions: Python 3.7
process
Status: closed Resolution: later
Dependencies: Superseder:
Assigned To: Nosy List: skrah, vstinner, xdegaye, yan12125
Priority: normal Keywords: patch

Created on 2017-02-03 16:56 by yan12125, last changed 2017-02-19 17:43 by xdegaye. This issue is now closed.

Files
File name Uploaded Description Edit
nl_langinfo.patch yan12125, 2017-02-03 16:56 Requires autoreconf review
langinfo.h yan12125, 2017-02-03 17:00 langinfo.h from Android NDK
Pull Requests
URL Status Linked Edit
PR 159 closed yan12125, 2017-02-18 13:20
Messages (11)
msg286877 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-02-03 16:56
Since Android NDK r14 beta 2, unified headers provide langinfo.h but there's no nl_langinfo() in it, causing linking failures:

libpython3.7m.a(pylifecycle.o): In function `get_locale_encoding':
/home/yen/Projects/python3-android/src/cpython/Python/pylifecycle.c:234: undefined reference to `nl_langinfo'
libpython3.7m.a(fileutils.o): In function `_Py_device_encoding':
/home/yen/Projects/python3-android/src/cpython/Python/fileutils.c:65: undefined reference to `nl_langinfo'
libpython3.7m.a(_localemodule.o): In function `PyLocale_nl_langinfo':
/home/yen/Projects/python3-android/src/cpython/./Modules/_localemodule.c:447: undefined reference to `nl_langinfo'

Or compiler errors due to implicit function declarations if the patch at issue22747 is applied.

nl_langinfo.patch fixes it by adding some extra guarding macros.

Added some people from issue22747 to the nosy list, where the last change about langinfo.h and Android occurred.
msg286878 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-02-03 17:00
Here's a copy of $ANDROID_NDK/sysroot/usr/include/langinfo.h. (/sysroot/ stores unified headers [1]) To use those headers correctly, packagers have to add -D__ANDROID_API__=XY to CPPFLAGS. On the other hand, __ANDROID_API_FUTURE__ is defined in $ANDROID_NDK/sysroot/usr/include/android/api-level.h:

/*
 * Magic version number for a current development build, which has
 * not yet turned into an official release.
 */
#ifndef __ANDROID_API_FUTURE__
#define __ANDROID_API_FUTURE__ 10000
#endif

As a result nl_langinfo() does not exist in all real API versions.

[1] https://android.googlesource.com/platform/ndk.git/+/master/docs/UnifiedHeaders.md
msg288079 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-02-18 13:24
I've improved the patch and submitted it as PR 159. Now on Android:

1. _locale is built
2. _locale has several symbols, including CODESET
3. _locale doesn't have nl_langinfo()

Basically _locale maps what langinfo.h does. On Android langinfo.h has several #defines like CODESET but nl_langinfo() is missing from langinfo.h and libc.so.

On my ASUS ZE500KL, both test_locale and test_site (see: issue28596) pass.
msg288080 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-02-18 13:59
Well, I have to commit changes to configure and pyconfig.h, otherwise Travis tests fail. Maybe .travis.yml should call autoreconf first.
msg288082 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-02-18 14:51
Previously, we asked to not include generated files in patches, like
configure. With the new dev process (github, travis), you must now include
generated files.
msg288083 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-02-18 17:39
> I've improved the patch and submitted it as PR 159. Now on Android:

This is confusing. Android NDK r14 has not been released yet.
We do not develop CPython for platform beta releases.
Unified headers, the reason for your proposed change, are only introduced in NDK r14.
Any reason why you do omit mentioning these important facts in the issue ?
Maybe you think that Stephan, Victor and the other readers are aware of this ?
What about the time they waste reading this issue and your PR when this issue does not make sense with the NDK we are using (NDK r13) and is just a waste of time now ?
Why do you publish a PR ?
msg288084 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-02-18 17:45
Closing this issue as 'later'.
Chi Hsuan Yen, I am still interested by your answers to my questions.
msg288085 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-02-18 18:05
> Any reason why you do omit mentioning these important facts in the issue ?

Both in the title of this issue and that PR, I mention "NDK r14 beta 2". I'm not sure what you're referring to. On IRC, irker states the title first and then the comment content. I believe people will read the title first to determine whether they're interested or not. On email notifications of b.p.o and Github updates, the title is used as the email subject, so people can determine whether they want to see this email fast.

> Maybe you think that Stephan, Victor and the other readers are aware of this ?

As explained above, I've already stated "beta" clearly

> What about the time they waste reading this issue and your PR when this issue does not make sense with the NDK we are using (NDK r13) and is just a waste of time now ?

For me, reading the title takes a few seconds. If a developer thinks there's no need to support beta platforms, they can just ignore it.

> Why do you publish a PR ?

This is a real issue. Usually NDK beta 2 is almost identical to the final release. For me it's good to fix things as soon as possible. There are some other examples for beta toolchains like issue1465, issue27806 or issue27596.

If you think issues on beta should be postponed until the final release, I can keep my patches locally and submit them later.
msg288124 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-02-19 10:11
> For me it's good to fix things as soon as possible.

The problem here is that you are attempting to push a change through a PR without providing the other core developers with important information. You should at least have given a reference to issue 29040 and explain why you think it is safe to push this change now and not wait for the decision to switch to r14 and you should have done it *before* publishing the PR. By behaving this way, you may lead other core developers to think that you cannot be trusted.

> If you think issues on beta should be postponed until the final release, I can keep my patches locally and submit them later.

Yes, please do. Note that this issue is closed for now anyway.
Also when you are running Python on Android and publishing some results on bpo (such as for example in issue 29176) please make sure that Python has been built with r13.
msg288129 - (view) Author: Chih-Hsuan Yen (yan12125) * Date: 2017-02-19 11:25
Thanks for the response. Sorry if my previous work on Android brings confusion.

To prevent possible wasting of time in the future, I'd like to confirm myself for some conclusions: CPython's support for Android targets only stable releases of the official NDK. That includes all building time and runtime results. I guess that's the current policy? Thanks in advance for any further replies.
msg288148 - (view) Author: Xavier de Gaye (xdegaye) * (Python triager) Date: 2017-02-19 17:43
> CPython's support for Android targets only stable releases of the official NDK.

As the NDK is seeing many changes in the recent past and near future (deprecation of the support of gcc in favor of clang, Unified Headers, nearly all the NDK scripts written in python, ...) we should use the latest stable NDK release.

> That includes all building time and runtime results.

You can open new issues for NDK r14 with the description of the problem and with NDK r14 building time and runtime results, as a reminder of what will need to be done when we switch to r14 as it was done for issue 29040. But please no PR. Maybe add a comment saying that for the moment this is just a reminder.
History
Date User Action Args
2017-02-19 17:43:58xdegayesetmessages: + msg288148
2017-02-19 11:25:47yan12125setmessages: + msg288129
2017-02-19 10:11:17xdegayesetmessages: + msg288124
2017-02-18 18:05:28yan12125setmessages: + msg288085
2017-02-18 17:45:10xdegayesetstatus: open -> closed
resolution: later
messages: + msg288084

stage: resolved
2017-02-18 17:39:40xdegayesetmessages: + msg288083
2017-02-18 14:51:10vstinnersetmessages: + msg288082
2017-02-18 13:59:33yan12125setmessages: + msg288080
2017-02-18 13:24:20yan12125setmessages: + msg288079
2017-02-18 13:20:23yan12125setpull_requests: + pull_request124
2017-02-03 17:00:56yan12125setfiles: + langinfo.h

messages: + msg286878
2017-02-03 16:56:30yan12125create