Skip to content

Commit

Permalink
add support for Android NDK unified headers
Browse files Browse the repository at this point in the history
  • Loading branch information
@rraallvv
rraallvv committed Nov 6, 2017
1 parent b8ac700 commit ec31b23
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 3 deletions.
39 changes: 36 additions & 3 deletions platform/android/detect.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def get_opts():
EnumVariable('android_arch', 'Target architecture', "armv7", ('armv7', 'armv6', 'arm64v8', 'x86')),
BoolVariable('android_neon', 'Enable NEON support (armv7 only)', True),
BoolVariable('android_stl', 'Enable Android STL support (for modules)', False),
BoolVariable('ndk_unified_headers', 'Enable NDK unified headers', True)
]


Expand Down Expand Up @@ -178,12 +179,28 @@ def mySpawn(sh, escape, cmd, args, env):
env['RANLIB'] = tools_path + "/ranlib"
env['AS'] = tools_path + "/as"

sysroot = env["ANDROID_NDK_ROOT"] + "/platforms/" + env['ndk_platform'] + "/" + env['ARCH']
ndk_unified_headers = env['ndk_unified_headers']
ndk_version = get_ndk_version(env["ANDROID_NDK_ROOT"])

common_opts = ['-fno-integrated-as', '-gcc-toolchain', gcc_toolchain_path]

if not ndk_unified_headers and ndk_version != None and ndk_version[0] >= 16:
ndk_unified_headers = True

This comment has been minimized.

Sign in to view

Copy link
@endragor

endragor Nov 7, 2017

Contributor

Isn't it misleading that the user input gets overridden? What's the point is then to have it as an option?
Also, this code forces usage of unified headers by default even on older NDK versions that do not support them which breaks compilation there.

My suggestion is to determine ndk_unified_headers value strictly from NDK version. If it's old, do not use them, otherwise do use them.

This comment has been minimized.

Sign in to view

Copy link
@rraallvv

rraallvv Nov 7, 2017 •
edited
Loading

Author Contributor

@endragor ndk_unified_headers works in a similar way to APP_UNIFIED_HEADERS in ndk-build and ANDROID_UNIFIED_HEADERS in CMake.

This blog post explains those shenanigans.

The prior approach relied on periodically-captured snapshots of the platform headers. This meant that any time we fixed a header-only bug, the fix was only available in the latest version aside from the occasional backport. Now bugfixes are available regardless of your NDK API level.

So, it seems that having ndk_unified_headers on by default is a more desirable configuration, also the option is overridden for r16 and newer versions because if someone turns that option off is because he/she intentionally wanted to compile with the old headers, and knows what version has those available, otherwise he/she might not know what the option is about ndk_unified_headers.

This comment has been minimized.

Sign in to view

Copy link
@endragor

endragor Nov 7, 2017

Contributor

It doesn't work similar here according to what I read. They have it off by default on older version and on by default since r15. Your changes break compilation if a person has NDK < r14 and compiles Android with scons platform=android, without providing additional options. I'm sure it's not how it's supposed to be. I have NDK r12 and I came here specifically because this was a breaking change.

And I still don't see a reason behind having this as an option on old NDKs, including those that don't support unified headers. And it turns out to be a fake option on newer NDKs. There is a clear distinction between NDKs with unified header support and without one, so why not use it?

@RandomShaper @volzhs what do you think?

This comment has been minimized.

Sign in to view

Copy link
@rraallvv

rraallvv Nov 7, 2017

Author Contributor

@endragor I'm not sure what you mean by breaking change, does Scons fail to build passing ndk_unified_headers=no with NDK r12?

This comment has been minimized.

Sign in to view

Copy link
@endragor

endragor Nov 8, 2017

Contributor

I mean that building without additional options should work, i.e. you should not provide ndk_unified_headers=no to be able to build with NDK r12. And it doesn't make sense to do that, since r12 does not support unified headers, so why burden the user?

This comment has been minimized.

Sign in to view

Copy link
@endragor

endragor Nov 8, 2017

Contributor

Also @akien-mga

This comment has been minimized.

Sign in to view

Copy link
@RandomShaper

RandomShaper Nov 8, 2017

Member

Maybe the should be an 'auto' default that renders to

  • 'yes' if they are available (NDK r14b or newer);
  • 'no' in any other case.

This comment has been minimized.

Sign in to view

Copy link
@endragor

endragor Nov 8, 2017

Contributor

That is certainly better than the current approach. My personal taste is to have less config options, especially when they are unnecessary. In this case I don't see why would anyone bother with setting this option to yes or no, but maybe I'm missing something.

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Nov 8, 2017

Member

If the autodetection logic is trustworthy, then we don't need any option indeed.

This comment has been minimized.

Sign in to view

Copy link
@RandomShaper

RandomShaper Nov 8, 2017

Member

I second that. Let it be always automatic and print a message about the outcome.

This comment has been minimized.

Sign in to view

Copy link
@rraallvv

rraallvv Nov 8, 2017 •
edited
Loading

Author Contributor

@endragor these PRs (master and 2.1) should get you covered.

@akien-mga, @RandomShaper I removed the option and split the logic in three separate functions (get_ndk_version(), same_version_or_newer(), and have_unified_headers()). I was thinking that maybe they could be useful in some other place in configure() on in the future, but I can squash them into have_unified_headers() or move the detection logic to configure().

