search

yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.45k stars 1.57k forks source link

Do not load Discord if CPU ISA is not supported #4151

Closed MayeulC closed 3 years ago

MayeulC commented 3 years ago

Describe the bug Basically https://github.com/yairm210/Unciv/issues/1624 and https://github.com/yairm210/Unciv/issues/3283, also similar to https://github.com/yairm210/Unciv/issues/2361

When running on a CPU Instruction Set Architecture (or just a platform) not supported by Discord, loading the library fails, which crashes the process.

Exception in thread "main" java.lang.UnsatisfiedLinkError: Unable to load library 'discord-rpc': Native library (linux-aarch64/libdiscord-rpc.so) not found in resource path (/app/data/Unciv.jar)
        at com.sun.jna.NativeLibrary.loadLibrary(NativeLibrary.java:303)
    at com.sun.jna.NativeLibrary.getInstance(NativeLibrary.java:427)
    at com.sun.jna.Library$Handler.<init>(Library.java:179)
    at com.sun.jna.Native.loadLibrary(Native.java:569)
    at com.sun.jna.Native.loadLibrary(Native.java:544)
    at club.minnced.discord.rpc.DiscordRPC.<clinit>(DiscordRPC.java:42)
    at com.unciv.app.desktop.DesktopLauncher.tryActivateDiscord(DesktopLauncher.kt:193)
    at com.unciv.app.desktop.DesktopLauncher.main(DesktopLauncher.kt:64)

The above is the output of the flatpak ran on a pinephone with postmarketos. The architecture is aarch64 in this case.

To Reproduce

  1. Run the game on some non-x86 platform
  2. Alternatively, you could run java in qemu (using pmbootstrap from postmarketos for instance), or delete the shared libraries from the jar to reproduce on a computer.

Expected behavior Discord is not loaded if it can't be, but the game continues.

Additional context I had a look at the code. With all due respect, this is a terrible implementation :)

https://github.com/yairm210/Unciv/blob/e0d7128bc627f725f94233af08545bc8ec79e7dd/desktop/src/com/unciv/app/desktop/RaspberryPiDetector.kt#L20-L22

This will only detect Raspbian (which I run on only one of the tens of ARM devices that I have at hand). For instance, it will not run on a raspberrypi running Archlinux, nor postmarketos/alpine/etc.

The issue here is that it won't load a native library on the wrong microarchitecture. Instead of querying the OS name, the proper way would be to query the architecture (with os.arch it seems) and not load Discord if it is not part of the supported architectures (which are probably something like amd64 and i686/i386).

This will make it possible to run unciv on a lot of other architectures, from aarch64 (armv8), armv7, POWER, but also more esoteric things like RISC-V, Itanium, SPARC and whatnot :)

Another possible implementation is to try to load discord, catch any errors, log them and continue as if nothing happened. This would be my favorite implementation, I think. I would also allow disabling discord from an environment variable for testing purposes, and those that don't want to load Discord.

This is the section that would need a try/catch statement: https://github.com/yairm210/Unciv/blob/434136e6cc2d11b082c3937df9d0e15d0b87fcc3/desktop/src/com/unciv/app/desktop/DesktopLauncher.kt#L63-L64

Catching exceptions can also future-proof the binary a bit against changing APIs and issues loading Discord.

SomeTroglodyte commented 3 years ago

I hate Unciv hammering the network interface all the time in purely single player games anyway... I vote for 100% opt-in!

But - say - "disabling discord from an environment variable" - is that a feature of discord's libraries? Or would we have to do that ourselves (in which case gameSettings.json might be the nicer place anyway - how to access that before the game is initialized is in the Android portrait code)?

SomeTroglodyte commented 3 years ago

One could also delay the activation until the game knows it may help - i.e. an actual multiplayer game is joined/started. Then try-catch->Toast. One would have to understand Android MultiplayerTurnCheckWorker first and why there isn't a platform-agnostic interface for the usecase (catch->Toast would also need some interface to core code)...

More architecturally challenging - new link between platform-specific stuff and core code. And before that - is there something functionally equivalent in Android (yes) -> unify code...

MayeulC commented 3 years ago

I hate Unciv hammering the network interface all the time in purely single player games anyway... I vote for 100% opt-in!

I dislike closed-source code so I would prefer opt-in, but I understand many user prefer the convenience of not having to change anything. Also... Wouldn't it just "hammer" the local interface? If so, that's not a big deal, really, especially if there is no listener.

But - say - "disabling discord from an environment variable" - is that a feature of discord's libraries? Or would we have to do that ourselves (in which case gameSettings.json might be the nicer place anyway - how to access that before the game is initialized is in the Android portrait code)?

Not that I know, that would need to be done in Unciv. It should be quite simple, however:

Boolean.parseBoolean(System.getEnv("UNCIV_NO_DISCORD"))

Note that this only works if UNCIV_NO_DISCORD is set to true. Ideally it should also support 1. Settings are a good place, but it's pretty standard for the decision tree to be:

The idea being that specific and temporary config is easier to apply on-the-fly, and that should take precedence. But if that's not a mechanism Unciv already uses, I don't want to impose it on you.

yairm210 commented 3 years ago

For all settings, Unciv currently sports the first two and not the second two. That could be a separate issue, but I think 'adding a new strong' and 'adding a new setting input method' are definitely different subjects.

SomeTroglodyte commented 3 years ago

My "sorry no" on environment variable and command line would be: We're multiplatform including mobile. On Android these things exist, but a normal user doesn't know or have any kind of control over them, right?

I shouldn't try to code this - that part is not transparent to me and I couldn't test properly. Unless - the options switch would be fairly simple - except adding another line to that screen would compel me to redesign it with something like OverviewScreen/Civilopedia - style tabs or that ExpanderTab widget currently only used once...

yairm210 commented 3 years ago

@MayeulC The thing is, there IS a try/catch inside of that function, but that doesn't stop the crashes. I have a feeling that this is due to using the import at the top of the file, which tries to load the library before reaching that line. I'm changing this to a direct-reference implementation and we'll see if that solves it :)

yairm210 commented 3 years ago

Ah no, sorry, the problem seems to be in a different place. The code there uses a native library load not ON A LINE OF CODE, as it were, but WHEN INITIALIZING THE STATIC VARIABLE which is "outside" the lines of code. A possible solution is to try and load that library ourselves, before accessing the static instance.

SomeTroglodyte commented 3 years ago

Ideas:

Can't offer more for now as I can't force the (expletive) thing to fail. Either the compiler fails and I can't debug or it loads the lib from gradle cache...

yairm210 commented 3 years ago

Nice catch with the image

SomeTroglodyte commented 3 years ago

... the marvel green goblin?

stephanepare commented 1 year ago

I am having this issue on Raspbian buster 32 bits, exactly the OS which this is trying to detect. Raspberry pi 4b with 8gb ram

pi@raspberrypi:~/games/unciv $ java -jar ./Unciv.jar Packing textures - 206ms [LwjglApplication] Couldn't initialize audio, disabling audio java.lang.UnsatisfiedLinkError: /tmp/libgdxpi/e06d3531/liblwjgl.so: /tmp/libgdxpi/e06d3531/liblwjgl.so: cannot open shared object file: No such file or directory (Possible cause: can't load IA 32-bit .so on a ARM-bit platform) at java.base/java.lang.ClassLoader$NativeLibrary.load0(Native Method) at java.base/java.lang.ClassLoader$NativeLibrary.load(ClassLoader.java:2445) at java.base/java.lang.ClassLoader$NativeLibrary.loadLibrary(ClassLoader.java:2501) at java.base/java.lang.ClassLoader.loadLibrary0(ClassLoader.java:2700) at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2630) at java.base/java.lang.Runtime.load0(Runtime.java:768) at java.base/java.lang.System.load(System.java:1837) at org.lwjgl.Sys$1.run(Sys.java:70) at java.base/java.security.AccessController.doPrivileged(Native Method) at org.lwjgl.Sys.doLoadLibrary(Sys.java:66) at org.lwjgl.Sys.loadLibrary(Sys.java:96) at org.lwjgl.Sys.(Sys.java:117) at org.lwjgl.openal.AL.(AL.java:59) at com.badlogic.gdx.backends.lwjgl.audio.OpenALLwjglAudio.(OpenALLwjglAudio.java:72) at com.badlogic.gdx.backends.lwjgl.LwjglApplication.createAudio(LwjglApplication.java:282) at com.badlogic.gdx.backends.lwjgl.LwjglApplication.(LwjglApplication.java:90) at com.badlogic.gdx.backends.lwjgl.LwjglApplication.(LwjglApplication.java:71) at com.unciv.app.desktop.DesktopLauncher.main(DesktopLauncher.kt:58) Exception in thread "LWJGL Application" java.lang.NoClassDefFoundError: Could not initialize class org.lwjgl.Sys at org.lwjgl.opengl.Display.(Display.java:135) at com.badlogic.gdx.backends.lwjgl.LwjglGraphics.setVSync(LwjglGraphics.java:643) at com.badlogic.gdx.backends.lwjgl.LwjglApplication$1.run(LwjglApplication.java:125)`

yairm210 commented 1 year ago

Nope, what you have is a completely different error, causes by libgdx and not Discord at all I can only assume that Libgdx does not support your architecture/OS, and therefore neither can Unciv

MayeulC commented 1 year ago

@stephanepare, the issue is not with Discord but liblwjgl.

See #4943, your issue is similar to that one. This is a bit surprising, as I think it should work on that arch. Maybe only with 64 bit distributions?

stephanepare commented 1 year ago

@stephanepare, the issue is not with Discord but liblwjgl.

See #4943, your issue is similar to that one. This is a bit surprising, as I think it should work on that arch. Maybe only with 64 bit distributions?

That would be a new development then, one which is becoming more and more prevalent unfortunately. The whole linux world is moving away from 32 bit, but most of the better known stuff on the pi is struggling to migrate to 64bit. In theory, can I assume that the included libgdx libs stopped supporting armhf relatively recently and loading an older version of the game might be fine?

MayeulC commented 1 year ago

@stephanepare, I am pretty sure it should work, you might have better luck interacting on that other issue, it's a bit off-topic here. Unfortunately, I won't have the bandwidth to look into that soon, but I suggest asking other people who have tried to make it run on a similar platform.

armhf is becoming less common, but I think I had issues running unciv on aarch64 too recently. Maybe that library fails to load for some other reason, or the wrong library is selected. A good first step is to extract the jar (it's just a fancy zip file), and see if the .so files are here, and there's one for your arch (the command file) can help you here.

I really suggest you answer in #4943 though.