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: Updated toolchain file and dependencies #791

Merged
merged 1 commit into from Mar 4, 2015

Conversation

3 participants
@MarioLiebisch
Member

MarioLiebisch commented Jan 30, 2015

  • This PR supercedes the branch feature/android_toolchain.
  • Replaced the toolchain file with a new version based on zuhowei's fork, which supports the latest NDK versions as well as 64 bit targets.
  • Removed the STL dependency from sfml-activity rather than relying on some implementation implicitly linked by default.
  • Deleted project.properties, which wasn't supposed to be part of the repository code.
  • Updated CMake files to properly support Nvidia's Nsight Tegra Visual Studio Edition as an alternative to using the toolchain file (you might still have to update the STL implementation as that's not yet set through CMake).

Some Considerations:

  • getLibraryName() is used only once. As such we could skip the whole string handling and merge this function into loadLibrary().

@MarioLiebisch MarioLiebisch self-assigned this Jan 30, 2015

@MarioLiebisch MarioLiebisch added this to the 2.3 milestone Jan 30, 2015

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 7, 2015

Member

Updated the branch. It's now possible to build SFML without utilizing the toolchain file if you're using Visual Studio and Tegra Nsight VS Edition:

cmake -G "Visual Studio 12 2013" -DCMAKE_SYSTEM_NAME=Android -DANDROID_NATIVE_API_LEVEL=9 -DANDROID_ABI=armeabi path/to/source
Member

MarioLiebisch commented Feb 7, 2015

Updated the branch. It's now possible to build SFML without utilizing the toolchain file if you're using Visual Studio and Tegra Nsight VS Edition:

cmake -G "Visual Studio 12 2013" -DCMAKE_SYSTEM_NAME=Android -DANDROID_NATIVE_API_LEVEL=9 -DANDROID_ABI=armeabi path/to/source
Show outdated Hide outdated cmake/Macros.cmake Outdated
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 10, 2015

Member

getLibraryName() is used only once. As such we could skip the whole string handling and merge this function into loadLibrary().

What are the chances that it will be used multiple times at a later point?
Is the "string handling" an issue or can it be an issue?
Is it a "public" function?

Let me know if you intend to keep working on this PR or rather when it's ready to be merged.

Member

eXpl0it3r commented Feb 10, 2015

getLibraryName() is used only once. As such we could skip the whole string handling and merge this function into loadLibrary().

What are the chances that it will be used multiple times at a later point?
Is the "string handling" an issue or can it be an issue?
Is it a "public" function?

Let me know if you intend to keep working on this PR or rather when it's ready to be merged.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 11, 2015

Member

What are the chances that it will be used multiple times at a later point?

It's private (i.e. even static) and very unlikely to be reused. I can hardly think of any reason to determine the current library name. This is a bit like determining/using argv[0] in a desktop program, but even less useful since you can't use it for recursive calls. Only case I could imagine using this would be to more or less DRM a shared library so it can only be used by a specific other shared library, but at the same time the common user will never even see these names.

Let me know if you intend to keep working on this PR or rather when it's ready to be merged.

There are still a few quirks I'd like to change (e.g. the way I preset the ABI doesn't work properly yet).

Member

MarioLiebisch commented Feb 11, 2015

What are the chances that it will be used multiple times at a later point?

It's private (i.e. even static) and very unlikely to be reused. I can hardly think of any reason to determine the current library name. This is a bit like determining/using argv[0] in a desktop program, but even less useful since you can't use it for recursive calls. Only case I could imagine using this would be to more or less DRM a shared library so it can only be used by a specific other shared library, but at the same time the common user will never even see these names.

Let me know if you intend to keep working on this PR or rather when it's ready to be merged.

There are still a few quirks I'd like to change (e.g. the way I preset the ABI doesn't work properly yet).

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 25, 2015

Member

I'm going to squash and merge this today unless I hear anything which would prevent this. I'll leave getLibraryName() the way it is right now.

Member

MarioLiebisch commented Feb 25, 2015

I'm going to squash and merge this today unless I hear anything which would prevent this. I'll leave getLibraryName() the way it is right now.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 25, 2015

Member

You can squash it, I'll then merge it. 😉

Member

eXpl0it3r commented Feb 25, 2015

You can squash it, I'll then merge it. 😉

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 25, 2015

Member

I wouldn't add the examples/android/.gitignore file. It doesn't really serve any purpose.

Member

