We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Created attachment 1908 [details]
patch for Android interface version

Here is a patch for a possible solution.

It adds an "interface version" in SDLActivity.java and SDL_android.c which needs to be edited manually for every change and is compared when initializing SDL. If the values from both parts are not equal a warning will be logged. From this it should be clear that a file was not updated. No message box is shown and execution continues because it is only a warning. The value is passed around to prevent querying it using JNI.

The advantage of this solution is that nothing changes for the users. They neither need additional programs or scripts nor change their way to update SDL.

The disadvantage of this solution is that it means more work for the maintainers. They need to remember editing the values or set up a Mercurial hook locally to warn them. It is no automatic solution.

That sounds good. We should have big comments at the top of both files that describe the interface version and its purpose. Since it should reflect incompatibilities between different versions of the Java and the C code, it should show a message box and exit if they don't match.

Looks ok to me, one nitpick would be calling the define "SDL_JNI_VERSION"

Created attachment 1914 [details]
patch for SDL JNI versioning on Android

Here is a new patch with message box, exit and the more descriptive constant name.

The check now in Java because the exit and the message box are there. An exit from Java is more useful because of the more documented clean up. Also doing the check in C and then messaging back to Java (because the exit is there) would have needed an additional message channel. The return value of nativeInit() was not used for this because it is the return value of main() and may therefore already be in use by some applications. But the recently added message box for not loaded libraries is used. Having everything in Java also means that the version does not need to be passed around and one file less needs to be modified.

A new disadvantage is that the actual version check will not work until the Java file was updated at least once (because the check is inside).

Hi, 

What about a more simple solution in SDL/Android.mk, that would just prevent the build:


SDL_JNI_VERSION_C_SIDE=$(shell cat $(LOCAL_PATH)/src/core/android/SDL_android.c | grep JNI_VERSION )
SDL_JAVA_FILE=$(shell find src | grep SDLActivity.java)
SDL_JNI_VERSION_JAVA_SIDE=$(shell cat $(SDL_JAVA_FILE) | grep JNI_VERSION)

ifeq ($(SDL_JNI_VERSION_JAVA_SIDE),$(SDL_JNI_VERSION_C_SIDE))
  $(info Ok: JNI Version API)
else
  $(error Error: JNI Version API mismatch. Update your Java file: $(SDL_JAVA_FILE))
endif

With a keyword JNI_VERSION more unique.

(In reply to Sylvain from comment #5)
> Hi, 
> 
> What about a more simple solution in SDL/Android.mk, that would just prevent
> the build:
> 
> 
> SDL_JNI_VERSION_C_SIDE=$(shell cat
> $(LOCAL_PATH)/src/core/android/SDL_android.c | grep JNI_VERSION )
> SDL_JAVA_FILE=$(shell find src | grep SDLActivity.java)
> SDL_JNI_VERSION_JAVA_SIDE=$(shell cat $(SDL_JAVA_FILE) | grep JNI_VERSION)
> 
> ifeq ($(SDL_JNI_VERSION_JAVA_SIDE),$(SDL_JNI_VERSION_C_SIDE))
>   $(info Ok: JNI Version API)
> else
>   $(error Error: JNI Version API mismatch. Update your Java file:
> $(SDL_JAVA_FILE))
> endif
> 
> With a keyword JNI_VERSION more unique.

This is a really a better idea. It also prevents overhead at runtime and confusing in the source files.

Does it work on Windows (if this is still a requirement)? There used to be no cat and grep (without Cygwin which is not required by NDK).

A disadvantage may be that it does not help if the compiled Java class files are outdated. The version is read from the source which may already be okay.

The NDK does seem to require Cygwin:

Required development tools

For all development platforms, GNU Make 3.81 or later is required. Earlier versions of GNU Make might work but have not been tested.
A recent version of awk (either GNU Awk or Nawk) is also required.
For Windows, Cygwin 1.7 or higher is required. The NDK will not work with Cygwin 1.5 installations.

Reference: http://developer.android.com/tools/sdk/ndk/index.html

(In reply to Gabriel Jacobo from comment #7)
> The NDK does seem to require Cygwin:
> 
> ...
> 
> Reference: http://developer.android.com/tools/sdk/ndk/index.html

You are right. Then using it on Windows at all should be no problem.

I did not read the requirements before my claim and just wrongly assumed it worked without Cygwin because it worked for me the last time I used it on Windows.

In the reference you linked under "Android NDK, Revision 7 (November 2011)" it says that building "on Windows without Cygwin" works but is still experimental. So that seems the reason why it worked for me.

One problem with this is that we're getting lots of patches for the Java file that won't automatically increment the interface version.

Maybe we should just write the major and minor version into the Java file, and check to make sure that matches SDL_version.h?

The other issue is that people aren't necessarily building SDL in-app, so we may not find the application Java file.

In my opinion the best solution would be to remove the need to copy the java file to user projects. In normal use cases you should not need to modify the class anyway, so adding it to your own project only creates these out-of-sync issues. Also I think the class should be made abstract so that the users would be forced to extend it in their own class. 

Then the SDL base class would be referenced by the user projects either from a jar library or preferably SDL as a whole would be packaged to an android archive (AAR): https://developer.android.com/studio/projects/android-library.html

Users can still override the important stuff in their implementation class and if they really need to modify the SDL stuff they need to do it the old way and copy the class or edit it in SDL source.

The thing is that while you can include native libraries in the AAR already, it doesn't support including native header files properly yet, but it's in the works: https://issuetracker.google.com/issues/37086680

This way there would be a precompiled distribution for android that would use AAR instead of  making everybody compile from source. I thought about testing it out and doing a patch, but I'm waiting for the native library support issues to be solved first.

Another related issue is the outdated sample project that could then be then updated to just use gradle and the AAR: (https://bugzilla.libsdl.org/show_bug.cgi?id=3510)

Created attachment 3781 [details]
android-project/installNewSdlLibVersion.sh

+1 what Olli suggests

In the meantime I wrote a shell script that makes installing a new lib pretty easy (attached installNewSdlLibVersion.sh goes in android-project folder and should be run from there).

It does the following:
    verifies arguments
    clears app/build/intermediates to make sure your build uses new version
    updates the symlink to the SDL library code
    copies the src/main/java/org/libsdl/app/ java files replacing all
    displays a diff of AndroidManifest.xml (which needs to be manually merged)

It's suppose to be harmless to run with the current setting (when the args define current symlink).

./installNewSdlLibVersion.sh SDL /full/path/to/libs/SDL-2.0.9-12723

For me it works with or without trailing slash (only tested on osx).  Should work fine to upgrade other symlinks too.

I guess its worth pointing out keeping this manifest file up to date is also important when upgrading, which is arguably also part of this issue.

Created attachment 3782 [details]
android-project/installNewSdlLibVersion.sh

Created attachment 3783 [details]
android-project/installNewSdlLibVersion.sh

diff the build.gradle files too