Converted to android-cmake for APK creation #2526

tigerw · Oct 5, 2015

Various changes to allow for Android to compile successfully

madmaxoft · Oct 5, 2015

madmaxoft reviewed Oct 5, 2015

View changes

madmaxoft · Oct 5, 2015

Android/CMakeLists.txt

+
+externalproject_add(lua_native
+	SOURCE_DIR ../lib/lua
+	CMAKE_GENERATOR "Visual Studio 14 2015"


I don't think this is the right way to go - what about building this on Linux?

It's definitely not right, but conversely, I have no idea how to get the autodetection working as it does when you run cmake .

Would anyone have any idea about this? It could be a showstopper for integration into @bearbin's Jenkins server.

Looking at this again, is their any reason not to use the CMAKE_GENERATOR variable? You're always going to want to use the same generator as the main project.

“is there any reason not to use...”

Did you mean to use? If you did, I think it didn’t work if I didn’t specify that variable, but I’ll try again soon.

I've only just seen this after your reference in #3161. But what I meant was why cant you do CMAKE_GENERATOR ${CMAKE_GENERATOR}?

Would anyone have any idea about this? It could be a showstopper for integration...

...and @worktycho comes to the rescue. :)

So with regards to this, I'm planning to merge the core server related changes in, then remove the Android folder and place that into cuberite-android, and add the main repository as a submodule.

tigerw · Oct 5, 2015

So with regards to this, I'm planning to merge the core server related changes in, then remove the Android folder and place that into cuberite-android, and add the main repository as a submodule.

sphinxc0re · Oct 5, 2015

sphinxc0re reviewed Oct 5, 2015

View changes

sphinxc0re · Oct 5, 2015

Android/.gitignore

+/lua_native-prefix/
+/tolua_native-prefix/
+CMakeCache.txt
+local.properties


missing newline

sphinxc0re · Oct 5, 2015

sphinxc0re reviewed Oct 5, 2015

View changes

sphinxc0re · Oct 5, 2015

Android/AndroidNdkModules.cmake

+        
+        unset(_NDK_HELPER_SRCS)
+    endif()
+endmacro()


Missing newline

sphinxc0re · Oct 5, 2015

sphinxc0re reviewed Oct 5, 2015

View changes

Android/CMakeLists.txt

You should really update your branch

sphinxc0re · Oct 5, 2015

You should really update your branch

I have updated my branch - if you're referring to project (MCServer), I did it before the rename en-masse.

tigerw · Oct 5, 2015

I have updated my branch - if you're referring to project (MCServer), I did it before the rename en-masse.

What about warnings? It looks like you've left it a gcc's defaults, which is particularly problematic for non-x86 platforms like android where assumptions are more likely to be violated.

worktycho · Oct 5, 2015

What about warnings? It looks like you've left it a gcc's defaults, which is particularly problematic for non-x86 platforms like android where assumptions are more likely to be violated.

worktycho · Oct 5, 2015

worktycho reviewed Oct 5, 2015

View changes

Android/CMakeLists.txt

What compile options does this build with? Experience of the pi has shown that cuberite requires several non-standard compile options on arm.

-fomit-frame-pointer : mbedtls miscompiles on armv6 without this
-fsigned-char : various bugs in cuberite's core are exposed if this is not enabled.
-ffast-math : improves performance, by allowing the compiler to break the IEEE 754 standard

worktycho · Oct 5, 2015

What compile options does this build with? Experience of the pi has shown that cuberite requires several non-standard compile options on arm.

-fomit-frame-pointer : mbedtls miscompiles on armv6 without this
-fsigned-char : various bugs in cuberite's core are exposed if this is not enabled.
-ffast-math : improves performance, by allowing the compiler to break the IEEE 754 standard

Have a look at the android cmake toolchain file, I think that the first two are present. I'm not sure about the last, but I'm assuming that since soft-float is so slow, there won't be a particularly noticeable speed increase.

tigerw · Oct 6, 2015

Have a look at the android cmake toolchain file, I think that the first two are present. I'm not sure about the last, but I'm assuming that since soft-float is so slow, there won't be a particularly noticeable speed increase.

About warnings; what are good warnings? Since the plan is to include the core/desktop repository as a submodule, perhaps warnings would only really be effective when generated compiling the desktop version?

tigerw · Oct 6, 2015

About warnings; what are good warnings? Since the plan is to include the core/desktop repository as a submodule, perhaps warnings would only really be effective when generated compiling the desktop version?

-fomit-frame-pointer is only passed in release, we need it passed in debug as well.
fast-math is especially important on soft float systems, because optimizations will have more of an effect on slower operations.

As for warnings, they are the real problem with the submodule strategy. We don't want warnings passed to library code but we do for our code. Why can you not have non-platform specific flags handled by setflags?

worktycho · Oct 6, 2015

-fomit-frame-pointer is only passed in release, we need it passed in debug as well.
fast-math is especially important on soft float systems, because optimizations will have more of an effect on slower operations.

As for warnings, they are the real problem with the submodule strategy. We don't want warnings passed to library code but we do for our code. Why can you not have non-platform specific flags handled by setflags?

Okay, I'll try to SetFlags then. I didn't use it since I really couldn't be bothered to trawl through the dense file and correct the "If not Windows, is Linux" and "Linux is all the same" behaviour.

tigerw · Oct 8, 2015

Okay, I'll try to SetFlags then. I didn't use it since I really couldn't be bothered to trawl through the dense file and correct the "If not Windows, is Linux" and "Linux is all the same" behaviour.

Theres very little that assumes Linux. It does tend to assume a POSIXish environment if not windows, but android should be POSIXish. And we have enough special cases for POSIX, so android should be simpler. Android is closer to Linux than FreeBSD and OS X. Look at the FreeBSD build cases for examples.

worktycho · Oct 8, 2015

Theres very little that assumes Linux. It does tend to assume a POSIXish environment if not windows, but android should be POSIXish. And we have enough special cases for POSIX, so android should be simpler. Android is closer to Linux than FreeBSD and OS X. Look at the FreeBSD build cases for examples.

Any updates?

sphinxc0re · Nov 5, 2015

Any updates?

@tigerw

Is this still in progress or did you drop this? @tigerw

sphinxc0re · Nov 13, 2015

Is this still in progress or did you drop this? @tigerw

Still in progress.

tigerw · Nov 21, 2015

Still in progress.

Now?

tigerw · Dec 20, 2015

Now?

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

SetFlags.cmake

@@ -86,7 +86,11 @@ macro(set_flags)
 		else()
 			add_flags_cxx("-pthread")
 		endif()
-
+	elseif(ANDROID)
+		set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14")


c++11 only. I don't want to have to deal with more compiler variation than we have to. And yes, c++14 has caused breaking changes for template free functions with the same name as a new standard library function. (ADL makes it impossible to add template free functions without causing breaking changes).

You won’t be dealing with it, considering this will be in a separate repository. It’ll probably end up being me dealing with it :P

Then let me advise you to stick to c++11 to avoid having a similar problem to when a PR wouldn't compile on windows because msvc exposes std::make_unique, which was causing issues with my make_unique if it was in the global namespace.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

lib/jsoncpp

@@ -1 +1 @@
-Subproject commit 81cf237917b6873decd27e15b7fe8473003a2762
+Subproject commit beae99924f29d88405576d5b9787bd494ec13e7a


worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/Bindings/ManualBindings_Network.cpp

 	S.GetStackValues(2, Data, RemotePeer, RemotePort);

 	// Check the port:
-	if ((RemotePort < 0) || (RemotePort > USHRT_MAX))
+	if ((RemotePort < 0) || (RemotePort > std::numeric_limits<UInt16>::max()))


This is never true. Use the checks in GetStackValues.

Oh yeah!

Then in that case, when are the checks in GetStackValues triggered? Is it something to do with a difference between lua_Number and the template type T? Is lua_Number bigger?

GetStackValues will return false if the input value is not a number or falls outside the range of the type you have requested conversion to.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

Android/CMakeLists.txt

+	INSTALL_COMMAND ""
+)
+externalproject_add(tolua_native
+	SOURCE_DIR ../lib/tolua++


How is this referencing the lua target?

I’m not sure.
• If you mean how it’s doing the referencing, I’m still not sure.
• If you mean it’s not referencing anything but that I’m saying that it is, then I’m confused.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

Android/CMakeLists.txt

+	../lib/libevent/include/
+)
+
+# Probably not the recommended method, but other ways don't seem to generate libraries in libs/armeabi


Move libs after build?

ehhh naah 😊

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

Android/CMakeLists.txt

+
+add_library(AndroidNative jni/app-android.cpp)
+add_dependencies(tolua_native lua_native)
+add_dependencies(AndroidNative tolua_native)


Why does the androidNative lib depend on tolua_native?

I thought it was more like tolua_native depended on AndroidNative. My intention was to get tolua_native and lua_native to actually build.

Then you should add a dependency on tolua_native in src/Bindings/CmakeLists.txt

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/ClientHandle.h

@@ -453,7 +453,12 @@ class cClientHandle  // tolua_export
 		// TODO: Add Kicking here as well
 	} ;
 	
+#ifdef ANDROID
+#pragma message "Android <atomic> support is incomplete or broken - using plain variable (subject to data races!)"
+	eState m_State;


This is not an acceptable solution. If we can't use std::atomic, then we need to setup typedefs to a custom std::atomic implementation using cCriticalSection.

nooooooooooooo

Unless you want to review all the code that uses std::atomic, and come up with alternative implementations, I think a custom minimal atomic implementation is easier.

I don't think you should be bothered about this, considering it may well be fixed anyway in future NDK releases and Android runs at 0.8 chunks per second and so is unlikely to be used as an actual server. I definitely don't have the time to try and re-implement atomic :P

As I said, if thread safety is less important than performance, you should make cCriticalSection a noop. A minimal std::atomic implantation using cCriticalSection would not be much work. It doesn't need to be fully standard complaint, just provide the interfaces we use.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/FastRandom.cpp

-	#define ATTRIBUTE_TLS static __thread
+#if defined (ANDROID)
+	// Tycho: I'm sorry, I'm sorry, I'm so, so, sorry, but please blame the NDK developers for lack of, thread_local
+	#define ATTRIBUTE_TLS static


Really? What about __thread?

Apparently only GCC emulates thread local storage and not Clang (yet), but GCC crashes when compiling Lua.

Also, imagining your expression when you read the downgrading of TLS to just the plain static, and then saying, "really?", makes me laugh :P

Just leaving it as a static is UB so not really an option. So we either need to use atomics (is this possible on android?), the pthread thread local api (slow), or work out some way of using a mutex (slow).

(They are working on TLS support for Clang.)

Unless someone is actually really in need of running a reliable server on their phone, it doesn't matter in my humble opinion.

Why not just replace cCriticalSection with a noop? It will offer a major performance boost on these low power devices? Because we need thread safety. Any instance of a race condition is grounds for the compiler to do whatever it feels like.

I am intending for this to be an experimental thing. Of course there will be bugs, but there are loads of them anyway, and the compiler doesn't seem to be doing anything untoward.

In an ideal situation, everything would be standards compliant, but since I've no time to reimplement nor wish to pollute the main repository with more Android artefacts, it's gonna be a case of buggy mobile builds versus none at all.

Does that sit comfortable on your conscience??

...unless you want to do something code-y about it?

On 21 Dec 2015, at 17:28, "worktycho" notifications@github.com wrote:

In src/FastRandom.cpp:

@@ -8,8 +8,11 @@

#include

-#if defined (GNUC)

#define ATTRIBUTE_TLS static __thread
+#if defined (ANDROID)

// Tycho: I'm sorry, I'm sorry, I'm so, so, sorry, but please blame the NDK developers for lack of, thread_local

#define ATTRIBUTE_TLS static
Why not just replace cCriticalSection with a noop? It will offer a major performance boost on these low power devices? Because we need thread safety. Any instance of a race condition is grounds for the compiler to do whatever it feels like.

—
Reply to this email directly or view it on GitHub.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/FurnaceRecipe.cpp

+			// There is a problem here on Android. Text files transferred from another OS may have a newline representation Android's implementation of getline doesn't expect
+			// Thus, part of a newline may be left in ParsingLine. ::empty() thus thinks the string isn't empty, and the below code outputs interesting errors since it was passed a nearly empty string
+			// Ref: http://stackoverflow.com/questions/6089231/getting-std-ifstream-to-handle-lf-cr-and-crlf
+			// TODO: There is a solution in the above reference, but it isn't very pretty. Fix it somehow.


Solution: Don't build on windows 😄 , but seriously, I'd say that we should use the wrapper from the question if it wasn't for licensing issues. The other short term option is to just tell anyone building the android app on window to set git to use unix line endings. Most text editors can handle it.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/LoggerListeners.cpp

@@ -295,6 +297,27 @@ std::unique_ptr<cLogger::cListener> MakeConsoleListener(bool a_IsService)

 ////////////////////////////////////////////////////////////////////////////////
 // cFileListener:
+#if defined (ANDROID)


Create a separate listener.

awww, do I have to

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/OSSupport/CMakeLists.txt

+		elseif(ANDROID)
+			# Android has neither pthread nor rt
+			target_link_libraries(OSSupport)
+		else()
 			target_link_libraries(OSSupport rt)


Everyone needs event_core and event_extra

And not apple needs pthread.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/OSSupport/File.cpp

+	#elif defined(ANDROID)
+		// Identical to Linux below, but st_mtime is an unsigned long, so cast is needed:
+		auto Time = static_cast<time_t>(st.st_mtime);
+		return static_cast<unsigned>(mktime(localtime(&Time)));


Why not just change the general linux code to be this?

Do compilers complain if you cast to the same type?

I don't think so. But test it.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/OSSupport/NetworkInterfaceEnum.cpp

 	#include <IPHlpApi.h>
 	#pragma comment(lib, "IPHLPAPI.lib")
-#else  // _WIN32
+#elif !defined(ANDROID)  // _WIN32
 	#include <sys/types.h>


How are you including networking headers on andriod?

Liberal application of magic.

Meaning? Is it just that this file isn't actually doing anything?

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/OSSupport/NetworkInterfaceEnum.cpp

@@ -121,7 +121,7 @@ AStringVector cNetwork::EnumLocalIPAddresses(void)
 {
 	AStringVector res;


So you're just breaking this function on android?

If you are going to break it, at least update the API docs.

worktycho · Dec 20, 2015

worktycho reviewed Dec 20, 2015

View changes

worktycho · Dec 20, 2015

src/Protocol/MojangAPI.cpp

@@ -584,7 +584,7 @@ void cMojangAPI::SaveCachesToDisk(void)
 	try
 	{
 		// Open up the SQLite DB:
-		SQLite::Database db("MojangAPI.sqlite", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE);
+		SQLite::Database db(FILE_IO_PREFIX "MojangAPI.sqlite", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE);


What about the SQLite lua bindings? Don't they need the prefix as well?

@tigerw

@tigerw I would recommend that you keep this one closed, re-write your branch to not generate an apk, instead binaries, the NDK can do that 😛. Then open a new pr

Schwertspize · Apr 24, 2016

@tigerw I would recommend that you keep this one closed, re-write your branch to not generate an apk, instead binaries, the NDK can do that 😛. Then open a new pr

Converted to android-cmake for APK creation
* Various changes to allow for Android to compile successfully

Loading status checks…

62c4f36

sphinxc0re closed this Mar 30, 2016

tigerw referenced this pull request Apr 23, 2016
Closed
Crosscompiling cuberite with the NDK #3161

tigerw added a commit to cuberite/lua that referenced this pull request May 29, 2016

Prepared Lua for cuberite/cuberite#2526

e86fc01

tigerw referenced this pull request Jun 11, 2016
Merged
Converted to android-cmake for Android building #3230

cuberite/cuberite

Join GitHub today

Converted to android-cmake for APK creation #2526

Conversation

tigerw commented Oct 5, 2015

madmaxoft reviewed Oct 5, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tigerw commented Oct 5, 2015

sphinxc0re reviewed Oct 5, 2015 View changes

This comment has been minimized.

sphinxc0re reviewed Oct 5, 2015 View changes

This comment has been minimized.

sphinxc0re reviewed Oct 5, 2015 View changes

This comment has been minimized.

sphinxc0re commented Oct 5, 2015

This comment has been minimized.

tigerw commented Oct 5, 2015

This comment has been minimized.

worktycho commented Oct 5, 2015

worktycho reviewed Oct 5, 2015 View changes

This comment has been minimized.

worktycho commented Oct 5, 2015

This comment has been minimized.

tigerw commented Oct 6, 2015

This comment has been minimized.

tigerw commented Oct 6, 2015

This comment has been minimized.

worktycho commented Oct 6, 2015

This comment has been minimized.

tigerw commented Oct 8, 2015

This comment has been minimized.

worktycho commented Oct 8, 2015

This comment has been minimized.

sphinxc0re commented Nov 5, 2015

This comment has been minimized.

sphinxc0re commented Nov 13, 2015

This comment has been minimized.

tigerw commented Nov 21, 2015

This comment has been minimized.

tigerw commented Dec 20, 2015

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

worktycho reviewed Dec 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

madmaxoft reviewed Oct 5, 2015

View changes

sphinxc0re reviewed Oct 5, 2015

View changes

sphinxc0re reviewed Oct 5, 2015

View changes

sphinxc0re reviewed Oct 5, 2015

View changes

worktycho reviewed Oct 5, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes

worktycho reviewed Dec 20, 2015

View changes