Use default property values to work on Graal.

[tommyettinger]

Graal Native Image seems to not define java.runtime.name, which means without a default, isAndroid would call .contains("Android") on null, and crash. Adding defaults for System.getProperty() seems like a vital step for anywhere that gets String properties. This fixes #7111 . Thanks to authorZhao for finding the bug and suggesting a fix.

[Berstanio]

Isn't it maybe the better solution, to force UIUtil to run the static initializer at build time, not at run time?
Should be safe to do since native-image can't be cross compiled and therefor should provide meaningful values.
Maybe might also be a good time to set up a native-image.properties file for libGDX.

[tommyettinger]

Isn't it maybe the better solution, to force UIUtil to run the static initializer at build time, not at run time? Should be safe to do since native-image can't be cross compiled and therefor should provide meaningful values.

But UIUtils is also used by non-Graal environments that are cross-platform; that's its whole point for existing. So no, it isn't safe to do it at build time.

Maybe might also be a good time to set up a native-image.properties file for libGDX.

Sure, but I don't know what that is, so PR away, just not here. This is a fix for an NPE that happens in UIUtils because it made the incorrect assumption that a specific property would always be defined.

[Berstanio]

Sorry, I think I haven't phrased myself well!
A native-image has two stages where a static initializer can run, during the AOT compilation and during runtime.
When running during AOT compilation it would infer the properties from the host environment, therefor setting up all the properties depending on the host. This is okay, since the host is always (in the important parts) identical with the compilation target.
When to run the initializer is a argument for the native-image command, so it would have no side effects outside native-image. Here is a article about it: https://www.graalvm.org/latest/reference-manual/native-image/guides/use-system-properties/

The "native-image.properties" file is related to this, because it is the way to automatically configure a native-image execution, therefor automazing passing the "--initialize-at-build-time" argument.
Here is more about it (e.g. the "Args" section): https://www.graalvm.org/latest/reference-manual/native-image/overview/BuildConfiguration/

[tommyettinger]

I mean, last I checked, LWJGL3 needed a lot of hoops to jump through to build any native image, and LWJGL2 wouldn't work at all. The way I remember used a repo by anyicomplex , required a dependency on all of Graal, and had some other drawbacks. If anyone has a convenient way to build native images of libGDX games, that would be really nice to hear about.

[Berstanio]

I have build libGDX games with native-image for MOE (iOS). I also maintain a fork of your mentioned library: https://github.com/Berstanio/gdx-graalhelper
However, we should be able to move that whole thing to the main libgdx repo using the native-image.properties file mentioned. I don't know whether this is wanted, I would be in favor of it though.
The caveat for lwjgl3 seems to be atm the shared libarys, however I haven't looked into this at all so far (I mainly used native-image for iOS so far).
The dependency on some graal libs is btw. compileOnly on libGDX side, so shouldn't do any problems.

[tommyettinger]

I think Graal has some real advantages for games, but like you said, the shared libraries are what's problematic for LWJGL3. Maybe there's a way to have that handled better by native-image.properties, though I want to point out, that is outside this PR's scope. I'll create an issue to bundle up Graal discussion and maybe spread knowledge between the various people who have used native-image.

[Berstanio]

though I want to point out, that is outside this PR's scope.

Yes, agreed. I think I also misunderstood, based on the diff I thought "os.name" also isn't set, but thats not the case, right? If it's really just "java.runtime.name" thats missing, which is only used for determining android, than I guess the empty default string is totally fine imo.

However a little question on the default of "os.name": If it is not set, it will result in silent failure of platform detection, right? I've never heard about a case where "os.name" is not set, but maybe it's still better to add some kind of warning, if it isn't present? Just in case, to not have to deal with potential silent platform detection failure?

[tommyettinger]

If os.name isn't set, then osName becomes "", and then the platform will (I think) detect as iOS because that's the last case that's checked. Before, if os.name wasn't set, osName would have been null, which would have crashed when UIUtils was loaded because it would call .contains() on null.

[Berstanio]

If os.name isn't set, then osName becomes "", and then the platform will (I think) detect as iOS because that's the last case that's checked. Before, if os.name wasn't set, osName would have been null, which would have crashed when UIUtils was loaded because it would call .contains() on null.

Yes, I'm aware. My point is, whether it is a good idea to have "If we don't know, we assume it's iOS" without any warning in the log. I was thinking about something like:

static {
    if (System.getProperty("os.name") == null) {
        System.err.println("Warning: os.name isn't set, host OS can't be determined.");
    }
}

[tommyettinger]

If os.name can be null, that has never happened so far as I'm aware on any OS, since it would have crashed with an NPE when UIUtils was loaded. I believe if someone is running on an obscure/custom-enough OS (that still somehow has a JVM) that os.name is not one of the known ones, then UIUtils not working is probably to be expected. UIUtils only makes sense for graphical applications, based on what it actually does, so server-only OSes aren't going to be a concern for it. For desktop platforms, we only need to be concerned about the platforms that LWJGL (2 and 3) support, all of which seem to define os.name just fine. That leaves a handful of old/rare mobile OSes, like BlackBerry, which I don't think we can currently target.

[Berstanio]

Okay, fair points!
In that case, I'm fine with the changes, not that my approval would have any meaning though!

[tommyettinger]

One thing did just occur to me though -- a novel JVM (like Graal, but maybe even more experimental) might not define os.name somehow, since Graal doesn't define java.runtime.name. It hasn't happened yet, but could -- in that case, the OS would detect as iOS, which might actually be OK. Detecting as iOS currently means the app is running on RoboVM or MOE (or Graal if you have that working), and I think most of those have a subset of the features on a desktop JVM. A brand new experimental JVM would probably also have a subset of features, so detecting a combination of iOS as the OS and some desktop-related property (like having a physical keyboard, maybe) would let you know that this is "some new kind of OS" and that expected Java features might not be present (on iOS or the unknown new OS).

[crykn]

I agree that a default value for java.runtime.name should be set and that part of the PR has my approval!

@Berstanio: However a little question on the default of "os.name": If it is not set, it will result in silent failure of platform detection, right? I've never heard about a case where "os.name" is not set, but maybe it's still better to add some kind of warning, if it isn't present? Just in case, to not have to deal with potential silent platform detection failure?

I think that raises a valid point. Silently failing isn't very nice, even if it is unlikely to ever happen.

[tommyettinger]

I agree that a default value for java.runtime.name should be set and that part of the PR has my approval!

@Berstanio: However a little question on the default of "os.name": If it is not set, it will result in silent failure of platform detection, right? I've never heard about a case where "os.name" is not set, but maybe it's still better to add some kind of warning, if it isn't present? Just in case, to not have to deal with potential silent platform detection failure?

I think that raises a valid point. Silently failing isn't very nice, even if it is unlikely to ever happen.

It wouldn't be hard to add an isUnknownOS() check now, which would just check if os.name wasn't found at all (or if it were the empty String). I don't know if iOS currently defines os.name, so I wouldn't mess with the working iOS check. That would mean an unknown OS would detect as both iOS and an unknown OS, which seems workable.

[Berstanio]

https://github.com/MobiVM/robovm/blob/c526f714ed2b7f26bd96a62ed6378545da9b73c7/compiler/vm/rt/robovm/java_lang_System.c#L49-L57
iOS should define os.name just fine. (It just seems to incorrectly recognize arm64 simulators as real devices, but thats not relevant)

[tommyettinger]

MOE seems to delegate to the OS to return os.name from uname(), a system call I think. https://github.com/multi-os-engine/libcore/blob/56d9ac94a4788f1ca6cc142702bbf00e81101595/luni/src/main/java/android/system/Os.java#L543-L546 Hopefully that is also a String containing 'iOS'...

[HydrolienF]

It looks good to me.
As osName and jrName are imutable static final String they migth be named as constant OS_NAME and JR_NAME ?

(Having a isUnknownOS() just in case may be good to)

[tommyettinger]

This is listed as a requirement (?) for the next release, and the unknown OS check hasn't ever been needed so far, so maybe the changes to have a default setting should be considered as-they-are-now (with one approval), and the unknown OS check can be discussed later.

[Berstanio]

