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

[zstd][android] Fix build with NDK r27 #4107

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

Adenilson
Copy link

The NDK cross compiler declares the target as __linux (which is not technically incorrect), which triggers the enablement of _GNU_SOURCE in the newly added code that requires the presence of qsort_r() used in the COVER dictionary code.

Even though the NDK uses llvm/libc, it doesn't declare qsort_r() in the stdlib.h header.

The build fix is to only activate the _GNU_SOURCE macro if the OS is not Android, as then we will fallback to the C90 compliant code.

This patch should solve the reported issue number #4103.

The NDK cross compiler declares the target as __linux (which is
not technically incorrect), which triggers the enablement of _GNU_SOURCE
in the newly added code that requires the presence of qsort_r() used
in the COVER dictionary code.

Even though the NDK uses llvm/libc, it doesn't declare qsort_r()
in the stdlib.h header.

The build fix is to only activate the _GNU_SOURCE macro if the OS is
*not* Android, as then we will fallback to the C90 compliant code.

This patch should solve the reported issue number facebook#4103.
@Adenilson
Copy link
Author

@Cyan4973 I've reached out to the Android folks at google and they started the work to actually fix the NDK/AOSP in upcoming future versions:
https://android-review.googlesource.com/c/platform/bionic/+/3199671

The function signature should be the same as on GNU/Linux systems.

@Adenilson
Copy link
Author

So hopefully in the future when API level 36 is released to the wild, we could actually enable the qsort_r() fix for that API level and fallback to the C90 code for older versions.

@Cyan4973
Copy link
Contributor

Looks good @Adenilson !

@Cyan4973 Cyan4973 merged commit d4b176d into facebook:dev Jul 30, 2024
93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants