-
Notifications
You must be signed in to change notification settings - Fork 37
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
Change ppc64el to ppc64le in jar library paths #49
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #49 +/- ##
=========================================
Coverage 52.81% 52.81%
Complexity 219 219
=========================================
Files 23 23
Lines 1456 1456
Branches 274 274
=========================================
Hits 769 769
Misses 605 605
Partials 82 82 Continue to review full report in Codecov by Sentry.
|
This doesn't work as the resulting jar would depend on the host that it was built on. A proper fix would be either in jitsi-utils to always handle both cases, or to include the .so at both locations (or maybe a symlink, but I'm not sure that works in a jar). |
OK, thank you for reviewing! Next time I update, I will try both these
approaches to fix the issue locally, then submit a patch that works for
me. To which project should I send the pull request?
|
If a symlink works, then I think that would be the best way to go. If that's a dead-end, then jitsi-utils is the right project to create a PR. |
@JonathanLennox fixed a very similar issue in jitsi/jitsi-sctp#20. This jitsi-srtp issue persists as of jitsi-videobridge v2.3-88-g0c54ff7c. Can you give jitsi-srtp the same treatment that @JonathanLennox gave jitsi-sctp? |
For sctp4j I canonicalized the system to always use JNA's spelling of the architecture, "ppc64le" -- see https://github.com/java-native-access/jna/blob/master/src/com/sun/jna/Platform.java#L269 . Jitsi-utils uses JNA's I believe that #52 should do the same thing for jitsi-srtp. @fitzsim , can you try dropping it in place in the jitsi-videobridge lib dir and confirm whether it works? |
Hi @JonathanLennox, First, I upgraded to
which at runtime, after two participants had joined a meeting, resulted in:
I overwrote
Therefore #52 seems to have fixed the issue, according to my testing. |
(I had to do some alternate-browser gymnastics in order to test, due to the (new?) "static/recommendedBrowsers.html" page blocking my particular older version of Firefox. Separately, I will have to figure out how to disable that check.) |
Looking at this, unfortunately the fix on L269 doesn't apply? Or am I missing something else in that class?
|
It's a bit spaghetti-ish to be sure, but L304 calls That function fixes the bare "ppc64" (with no endianness-description) for os.arch, fixing it to "ppc64le". |
Right, but this only checks for the specific OpenJDK bug (which is fixed for a while now), it doesn't normalize ppc64le and ppc64el into one. |
Are there indeed Java platforms which report "ppc64el" as os.name? When I look at the OpenJDK source, the only mention I see of the string "ppc64el" is in documentation of Debian platforms that are supported. It seems like Java is pretty consistent about using "ppc64le" as its os.name. |
According to @fitzsim in the first message of this issue, yes. I would need to verify this with qemu to run Java on such a CPU myself. |
The As I remember, in my original comment I was assuming that Can the native GitHub Actions be made to print |
I take it no one has complained about renaming ppc64el to ppc64le since jitsi/jitsi-sctp#20 was merged. I that is true, then please disregard my comment about ppc64el; it was based on an (apparently invalid or irrelevant) assumption. Can we instead rename ppc64el to ppc64le in jitsi-srtp's library paths? I updated the title of this pull request; let me know if you want me to resubmit it or if you want to redo it (it is probably in need of a rebase by now). Thanks! |
I updated
Thank you! I am closing this pull request. |
On Ubuntu ppc64 little endian systems, the os.arch property is "ppc64el". On Debian systems of the same architecture, the same property is "ppc64le". To handle all cases, the SRTP jar build scripts can include the same library at paths for both architectures.
To reproduce the failure, pass the alternate architecture name to the CMake wrapper script.
On Ubuntu:
mvn compile && resources/ubuntu-cmake.sh ppc64le 11 `pwd` 3 && mvn package
On Debian:
mvn compile && resources/ubuntu-cmake.sh ppc64el 11 `pwd` 3 && mvn package
The package tests will fail with:
java.lang.UnsatisfiedLinkError: no jitsisrtp_3 in java.library.path[...]
The change in this pull request installs the same ppc64 little endian library in the primary and alternate paths. The build can happen on either Ubuntu or Debian and the built jar will work on either.