This is listed as a requirement (?) for the next release, and the unknown OS check hasn't ever been needed so far, so maybe the changes to have a default setting should be considered as-they-are-now (with one approval), and the unknown OS check can be discussed later.

Can't we just just revert the default value for "os.name" instead? The linked issue is only about java.runtime.name, where the default value has way less implications.

[tommyettinger]

This is listed as a requirement (?) for the next release, and the unknown OS check hasn't ever been needed so far, so maybe the changes to have a default setting should be considered as-they-are-now (with one approval), and the unknown OS check can be discussed later.

Can't we just just revert the default value for "os.name" instead? The linked issue is only about java.runtime.name, where the default value has way less implications.

If we don't have a default value for os.name, then the property will be null and will lead to crashes if os.name isn't defined. This has not happened yet and I wouldn't really worry about it, whether the default is in or out. However, allowing null at all for a property that we call contains() on is probably a very bad idea.

On all platforms except Graal Native-Image, this will behave the same as before. With a check for an unknown os.name, and without such a check, that does not affect the rest of the PR. We have not encountered a single case of os.name being undefined.

[Berstanio]

If we don't have a default value for os.name, then the property will be null and will lead to crashes if os.name isn't defined. This has not happened yet and I wouldn't really worry about it, whether the default is in or out. However, allowing null at all for a property that we call contains() on is probably a very bad idea.

Yes, I'm aware and I do think, crashing is the correct behaviour. In my opinion, if os.name unset, the application is an unrecoverable state and therefor should stop executing. This way, developer that get a crash report will also immidiatly see, what went wrong. This isn't the case, with a default value.

[tommyettinger]

Wait, what's unrecoverable about having os.name unset? If an OS and JVM can run without that property being set, that could be intended for some hypothetical OS/JVM combination, and it doesn't prevent anything else from functioning (except that the OS will detect as iOS).

[Berstanio]

Wait, what's unrecoverable about having os.name unset?

Whoops, I always forget that this is not about SharedLibraryLoader code. But looking at SharedLibraryLoader, the app will in any case instantly crash, if os.name is unset.
https://github.com/libgdx/gdx-jnigen/blob/7d596d022bcb5bff7e51dd8a9490794728f95a32/gdx-jnigen-loader/src/main/java/com/badlogic/gdx/utils/SharedLibraryLoader.java#L39-L41
And in SharedLibraryLoader I would argue, that if os.name is unset, it is unrecoverable, since the librarys can't be loaded.

[tommyettinger]

So then this is all moot, since if os.name is unset, the core of libGDX will fail in SharedLibraryLoader -- we'll never encounter it.

[Berstanio]

Yep, seems like it!
In that case, I'm fine with this

[crykn]

I think for consistency's sake we should keep the OS checks the same in SharedLibraryLoader and UIUtils, but if you all agree on the opposite, we can go ahead and merge this anyways.

[Berstanio]

I think for consistency's sake we should keep the OS checks the same in SharedLibraryLoader and UIUtils, but if you all agree on the opposite, we can go ahead and merge this anyways.

Good point, couldn't we just rewrite UIUitls, so it references SharedLibraryLoader.isXX?
This way we only have platform checks in one place

[tommyettinger]

SharedLibraryLoader isn't available on GWT. We could maybe go the other way, and have some core class make these checks and make the results available to UIUtils and SharedLibraryLoader.

[crykn]

As part of #5960 I thought about adding a PlattformUtils class, which would take care of stuff like this.

[Berstanio]

SharedLibraryLoader isn't available on GWT.

Thats not a problem, since GWT has it's own UIUtil class:
https://github.com/libgdx/libgdx/blob/master/backends/gdx-backends-gwt/src/com/badlogic/gdx/backends/gwt/emu/com/badlogic/gdx/scenes/scene2d/utils/UIUtils.java

As part of #5960 I thought about adding a PlattformUtils class, which would take care of stuff like this.

I think this is not a bad idea, but it would take some time to implement, right? If we want to merge this soon, I think the cleanest way is using the results of SharedLibraryLoader for the moment.

[tommyettinger]

OK, I switched to using SharedLibraryLoader, and that does look much more clean.