Skip to content
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

Scaling-plan: A scaling plan applied to scale the API for professional use-cases #20

Merged
merged 55 commits into from
Jul 26, 2024

Conversation

pavly-gerges
Copy link
Member

@pavly-gerges pavly-gerges commented Jul 25, 2024

This PR conclusively adds the following to the core API:

  • PlatformPredicates: provides the ability to append propositions as platform variant {OS + ARCH + INSTRUCT_SET}.
  • SystemDetectionListeners: detects the system found and not found.
  • ExtractionListeners: exposes the lifecycle of the extraction process to the user applications.
  • LoadingListeners: exposes the lifecycle of the native loading process to the user applications.

In addition, I have updated the NativeVariant propositions to reflect the OS ARCHS; according to ChatGPT, the available architectural propositions are:

The System.getProperty("os.arch") method in Java returns the operating system architecture. 
Here is a list of possible outputs on various systems and CPUs:

Common Outputs
x86: 32-bit x86 architecture
x86_64: 64-bit x86 architecture, often referred to as amd64
amd64: Another name for x86_64
i386: Another designation for 32-bit x86 architecture
arm: ARM architecture
aarch64: 64-bit ARM architecture
sparc: SPARC architecture
sparcv9: 64-bit SPARC architecture
ppc: PowerPC architecture
ppc64: 64-bit PowerPC architecture
ppc64le: 64-bit PowerPC architecture, little-endian
s390: IBM System/390 architecture
s390x: 64-bit IBM System/390 architecture
riscv32: 32-bit RISC-V architecture
riscv64: 64-bit RISC-V architecture

@pavly-gerges pavly-gerges added documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed core Core API related stuff examples Stressful testing the functionalities labels Jul 25, 2024
@stephengold
Copy link

In addition to the CPUs listed, you may also want to support the 64-bit RISC-V, which must run Java because LWJGL supports it.

@pavly-gerges
Copy link
Member Author

pavly-gerges commented Jul 26, 2024

  • Windows failed with a cryptic exception:

In case this happens again, this exception has been replaced with a FileNotFoundExecption from the FileLocator API as a validation procedure to avoid this cryptic format:

Jul 26, 2024 8:09:54 AM electrostatic.snaploader.NativeBinaryLoader cleanExtractBinary
SEVERE: Extraction has failed!
java.io.FileNotFoundException: File locator has failed to locate the file inside the compression!
        at electrostatic.snaploader.filesystem.FileLocator.validateFileLocalization(FileLocator.java:97)
        at electrostatic.snaploader.filesystem.FileExtractor.extract(FileExtractor.java:110)
        at electrostatic.snaploader.filesystem.ConcurrentFileExtractor.extract(ConcurrentFileExtractor.java:74)
        at electrostatic.snaploader.NativeBinaryLoader.cleanExtractBinary(NativeBinaryLoader.java:311)
        at electrostatic.snaploader.NativeBinaryLoader.loadLibrary(NativeBinaryLoader.java:163)
        at electrostatic.snaploader.examples.TestBasicFeatures.main(TestBasicFeatures.java:77)

It's reproducible by two ways:

  • Trying to locate and extract a file that is not present in a compression.
  • Trying to load a library that is not present in the extraction path with an activated default retry criterion which retries with a clean extraction, hence, the stack will be directed to the former stack, so these are deterministic in order.

EDIT:
This will be available on the next testing version (typically the delta version).

@stephengold
Copy link

you have told me before that you aren't extracting files, so I suppose you have a problem with the windows binaries, either corrupted or not in the correct path.

For automated testing in the jolt-jni project, there's no need to extract files, because they're already present in the filesystem. However, the failures I reported are in the snap-jolt project, which does extraction: https://github.com/stephengold/snap-jolt/blob/f8fc077da82294bd80bbd47da97001101a80bbe6/src/main/java/com/github/stephengold/snapjolt/HelloWorld.java#L59-L74

Sorry for the confusion.

This happens at the FileExtractor level; as the FileLocator component has failed to locate the native library inside the Jar file (the zip entry); as a result of the retry criteria. Make sure that you have valid Windows binaries at the specified extraction path and/or inside the stock Jar for a clean extraction

It turns out there's a valid Windows binary in the platform directory, but its name is incorrect:

$ jar tf jolt-jni-Windows64-0.3.2-DebugSp.jar
META-INF/
META-INF/MANIFEST.MF
windows/x86-64/com/github/stephengold/joltjni.dll

The final component of the path should be "libjoltjni.dll" instead "joltjni.dll".

This mistake doesn't pose any difficulty for JME's NativeLibraryLoader, because the complete path is specified:
https://github.com/stephengold/kk-physics/blob/cfe378bd3eb5de1c96c11e71082514e8a88bf834/library/src/main/java/com/jme3/bullet/util/NativeLibrary.java#L90-L92

JSnapLoader's way (specifying only the platform directory) is superior to that of NativeLibraryLoader in most cases. However, when the extraction fails the JSnapLoader message should include the complete path. That would simplify troubleshooting.

I'll correct the name of the Windows binary in jolt-jni and try again.

@pavly-gerges
Copy link
Member Author

Thanks for this comprehension. So far I will try to provide the NativeDynamicLibrary with an extra constructor that enables the user APIs to assign the full name themselves, that way the issue will be resolved without changes in Jolt-jni building script.

@stephengold
Copy link

No, wait! I got it backward:

  • The names of Linux shared libraries always begin with "lib". For example, "libbulletjme.so".
  • The names of Windows shared libraries usually don't. So "joltjni.dll" is a valid native library name.

@pavly-gerges
Copy link
Member Author

So, should we just selectively remove the prefixed value "lib" before the windows binaries in the NativeDynamicLibrary as a matter of windows DLLs standardizations?

@stephengold
Copy link

So, should we just selectively remove the prefixed value "lib" before the windows binaries in the NativeDynamicLibrary as a matter of windows DLLs standardizations?

Yes, I think so:

--- a/snaploader/src/main/java/com/avrsandbox/snaploader/platform/NativeDynamicLibrary.java
+++ b/snaploader/src/main/java/com/avrsandbox/snaploader/platform/NativeDynamicLibrary.java
@@ -105,8 +105,8 @@
             NativeDynamicLibrary.LINUX_x86_64.library = "lib" + libraryInfo.getBaseName() + ".so";
             NativeDynamicLibrary.MAC_x86.library = "lib" + libraryInfo.getBaseName() + ".dylib";
             NativeDynamicLibrary.MAC_x86_64.library = "lib" + libraryInfo.getBaseName() + ".dylib";
-            NativeDynamicLibrary.WIN_x86.library = "lib" + libraryInfo.getBaseName() + ".dll";
-            NativeDynamicLibrary.WIN_x86_64.library = "lib" + libraryInfo.getBaseName() + ".dll";
+            NativeDynamicLibrary.WIN_x86.library = libraryInfo.getBaseName() + ".dll";
+            NativeDynamicLibrary.WIN_x86_64.library = libraryInfo.getBaseName() + ".dll";
         }
         
         /* Initializes the library jar path to locate before extracting, "null" to use the classpath */

However, it would also be prudent to provide some mechanism for specifying the full name.

@stephengold
Copy link

this evaluates the whole predicate as false, the correction is isX86().

I can confirm that this correction (in the gamma pre-release) solves the issue with MACOS_X86_64:

https://github.com/stephengold/snap-jolt/actions/runs/10114683678/job/27973857208

@pavly-gerges
Copy link
Member Author

I think so:

--- a/snaploader/src/main/java/com/avrsandbox/snaploader/platform/NativeDynamicLibrary.java
+++ b/snaploader/src/main/java/com/avrsandbox/snaploader/platform/NativeDynamicLibrary.java
@@ -105,8 +105,8 @@
             NativeDynamicLibrary.LINUX_x86_64.library = "lib" + libraryInfo.getBaseName() + ".so";
             NativeDynamicLibrary.MAC_x86.library = "lib" + libraryInfo.getBaseName() + ".dylib";
             NativeDynamicLibrary.MAC_x86_64.library = "lib" + libraryInfo.getBaseName() + ".dylib";
-            NativeDynamicLibrary.WIN_x86.library = "lib" + libraryInfo.getBaseName() + ".dll";
-            NativeDynamicLibrary.WIN_x86_64.library = "lib" + libraryInfo.getBaseName() + ".dll";
+            NativeDynamicLibrary.WIN_x86.library = libraryInfo.getBaseName() + ".dll";
+            NativeDynamicLibrary.WIN_x86_64.library = libraryInfo.getBaseName() + ".dll";
         }
         
         /* Initializes the library jar path to locate before extracting, "null" to use the classpath */

Beware that the classes on the current master are internally different from the current branch; the API has recently undergone a lot of changes, but with preservation of the method signatures, so no breaking changes so far.

@pavly-gerges
Copy link
Member Author

Alright, version 1.0.0-delta is now available with the following changelog:

  • FileLocalizingListener interface to bind user APIs to the FileLocator API lifecycle.
  • Validation of the FileLocator streams at the FileLocator levels with the suppression with business exceptions to avoid cryptic formats.
  • NativeDynamicLibrary.<init>(...): allows for full directory path designation by overwriting the basename from LibraryInfo.
  • NativeDynamicLibrary#initPlatformLibrary(LibraryInfo): removed the prefixed value lib for Windows runtime as a part of the standardization of Windows DLL files.

@stephengold
Copy link

I've updated my projects to 1.0.0-delta: both the snap-jolt testbed and also automated tests in the jolt-jni project. All tests are passing.

The loading code in jolt-jni is rather crude. I'd welcome any suggestions you might have for simplifying or refining it:

https://github.com/stephengold/jolt-jni/blob/6d49e4847de148b5cd137bc93c363fb905d8d23b/src/test/java/testjoltjni/TestUtils.java#L240-L263

I'm thinking about how to test jSnapLoader on Linux-on-ARM platforms. I have a 32-bit Raspberry Pi on my desk plus remote access to 64-bit build servers at Travis-CI. Libbulletjme is already built for Linux-on-ARM but jolt-jni currently is not.

@pavly-gerges
Copy link
Member Author

pavly-gerges commented Jul 26, 2024

I have a 32-bit Raspberry Pi on my desk plus remote access to 64-bit build servers at Travis-CI. Libbulletjme is already built for Linux-on-ARM but jolt-jni currently is not.

Well, if you have any TestBullets techdemos (on Libbulletjme or even jme3-examples), you could plug them on with jSnapLoader. However, I don't know how you will suppress the JME's NativeLibraryLoader...

@pavly-gerges
Copy link
Member Author

pavly-gerges commented Jul 26, 2024

The loading code in jolt-jni is rather crude. I'd welcome any suggestions you might have for simplifying or refining it:

https://github.com/stephengold/jolt-jni/blob/6d49e4847de148b5cd137bc93c363fb905d8d23b/src/test/java/testjoltjni/TestUtils.java#L240-L263

In case you accept contributions, could you activate running the GitHub workflows for PRs against master, please?

So, something like:

# Controls when the workflow will run
on:
  # Triggers the workflow on push or pull request events but only for the "master" branch
  push:
    branches: [ "master" ]
  pull_request:
    branches: [ "master" ]

Then, we could test the changes remotely on GitHub before merging.

@stephengold
Copy link

Good point about suppressing JME's loader.

Luckily, I have many Libbulletjme demos and tests that don't use JMonkeyEngine at all. Instead of extracting native libraries from JARs (as Minie, KK Physics, and jolt-jni do), those apps download native libraries from GitHub Releases via HTTPS (because Libbulletjme natives aren't currently available from Maven Central).

There's an open issue to publish Libbulletjme natives to MavenCentral. If I solved that, I could use Libbulletjme to test extraction.

Ideally I should both solve stephengold/Libbulletjme#12 and publish Linux-on-ARM natives for jolt-jni. Then I could test jSnapLoader extraction on Linux-on-ARM using both libraries. But it's difficult to predict how long those changes might take. A week or two, maybe.

Are you willing to delay the stable release of jSnapLoader v1.0 by a couple weeks, to allow for additional testing?

@pavly-gerges
Copy link
Member Author

Are you willing to delay the stable release of jSnapLoader v1.0 by a couple weeks, to allow for additional testing?

Yes, sure, I am not on a hurry. Since, the delta has proven efficiency, the stable release will not add that much, so we could wait until full testing is done.

@stephengold
Copy link

could you activate running the GitHub workflows for PRs against master, please?

I've done so, both in jolt-jni and snap-jolt.

@pavly-gerges
Copy link
Member Author

I've done so, both in jolt-jni and snap-jolt.

Thanks! I propose to merge this PR, and continue on forums.

@pavly-gerges pavly-gerges merged commit e7a839c into master Jul 26, 2024
9 checks passed
@pavly-gerges
Copy link
Member Author

@stephengold Your contributions are much appreciated! Thank you!

@pavly-gerges pavly-gerges deleted the scaling-plan branch July 26, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core API related stuff documentation Improvements or additions to documentation enhancement New feature or request examples Stressful testing the functionalities help wanted Extra attention is needed
Projects
None yet
2 participants