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

Not up-to-date with latest Android NDK #632

Closed
ndusart opened this issue Jun 28, 2017 · 9 comments
Closed

Not up-to-date with latest Android NDK #632

ndusart opened this issue Jun 28, 2017 · 9 comments

Comments

@ndusart
Copy link
Contributor

ndusart commented Jun 28, 2017

When trying to build the nix crate for target arm-linux-androideabi (nix-rust/nix#313), it fails with some undefined symbols that are present in the latest Android NDK headers (some seem to be added in Android API 21).

Since nearly all the symbols are merged in the android/mod.rs, and I didn't figured out how they are organised, I find it hard to list exhaustively the missing symbols.

So I just list the ones that are needed for the nix crate with a link to the corresponding header in the current ndk:

I could make a PR for these symbols but I think that we should make a full update to match the latest ndk and I do not feel comfortable enough to figure out what should be done and how it should be organised in this crate.

@alexcrichton
Copy link
Member

Thanks for the report! We take PRs at any point to add new constants and such and you can also just update the associated Dockerfile to the latest ndk to ensure that CI passes.

@ndusart
Copy link
Contributor Author

ndusart commented Jun 29, 2017

I'm working on adding these constants but I face a problem I prefer to ask you how to handle.

Before ndk r14b, it seems some headers were not correctly updated (changelog - Headers in M and N were actually headers for L) so even if in r13b (the rev the test suite is using) there are headers for api-24, they were actually for api-21 (lollipop).

Since r14b, they unified headers with one headers set for all the platforms with some #ifdef to select the correct definition based on the API level.

Therefore, I wonder how we should implement constants or functions that have different signature based on the API level. For example, the strerror_r is defined like this:

#if defined(__USE_GNU) && __ANDROID_API__ >= 23
char* strerror_r(int, char*, size_t) __RENAME(__gnu_strerror_r) __INTRODUCED_IN(23);
#else /* POSIX */
int strerror_r(int, char*, size_t);
#endif

Do we always use the most recent definitions and discard older ones ?

@roblabla
Copy link
Contributor

roblabla commented Jun 29, 2017

@ndusart your issue reminds me of #570

However, I checked with nm libc.so, and strerror_r is still defined in the binary (along with the new __gnu_strerror_r). So I think we should just keep the old one.

Eventually, I guess this could be implemented with https://internals.rust-lang.org/t/pre-rfc-target-extension-dealing-with-breaking-changes-at-os-level/5289/15 (hopefully android sets target_os_version to whatever API level).

@roblabla
Copy link
Contributor

BTW @ndusart ndk r15b is out now :) You may want to use this.

@ndusart
Copy link
Contributor Author

ndusart commented Jun 29, 2017

The thing with strerror_r is that if we build libc targeting api >= 23, then only the new definition will be available. Then if we stick to the old one, CI will fail.

Until we have a way to use target_os_version I think we should match the latest source.
Unified headers in NDK source specify quite well when a function or constant has been modified in a specific version, it should not be that hard to provide correct binding for older versions.

@roblabla
Copy link
Contributor

@ndusart Both strerror_r and __gnu_strerror_r are defined in the binary libc.so though, so keeping the old strerror_r will work on both old and new androids, whereas doing what you did not only breaks libc semver (meaning it needs a major bump to be merged), it won't work on old androids :/

You can check this by running nm $ANDROID_NDK/platforms/android-26/arch-arm/usr/lib/libc.so | grep strerror_r.

@alexcrichton
Copy link
Member

@ndusart I'd stick to the old symbol for sterror_r if it's still there, may as well keep backwards compat with older androids if we can!

@ndusart
Copy link
Contributor Author

ndusart commented Jun 30, 2017

I totally agree but I'd like to know how I can use the old definition while the generated test will expect the new one ?

If I use the old one, CI fail because strerror_r pointer is the wrong type as it is targeting api 24 and the C headers just do not keep the old one for api >= 23 (see previous post).

@alexcrichton, is it ok to modify the C header in the script that installs the android toolchain for the test to pass for this ?

@alexcrichton
Copy link
Member

@ndusart oh I'll comment in #634

bors added a commit that referenced this issue Jul 3, 2017
Update NDK to r15b and add some missing symbols

Use the new unified headers of the NDK and add some missing symbols for Android.

Fixes #632
@bors bors closed this as completed in #634 Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants