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

Android standalone toolchains are not longer necessary #874

Merged
merged 9 commits into from
Feb 25, 2019
Merged

Conversation

romainguy
Copy link
Collaborator

PLEASE CLEAN YOUR BUILD DIRECTORY (rm -Rf out/).

The latest NDK (19) contains ABI/API level specific command line
tools making standalone toolchains unnecessary. This change adapts
to the new model which greatly simplifies the build script and
ensures the latest version of the tools is being used.

Note that this requires a fairly recent version of CMake which fixes
a bug related to ranlib. This change was tested with CMake 3.13
(CMake 3.7 does not work).

The latest NDK (19) contains ABI/API level specific command line
tools making standalone toolchains unnecessary. This change adapts
to the new model which greatly simplifies the build script and
ensures the latest version of the tools is being used.

Note that this requires a fairly recent version of CMake which fixes
a bug related to ranlib. This change was tested with CMake 3.13
(CMake 3.7 does not work).
@romainguy
Copy link
Collaborator Author

Breaking in CI because of the version of CMake.

Copy link
Member

@bejado bejado left a comment

Choose a reason for hiding this comment

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

LGTM after CI is fixed

@@ -154,7 +154,7 @@ and tools.

To build Filament, you must first install the following tools:

- CMake 3.4 (or more recent)
- CMake 3.13 (or more recent)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could relax this requirement for users not building Android?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about it but wanted to keep things simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Android build should check for this requirement explicitly and fail with a useful message. The build errors I got were very opaque with cmake 3.10 and I took a circuitous route to get here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

build.sh now makes the check.

@romainguy romainguy merged commit b3284ed into master Feb 25, 2019
@romainguy romainguy deleted the rg/ndk branch February 25, 2019 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants