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

[API] Fix native loading by stopping creating tmpDir every time #197

Merged
merged 9 commits into from
Sep 18, 2023

Conversation

kusaanko
Copy link
Contributor

Description

imgui-java tries delete the native library when JVM shutdowns, but JVM locks the native library until JVM shutdowns, so deleteOnExit() will fail.

Instead of creating a temporary directory, imgui-java creates a directory that depends on its version on tmp dir of OS. This enables us to launch different versions of imgui-java at the same time.
This way is used by LWJGL.

Fixes #182

Type of change

  • Minor changes or tweaks (quality of life stuff)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@kusaanko
Copy link
Contributor Author

Oh I found this works in example code, but do not works in external project. I'll fix this so please wait.

@kusaanko
Copy link
Contributor Author

I fixed it.

@SpaiR
Copy link
Owner

SpaiR commented Aug 29, 2023

This one is really good!

Yet I think we can do it a little bit easier. With all imgui-java jars we provide manifest build.gradle#L21-L30 Which already containes field Implementation-Version with basically what you've added in the properties file. To avoid data duplication and additional logic around it you can try to strip up the logic around props file and modify your code to get the info from MANIFEST.MF file.

@SpaiR SpaiR added the feat New feature or request label Aug 29, 2023
@kusaanko
Copy link
Contributor Author

I think we should not use MANIFEST.MF file. If you make a fat jar, the MANIFEST.MF would not be included.
And there are someone who delete META-INF/ in order to modify jar files and disable signature. imgui-java.It may be better if imgui-java.properties created in imgui/ directory.

@SpaiR
Copy link
Owner

SpaiR commented Aug 29, 2023

I think we should not use MANIFEST.MF file. If you make a fat jar, the MANIFEST.MF would not be included. And there are someone who delete META-INF/ in order to modify jar files and disable signature. imgui-java.It may be better if imgui-java.properties created in imgui/ directory.

Good call. Yeah, in that case let's keep it in generated properties.

imgui-binding/build.gradle Show resolved Hide resolved
imgui-binding/build.gradle Outdated Show resolved Hide resolved
imgui-binding/build.gradle Outdated Show resolved Hide resolved
imgui-binding/build.gradle Outdated Show resolved Hide resolved
@kusaanko
Copy link
Contributor Author

Thank you for your nice advice!

@kusaanko
Copy link
Contributor Author

I think imgui-java should load the native library in this order, how about you?

  1. From imgui.library.path
  2. From java.library.path
  3. From resources

@kusaanko kusaanko requested a review from SpaiR August 29, 2023 14:48
@SpaiR
Copy link
Owner

SpaiR commented Sep 1, 2023

I think imgui-java should load the native library in this order, how about you?

  1. From imgui.library.path
  2. From java.library.path
  3. From resources

Sounds reasonable 👍

@kusaanko
Copy link
Contributor Author

kusaanko commented Sep 1, 2023

And I think it should be added checking hash of native library. If user set imgui.library.path and put modified native library in it, it might be a danger.

1.imgui.library.path
2.java.library.path
3.extract from resources
@kusaanko kusaanko requested a review from SpaiR September 7, 2023 10:23
@SpaiR
Copy link
Owner

SpaiR commented Sep 10, 2023

I'll do merge on the next week. With it I plan to make a release, so if you are willing to do any other changes to the library - I'll glad to review them. 👍

@SpaiR SpaiR merged commit c46830a into SpaiR:main Sep 18, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: every launch copies the binary DLL into Windows Temp dir
2 participants