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

unified headers fix (master) #12746

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

rraallvv
Copy link
Contributor

@rraallvv rraallvv commented Nov 8, 2017

Enable using NDK unified headers when available and opt-out (more info)

@akien-mga
Copy link
Member

akien-mga commented Nov 8, 2017

I still don't see why we need an option at all. It doesn't matter to use whether we use unified headers or not, the result is the same and we just want it to build. Having the option makes sense for very specific Android apps, but in our case it's "one size fits all" and such customization isn't relevant for game devs.

Especially I'm not fond of having more code in the detect.py script for something that wouldn't be used.

@akien-mga akien-mga added this to the 3.0 milestone Nov 8, 2017
@rraallvv rraallvv force-pushed the unified_headers_fix branch from 7fac5c1 to 0e89425 Compare November 8, 2017 16:07
@rraallvv rraallvv changed the title unified headers fix unified headers fix (master) Nov 8, 2017
@rraallvv
Copy link
Contributor Author

rraallvv commented Nov 8, 2017

@akien-mga I see... I removed the option ndk_unified_headers, but I'm not sure if the helper functions should be squashed in one function (get_ndk_version(), same_version_or_newer(), and have_unified_headers()), or if the detection logic should be added in the body of configure().

@rraallvv rraallvv force-pushed the unified_headers_fix branch 2 times, most recently from 92fcfc9 to 30fcbdb Compare November 9, 2017 00:53
@rraallvv rraallvv force-pushed the unified_headers_fix branch from 30fcbdb to 7664a0f Compare November 9, 2017 13:47
@akien-mga akien-mga merged commit e4effb4 into godotengine:master Nov 9, 2017
lib_sysroot = env["ANDROID_NDK_ROOT"] + "/platforms/" + env['ndk_platform'] + "/" + env['ARCH']

## Compile flags

if ndk_unified_headers:
ndk_version = get_ndk_version(env["ANDROID_NDK_ROOT"])
if ndk_version != None and LooseVersion(ndk_version) >= LooseVersion("15.0.4075724"):
Copy link
Member

Choose a reason for hiding this comment

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

Well I merged too fast, this commit is incomplete. In the 2.1 one you have from distutils.version import LooseVersion, it's missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akien-mga My bad, somehow the built didn't fail alerting of the missing import. This PR has it. Sorry about that.

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.

2 participants