Please let me know that approach you think is a better.

This comment has been minimized.

Sign in to view

Copy link
@RandomShaper

RandomShaper Nov 8, 2017

Member

I've not seen that code yet, but I'd say get_ndk_version() and same_version_or_newer() are fine as separate functions, while have_unified_headers() is too trivial to be a function (assumming it's just comparing installed NDK version with the minimum version in which unified headers were available).

Even if the logic was needed in multiple places, just assigning the result to a flag and checking it would be clean enough.

This comment has been minimized.

Sign in to view

Copy link
@rraallvv

rraallvv Nov 8, 2017

Author Contributor

@akien-mga, @RandomShaper

Researching a bit further this is what I've found:

14.0.3529234-beta1 <- introduced unified headers as opt-in feature (r14-beta1 changelog)
14.0.3675639-beta2
14.0.3770861
14.1.3816874

15.0.3869609-beta1 <- unified headers are opt-out (r15-beta1 changelog)
15.0.3996722-beta2
15.0.4075724 <- unified headers release

16.0.4293906-beta1 <- deprecated headers removed (r16-beta1 changelog)
16.0.4414007-beta2

It seems the most sensible thing would be to use unified headers starting from r15-release (15.0.4075724)

I also ended up using distutils.version.LooseVersion to compare the string versions, which render unnecessary the additional helper functions except for get_ndk_version().

Cheers!

This comment has been minimized.

Sign in to view

Copy link
@RandomShaper

RandomShaper Nov 9, 2017

Member

Nice job!

But as far as I know, unified headers are always better so why not using them starting from the release they were first available?

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Nov 9, 2017

Member

Starting from r15 seems good to me. We don't need to go faster than Google, if they were opt-in in r14, we don't need them.

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Nov 9, 2017 •
edited
Loading

Member

(Again, we did not have any issue with those non-unified headers anyway, so no need to go too much into the detail. We just need Godot to compile out of the box without hacks on all recent NDK releases).

This comment has been minimized.

Sign in to view

Copy link
@RandomShaper

RandomShaper via email Nov 9, 2017

Member

This comment has been minimized.

Sign in to view

Copy link
@rraallvv

rraallvv Nov 9, 2017

Author Contributor

@akien-mga, @RandomShaper switched to r15-release

By the way, it seems travis has some issues with macOS, here and here

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Nov 9, 2017

Member

Yeah they have infra issues, we can ignore those builds for now.
print("Turning NDK unified headers on (starting from r16)")

lib_sysroot = env["ANDROID_NDK_ROOT"] + "/platforms/" + env['ndk_platform'] + "/" + env['ARCH']

## Compile flags

env.Append(CPPFLAGS=["-isystem", sysroot + "/usr/include"])
if ndk_unified_headers:
sysroot = env["ANDROID_NDK_ROOT"] + "/sysroot"
env.Append(CPPFLAGS=["-isystem", sysroot + "/usr/include"])
env.Append(CPPFLAGS=["-isystem", sysroot + "/usr/include/" + abi_subpath])
# For unified headers this define has to be set manually
env.Append(CPPFLAGS=["-D__ANDROID_API__=" + str(int(env['ndk_platform'].split("-")[1]))])
else:
env.Append(CPPFLAGS=["-isystem", lib_sysroot + "/usr/include"])

env.Append(CPPFLAGS='-fpic -ffunction-sections -funwind-tables -fstack-protector-strong -fvisibility=hidden -fno-strict-aliasing'.split())
env.Append(CPPFLAGS='-DNO_STATVFS -DGLES2_ENABLED'.split())

Expand Down Expand Up @@ -224,7 +241,7 @@ def mySpawn(sh, escape, cmd, args, env):

## Link flags

env['LINKFLAGS'] = ['-shared', '--sysroot=' + sysroot, '-Wl,--warn-shared-textrel']
env['LINKFLAGS'] = ['-shared', '--sysroot=' + lib_sysroot, '-Wl,--warn-shared-textrel']
if env["android_arch"] == "armv7":
env.Append(LINKFLAGS='-Wl,--fix-cortex-a8'.split())
env.Append(LINKFLAGS='-Wl,--no-undefined -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now'.split())
Expand All @@ -248,3 +265,19 @@ def mySpawn(sh, escape, cmd, args, env):
if (env["android_arch"] == "armv6" or env["android_arch"] == "armv7"):
env.Append(CFLAGS=["-DOPUS_ARM_OPT"])
env.opus_fixed_point = "yes"

# Return NDK version as [<major>,<minor>,<build>] or None if cannot be figured out (adapted from the Chromium project).
def get_ndk_version (path):
if path == None:
return None
prop_file_path = os.path.join(path, "source.properties")
try:
with open(prop_file_path) as prop_file:
for line in prop_file:
key_value = map(lambda x: string.strip(x), line.split("="))
if key_value[0] == "Pkg.Revision":
version_parts = key_value[1].split("-")[0].split(".")
return map(int, version_parts[0:3])
except:
print("Could not read source prop file '%s'" % prop_file_path)
return None
1 change: 1 addition & 0 deletions platform/android/platform_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/*************************************************************************/
#include <alloca.h>
#include <malloc.h>

0 comments on commit ec31b23

Please sign in to comment.