eXpl0it3r commented Feb 25, 2015

I wouldn't add the examples/android/.gitignore file. It doesn't really serve any purpose.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 25, 2015

Member

Well, it keeps git's status clean when building in-tree (which you typically do), but then again it's not necessary... hm...

Member

MarioLiebisch commented Feb 25, 2015

Well, it keeps git's status clean when building in-tree (which you typically do), but then again it's not necessary... hm...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 25, 2015

Member

It's never really recommended to build in tree and if you want the status to be clean we'd have to add a ton of additional git ignores, which is just ugly and doesn't really help.
If you personally want to ignore something, then the best way is to add it locally to the global gitignore file.

Member

eXpl0it3r commented Feb 25, 2015

It's never really recommended to build in tree and if you want the status to be clean we'd have to add a ton of additional git ignores, which is just ugly and doesn't really help.
If you personally want to ignore something, then the best way is to add it locally to the global gitignore file.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 25, 2015

Member

when building in-tree (which you typically do)

Nooooooooooooooooo. Nobody does that. Not in a source tree that you're constantly working in, at least. Please make you a favor, and create your build directory out of the source tree 😛

Member

LaurentGomila commented Feb 25, 2015

when building in-tree (which you typically do)

Nooooooooooooooooo. Nobody does that. Not in a source tree that you're constantly working in, at least. Please make you a favor, and create your build directory out of the source tree 😛

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 25, 2015

Member

I obviously do. :D But okay, point taken, removing it.

Member

MarioLiebisch commented Feb 25, 2015

I obviously do. :D But okay, point taken, removing it.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 25, 2015

Member

There you go, squashed, rebased and ready. :)

Member

MarioLiebisch commented Feb 25, 2015

There you go, squashed, rebased and ready. :)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 27, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 27, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 27, 2015

Member

There are tabs all over the place in CMake files, uppercase functions ("SET" instead of "set"), and what's the thing with Nsight?

Member

LaurentGomila commented Feb 27, 2015

There are tabs all over the place in CMake files, uppercase functions ("SET" instead of "set"), and what's the thing with Nsight?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 27, 2015

Member

Gna, there shouldn't be tabs~ Nsight is a plugin for Visual Studio providing NDK integration (compiling/linking, debugging, etc.).

Member

MarioLiebisch commented Feb 27, 2015

Gna, there shouldn't be tabs~ Nsight is a plugin for Visual Studio providing NDK integration (compiling/linking, debugging, etc.).

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 27, 2015

Member

Okay, updated the branch - I didn't touch the new toolchain file though, since I'd like to keep it 1:1 to the original (linked above).

Member

MarioLiebisch commented Feb 27, 2015

Okay, updated the branch - I didn't touch the new toolchain file though, since I'd like to keep it 1:1 to the original (linked above).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 27, 2015

Member

If it's an "external dependency" you should exlude it from the .gitattributes (and .editorconfig).

Member

eXpl0it3r commented Feb 27, 2015

If it's an "external dependency" you should exlude it from the .gitattributes (and .editorconfig).

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 27, 2015

Member

Added. EditorConfig doesn't support exclusions as far as I know, but should be fine (since the shouldn't be edited anyway; similar to stb stuff).

Member

MarioLiebisch commented Feb 27, 2015

Added. EditorConfig doesn't support exclusions as far as I know, but should be fine (since the shouldn't be edited anyway; similar to stb stuff).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 28, 2015

Member

Nsight is a plugin for Visual Studio providing NDK integration

Ok but my question was more: why does it need special stuff (find_host_package) in addition to the regular toolchain? Would be nice to leave a comment directly in the CMake file if there's something relevant to know about this.

Member

LaurentGomila commented Feb 28, 2015

Nsight is a plugin for Visual Studio providing NDK integration

Ok but my question was more: why does it need special stuff (find_host_package) in addition to the regular toolchain? Would be nice to leave a comment directly in the CMake file if there's something relevant to know about this.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 28, 2015

Member

The latest change on master has created a conflict with this branch. Make sure to rebase and resolve.

Member

eXpl0it3r commented Feb 28, 2015

The latest change on master has created a conflict with this branch. Make sure to rebase and resolve.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 28, 2015

Member

Ok but my question was more: why does it need special stuff (find_host_package) in addition to the regular toolchain?

Simply because it doesn't use the toolchain at all. It's a "special" target built into CMake, which is triggered by setting CMAKE_SYSTEM_NAME to Android while creating a MSVC project.

