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

Change ppc64el to ppc64le in jar library paths #49

Closed
wants to merge 1 commit into from

Conversation

fitzsim
Copy link

@fitzsim fitzsim commented Dec 17, 2023

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.

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

Merging #49 (5e2cf7c) into master (a64adcc) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d0db60...5e2cf7c. Read the comment docs.

@ibauersachs
Copy link
Member

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).

@fitzsim
Copy link
Author

fitzsim commented Jan 10, 2024 via email

@ibauersachs
Copy link
Member

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.

@fitzsim
Copy link
Author

fitzsim commented Mar 17, 2024

@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?

@JonathanLennox
Copy link
Member

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 Platform.RESOURCE_PREFIX so it will always look for the file at linux-ppc64le regardless of how os.arch spells the platform name.

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?

@fitzsim
Copy link
Author

fitzsim commented Apr 1, 2024

Hi @JonathanLennox,

First, I upgraded to jitsi-videobridge2 2.3-94-ge80e8683-1:

jar tf /usr/share/jitsi-videobridge/lib/jitsi-srtp-1.1-15-ga19c05a.jar|grep ppc
linux-ppc64el/
linux-ppc64el/libjitsisrtp_1.1.so
linux-ppc64el/libjitsisrtp_3.so

which at runtime, after two participants had joined a meeting, resulted in:

JVB 2024-04-01 15:58:29.910 WARNING: [117] JitsiOpenSslProvider.<clinit>#66: Unable to load jitsisrtp
java.lang.UnsatisfiedLinkError: no jitsisrtp_3 in java.library.path: /usr/java/packages/lib:/usr/lib/powerpc64le-linux-gnu/jni:/lib/powerpc64le-linux-gnu:/usr/lib/powerpc64le-linux-gnu:/usr/lib/jni:/lib:/usr/lib

I overwrote jitsi-srtp-1.1-15-ga19c05a.jar with jitsi-srtp-1.1-18-g06c91ea.jar from https://github.com/jitsi/jitsi-srtp/actions/runs/8512650288, and restarted Jitsi and related services. The new result of the same test was:

JVB 2024-04-01 16:00:43.841 INFO: [111] JitsiOpenSslProvider.<clinit>#52: jitsisrtp successfully loaded for OpenSSL 3

Therefore #52 seems to have fixed the issue, according to my testing.

@fitzsim
Copy link
Author

fitzsim commented Apr 1, 2024

(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.)

@ibauersachs
Copy link
Member

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 Platform.RESOURCE_PREFIX so it will always look for the file at linux-ppc64le regardless of how os.arch spells the platform name.

Looking at this, unfortunately the fix on L269 doesn't apply? Or am I missing something else in that class?
https://github.com/java-native-access/jna/blob/master/src%2Fcom%2Fsun%2Fjna%2FPlatform.java#L304

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?

@JonathanLennox
Copy link
Member

Looking at this, unfortunately the fix on L269 doesn't apply? Or am I missing something else in that class?
https://github.com/java-native-access/jna/blob/master/src%2Fcom%2Fsun%2Fjna%2FPlatform.java#L304

It's a bit spaghetti-ish to be sure, but L304 calls getNativeLibraryResourcePrefix, which at https://github.com/java-native-access/jna/blob/0a33062e1971962ca6b283c397ed1de5cc053d33/src/com/sun/jna/Platform.java#L316 calls getCanonicalArchitecture` which is where L269 lives.

That function fixes the bare "ppc64" (with no endianness-description) for os.arch, fixing it to "ppc64le".

@ibauersachs
Copy link
Member

Looking at this, unfortunately the fix on L269 doesn't apply? Or am I missing something else in that class?
https://github.com/java-native-access/jna/blob/master/src%2Fcom%2Fsun%2Fjna%2FPlatform.java#L304

It's a bit spaghetti-ish to be sure, but L304 calls getNativeLibraryResourcePrefix, which at https://github.com/java-native-access/jna/blob/0a33062e1971962ca6b283c397ed1de5cc053d33/src/com/sun/jna/Platform.java#L316 calls getCanonicalArchitecture` which is where L269 lives.

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.

@JonathanLennox
Copy link
Member

Looking at this, unfortunately the fix on L269 doesn't apply? Or am I missing something else in that class?
https://github.com/java-native-access/jna/blob/master/src%2Fcom%2Fsun%2Fjna%2FPlatform.java#L304

It's a bit spaghetti-ish to be sure, but L304 calls getNativeLibraryResourcePrefix, which at https://github.com/java-native-access/jna/blob/0a33062e1971962ca6b283c397ed1de5cc053d33/src/com/sun/jna/Platform.java#L316 calls getCanonicalArchitecture` which is where L269 lives.
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.

@ibauersachs
Copy link
Member

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.

@fitzsim
Copy link
Author

fitzsim commented Apr 2, 2024

The ppc64le Temurin releases for 17-LTS and 11-LTS both print ppc64le for os.arch on my Debian 12 machine.

As I remember, in my original comment I was assuming that Ubuntu ppc64el was being actively tested and worked, and therefore I did not want to suggest simply renaming ppc64el to ppc64le. I never actually tested on Ubuntu ppc64 little endian though; I was showing how the script was invoked according to what I found in the build scripts (and minimal steps to reproduce, assuming someone had access to an Ubuntu ppc64 little endian machine).

Can the native GitHub Actions be made to print os.arch, to get a conclusive result for the Ubuntu Natives ppc64el builders?

@fitzsim fitzsim changed the title Ensure both ppc64le and ppc64el library paths in jar Change ppc64el to ppc64le in jar library paths May 24, 2024
@fitzsim
Copy link
Author

fitzsim commented May 24, 2024

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.

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!

@fitzsim
Copy link
Author

fitzsim commented Jun 30, 2024

I updated Jitsi on my machine and I can confirm this was fixed by #52:

$ dpkg -l | grep jitsi-videobridge2
ii  jitsi-videobridge2  2.3-150-gb67f0c0d-1  all  WebRTC compatible Selective Forwarding Unit (SFU)
$ dpkg -S /usr/share/jitsi-videobridge/lib/jitsi-srtp-1.1-18-g98e4c5d.jar
jitsi-videobridge2: /usr/share/jitsi-videobridge/lib/jitsi-srtp-1.1-18-g98e4c5d.jar
$ jar tf /usr/share/jitsi-videobridge/lib/jitsi-srtp-1.1-18-g98e4c5d.jar | grep ppc64
linux-ppc64le/
linux-ppc64le/libjitsisrtp_1.1.so
linux-ppc64le/libjitsisrtp_3.so

Thank you! I am closing this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants