Skip to content

Conversation

@sgammon
Copy link
Member

@sgammon sgammon commented Jun 8, 2024

Ready for review Powered by Pull Request Badge

Summary

Builds on top of #988 with a better suite of fixes on both macOS and Linux for OS info obtain via OSHI/JNA. Fixes and closes #990.

  • Fixes on macOS
  • Fixes on Linux

Changelog

  • fix: jna/oshi reflection and proxy metadata
  • fix: no stacktrace for host-side error causes in native mode
  • chore: cpu test script
  • chore: update graalvm module pin / detekt baseline

@sgammon sgammon added bug Something isn't working ✋ embargoed PRs and issues that can't proceed because of some non-code condition 🚧 WIP Works-in-progress. Blocks merge labels Jun 8, 2024
@sgammon sgammon added this to the Release R6: Alpha 10 milestone Jun 8, 2024
@sgammon sgammon self-assigned this Jun 8, 2024
@sgammon sgammon added platform:macos Issues relating to macOS platform:linux Issues relating to Linux and removed ✋ embargoed PRs and issues that can't proceed because of some non-code condition 🚧 WIP Works-in-progress. Blocks merge labels Jun 8, 2024
@sgammon sgammon marked this pull request as ready for review June 8, 2024 02:17
@sgammon sgammon enabled auto-merge (rebase) June 8, 2024 02:18
@sgammon sgammon requested a review from a team June 8, 2024 02:20
Comment on lines +174 to +176
SHARED -> if (it.registerJni) { /* Dynamic JNI libraries use JNI to load. */ } else {
nativeLibraries.addDynamicNonJniLibrary(it.name)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a point to having the empty if branch? why not invert the condition and keep the comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darvld good point -- should get compiled out since it.registerJni is known at compile time (this is all build time code anyway) but will catch in the next PR. there is going to be logic in that other branch eventually, i think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the compiler will optimize it out, I was just checking if you had forgotten to add some logic there 👍

Copy link
Member

@darvld darvld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with small comments 👍

@sgammon sgammon force-pushed the fix/jna-os-info branch 2 times, most recently from b18911c to 61a8999 Compare June 8, 2024 05:39
@sgammon sgammon mentioned this pull request Jun 8, 2024
- fix: jna/oshi reflection and proxy metadata
- fix: no stacktrace for host-side error causes in native mode
- chore: cpu test script
- chore: update `graalvm` module pin / detekt baseline

Fixes and closes #990

Signed-off-by: Sam Gammon <[email protected]>
@sgammon sgammon force-pushed the fix/jna-os-info branch from 61a8999 to 54aeaf6 Compare June 8, 2024 05:57
@sgammon sgammon merged commit 55c83e3 into main Jun 8, 2024
@sgammon sgammon deleted the fix/jna-os-info branch June 8, 2024 06:15
This was referenced Jun 18, 2024
@sgammon sgammon mentioned this pull request Mar 11, 2025
64 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working platform:linux Issues relating to Linux platform:macos Issues relating to macOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot load JNA on main

3 participants