Going to add a short note together with the rebase/merge.

Member

MarioLiebisch commented Feb 28, 2015

Ok but my question was more: why does it need special stuff (find_host_package) in addition to the regular toolchain?

Simply because it doesn't use the toolchain at all. It's a "special" target built into CMake, which is triggered by setting CMAKE_SYSTEM_NAME to Android while creating a MSVC project.

Going to add a short note together with the rebase/merge.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch
Member

MarioLiebisch commented Feb 28, 2015

Done.

@eXpl0it3r eXpl0it3r added the s:accepted label Mar 2, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 2, 2015

Member

This PR has been re-added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 2, 2015

This PR has been re-added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 2, 2015

Member

Building this for testing I noticed that you added (at least) three options, but didn't mark them with the SFML_ prefix.
Also is it possible to only "display"/set them when one is actually trying to build for Android, otherwise it can be a bit irritating.

Android

Member

eXpl0it3r commented Mar 2, 2015

Building this for testing I noticed that you added (at least) three options, but didn't mark them with the SFML_ prefix.
Also is it possible to only "display"/set them when one is actually trying to build for Android, otherwise it can be a bit irritating.

Android

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 2, 2015

Member

They shouldn't appear when you're not building for Android. Do they still appear for you? If so, that's a bug. Unfortunately they have to be set before the toolchain is invoked (using CMake's project() command), so there's no way in using your usual CMake variables for branching.

We could wrap those with a SFML_ prefix, but behind the scenes we'd still have to set the non-prefixed variables. Those are used by the toolchain file and CMake.

Edit: Yeah they appear, bah, that's definitely not intentional. :)

Member

MarioLiebisch commented Mar 2, 2015

They shouldn't appear when you're not building for Android. Do they still appear for you? If so, that's a bug. Unfortunately they have to be set before the toolchain is invoked (using CMake's project() command), so there's no way in using your usual CMake variables for branching.

We could wrap those with a SFML_ prefix, but behind the scenes we'd still have to set the non-prefixed variables. Those are used by the toolchain file and CMake.

Edit: Yeah they appear, bah, that's definitely not intentional. :)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 2, 2015

Member

I've updated the branch. Try again, they should be gone now. This still leaves the name thing though. Although there are many, many more advanced variables ANDROID_... set by the toolchain file.

Member

MarioLiebisch commented Mar 2, 2015

I've updated the branch. Try again, they should be gone now. This still leaves the name thing though. Although there are many, many more advanced variables ANDROID_... set by the toolchain file.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 2, 2015

Member

Ah I guess the naming is okay if it's used by the toolchain. I was mostly confused to see the options on my standard MinGW build.

Seems to work fine now. 👍

Member

eXpl0it3r commented Mar 2, 2015

Ah I guess the naming is okay if it's used by the toolchain. I was mostly confused to see the options on my standard MinGW build.

Seems to work fine now. 👍

Android: Updated the toolchain file and CMake scripts
* Replaced the toolchain file with a new version based on [zuhowei's fork](https://github.com/zhuowei/android-cmake), which enables x64 builds as well as support for the latest NDK. This breaks compatibility with old build directories.
* Removed the STL dependency from **sfml-activity** rather than relying on *some* implementation implicitly linked by default.
* Deleted *project.properties*, which wasn't supposed to be part of the repository code. You have to use the Android SDK to recreate it (`android update project --path to/your/example --target 1 --name SFML-Example`).
* Made it possible to select a STL implementation to be used (default: `c++_shared`). Keep in mind that not all available configurations are necessarily compatible with SFML.
* Fixed linker flags to be compatible with Nvidia's Nsight Tegra for Visual Studio.
* It is now possible to compile the Android version using Nvidia's Nsight Tegra for Visual Studio (requires up-to-date CMake and `CMAKE_SFML_SYSTEM` to be set to `Android`; keep in mind that this is still experimental and requires further CMake updates).
* Updated and renamed some Android specific CMake variables.
* Made `armeabi-v7a` the default ABI for Android builds.

@eXpl0it3r eXpl0it3r merged commit 34692d5 into master Mar 4, 2015

@eXpl0it3r eXpl0it3r deleted the feature/andfixes branch Mar 4, 2015

@MarioLiebisch MarioLiebisch removed their assignment Apr 30, 2015

@eXpl0it3r eXpl0it3r added this to Done in Android Backlog Jan 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment