-
Notifications
You must be signed in to change notification settings - Fork 24
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
Added Windows support. Migrated on CMake to build native libraries. GitHub Actions + GitHub Packages Registry. #3
base: master
Are you sure you want to change the base?
Conversation
Hi @bbaldino, Yura. |
thanks @mstyura this is obviously a lot to take in and i've just about totally evicted all context of the maven/jni hell that this project involved from my brain so it will take me a while to go through it. |
Thanks @bbaldino, I completely understand it's not easy to get into jni build procedure after not touching it for a while, but I hope I made things actually simpler. As a starting point could you please follow an updated README.md from my fork and let me know if you have any issue on your environment? I personally followed original readme and it helped me a lot, thank you very much for documenting build procedure! |
hey @mstyura, haven't forgotten about this just haven't gotten to it yet. |
Hello, @bbaldino, |
@mstyura i'm really sorry, i just haven't had the time. i promise you i haven't forgotten (i have a reminder every week in my email for this), but i just haven't gotten to it. jonathan is also pretty busy as we're pushing to get jvb 2.0 fully released and stable. since we don't have any commits going into this repo, really, at least we don't have to worry about 'rot' and this becoming outdated soon. |
@bbaldino thank you very much for a quick reply, don't worry that this PR is not been reviewed yet. I didn't want to make a pressure on you. I'm not blocked on it and can continue without it for a few month I think. Have a good day! |
Any news? |
@ZeWaren could you please try building |
Any news on the merge? This makes the bridge work fine on ARM64 ;) |
With recent changes I've altered builds scripts to take into account different CPU architectures on various supported OSes |
@licaon-kter could you please check if latest changes to this PR still works for you? I've taken into account different CPU architectures per supported OS, but don't have anything with ARM to check myself if it works. |
@mstyura builds and runs fine for me on RPi3+ arm64 👍 Thanks for the effort. |
One question, trying to compile this on CentOS 6.10, I have to remove I've edited the local maven files to remove the "dirty" part and then it succeeded. Is this expected on this old platform or my changes triggered it? |
@licaon-kter the durty was added because you've edited |
Ignoring old distros, any blockers for this? I need to replace JARs after each DEB upgrade now.... :( |
Thanks @mstyura , builds for me now. Another thought I had: I've often regretted that there isn't a good way to know which usrsctp hash is used. Does this maintain that anywhere? I'm wondering if we should include the usrsctp hash somewhere. |
Yes! With submodule you are know exactly which version is used by
|
Ah ha! I just read that it does. Great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some doc comments--haven't finished yet
README.md
Outdated
### Building the jar files | ||
* Clone the project | ||
* Run `mvn package` (and `mvn install` to install locally) | ||
* The `usrsctp` module handles the compilation of the [`usrsctp`](https://github.com/sctplab/usrsctp). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: lost some words here:
The `usrsctp` module handles the compilation of the [`usrsctp`](https://github.com/sctplab/usrsctp)
should be
The `usrsctp` module handles the compilation of the [`usrsctp`](https://github.com/sctplab/usrsctp) source library.
README.md
Outdated
* Clone the project | ||
* Run `mvn package` (and `mvn install` to install locally) | ||
* The `usrsctp` module handles the compilation of the [`usrsctp`](https://github.com/sctplab/usrsctp). | ||
This maven module produces platform specific artifact having pre-compiled `usrsctp` static library and corresponding `C` API-header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wording:
This maven module produces a platform-specific artifact containing a platform-specific build of the `usrsctp` static library and corresponding `C` API-header.
README.md
Outdated
* Run `mvn package` (and `mvn install` to install locally) | ||
* The `usrsctp` module handles the compilation of the [`usrsctp`](https://github.com/sctplab/usrsctp). | ||
This maven module produces platform specific artifact having pre-compiled `usrsctp` static library and corresponding `C` API-header. | ||
The module is build only when `build-usrsctp` profile is enabled, e.g. `-Pbuild-usrsctp` switch is passed to Maven. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module is built only when the build-usrsctp
profile is enabled by passing the -Pbuild-usrsctp
switch to mvn
.
README.md
Outdated
`-- SctpJni.class | ||
``` | ||
* The `jniwrapper-native` module contains the `C` portion of the JNI API that bridges the Java and the [`usrsctp`](https://github.com/sctplab/usrsctp) native lib. | ||
The module is build only when `build-jnisctp` profile is enabled, e.g. `-Pbuild-jnisctp` switch is passed to Maven. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build
-> built
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I'd phrase the second bit as: "...only when the build-jnisctp
profile is enabled by passing the -Pbuild-jnisctp
switch to mvn
."
* The `jniwrapper-native` module contains the `C` portion of the JNI API that bridges the Java and the [`usrsctp`](https://github.com/sctplab/usrsctp) native lib. | ||
The module is build only when `build-jnisctp` profile is enabled, e.g. `-Pbuild-jnisctp` switch is passed to Maven. | ||
It depends on two other modules: | ||
* `usrsctp` module, because it needs the pre-built `usrsctp` static library and include headers. The version of `usrsctp` artifact used is specified by property `usrsctp_commit_id` in `jniwrapper/pom.xml` and having short commit hash of pre-built `usrsctp`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is usrsctp_commit_id
populated dynamically to the checked-out usrsctp hash when building everything at once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbaldino no, right now it is "hard-coded" in the jniwrapper/pom.xml
as a property.
@mstyura haven't made it through the readme in depth entirely yet, and I think after that I'll go through the poms a bit but seems like it's on a good track. I'm wondering, though, if we should include some bash scripts to make the common use cases easier. Something that could take in a usrsctp hash, for example, and just build everything. Thoughts? |
I could add simple bash script to build library from scratch for a current platform, which might be convenient for a developers - but it will basically |
Any news? |
8b3fe39
to
ed84714
Compare
Move to cmake-based usrsctp build. Put static library into predictable directorly. Updated assembly descriptor. Updated assembly descriptor to properly handle cmake's single configuration generators like make as well as multi-configuration generators like VS and XCode. Use target/ directory for CMake compilation. Remove redundant entries from .gitignore. Fixed file path's included in zip. Build jnsctp (jni wrapper) with CMake. Use OS alias as destination directory. Fixed JNI library name. Added support of Windows into Sctp JNI loader. Install pdb along with dll. Specify build plugin versions in root pom. Updated usrsctp submodule. Make some CMake settings platform dependent. Move CMake flags and generator props into root pom Fixed CMake option names. Implemented platform specific artifacts. No longer need to store pre-compiled JNI libraries inside this Git repo. Fixed maven_platform_classifier. Fixed usrsctp assembly definition. Debugging CMake variable definitions on Linux. Fixing CMake's user provided variables. Removed quotes around paths. Install PDB only on Windows. Added missing groupId. Fixed MacOS X jar classifier. Fixed profile id to match other id convention. Minor pom rearrangements. Use pom packaging for native artifacts. Updated README to describe updated build procedure Corrections to README. Use pom packaging for jnilib. Use root pom with project to build specified. Workaround for GCC 9 issue. Fix make dependencies flag. Updated readme. Fixed compilation when usrsctp localed in deep dir. Updated README. Remove warning switch when build on mac. Minor fixed after self-review. Fix in README. Minor comment fix after self-review. Fixed usrsctp CMake binary directory. Include usrsctp commit id into artifact classifier Use javac -h instead of deprecated javah. https://openjdk.java.net/jeps/313 Fixed typos in readme after self review. Updated README.md. Use more stable usrsctp version. Disable unsupported warning on Mac Don't define __FUNCTION__=__func__ on Windows. Added example content of fat jar. Do nothing on install when usrsctp ins not build. Skip install target when nothing usefull is built. Undefine INET6/INET which are not needed and cause crash due to miscompilation Adds FreeBSD support. Define properties for freebsd profile. Updated README.md. Activate profile explicitly via -P switch. Use ${project.groupId} to refer to org.jitsi. maven: updated plugins verions. Added os-maven-plugin to more precise os detection. use maven platform classifier based on os-maven-plugin Renamed usrsctp_install_dir into usrsctp_cmake_install_dir delete pre-built libjnisctp.so for freebsd from git (moved to maven) jniwrapper/native: renamed cmake related properties. put freebsd profile next to linux. only include linux x86_64 and osx x86_64 into "official" fat jar. fixed native library loader to take into account CPU architecture. Updated readme to include cpu arcitecture in maven classifier. bring back freebsd and windows into fat jar to match readme. jnilib: exclude transitive dependencies of jnilib which are embedded into jar. Fix library load on mac Updated readme according to code review feedback. Disable invariants to avoid crashes due asserts. Fixed to readme according to code review.
* Trying github actions. * Fixing projects lists. * Attempt * Enable native build profiles. * Checkout submodule * Submodules. * Attempt to deploy. * Publish 2 * Attemp to fix deploy. * Attempting deploy fix. * Noop * Attemp to fix badrequest * Noop * Verbose maven. * Disable http * Change to username. * Full logs. * Adding settings. * Update version in pom.xml * Remove unnecessary switch. * Print git info. * Git fetch tags. * Git info. * Fetch all history. * Git fetch all tags. * Removed git info step. * Attemp to fix deploy of artifact with classifier. https://github.community/t5/GitHub-API-Development-and/GitHub-package-registry-as-Maven-repo-trouble-uploading-artifact/m-p/56826/highlight/true#M4777 * Deploy only one project. * Attempt to fix double upload. * Attempt to enable wagon. * Attempt to fix double-deploy. * Enable profile * Switch typo. * explicit install step. * Attach. * Attempt to join steps. * Dedicated deploy step. * Remove deploy. * Build jniwrapper-native. * Enable osx build * use api token * Disable deploy step. * Force bash shell. * Bash. * Trying upload step. * Fix upload * Name artifacts. * Download uploaded artifacts. * Fixed syntax. * Fixed sytax. * Attempt to build java. * Install. * Fixed find. * Pass file into install-file * Upload m2. * Workaround for non-expanded path. * Workaround * Attempt to build xplat. * fixed typo * Don't native deploy libraries, they are deployed being embedded into jnilib.jar * Attempt to avoid usage of personal account name. * Fetch entire history to make describe work. * Enable GitHub Actoin only on specific branches. * Deploy only if pushed into master * Fixed copy-paste.
@licaon-kter I thinks this PR is currently blocked on unresolved issue with GitHub packages: there is no way to download artifact without authentication token, which is unfortunate for OSS projects: https://github.community/t/download-from-github-package-registry-without-authentication/14407 |
It might be possible to workaround authentication by not using GitHub Packages and using some other Maven repos hosting instead to publish pre-built packages. Maybe maven centeral? |
All the other stuff is in some maven, I don't understand why this needs Github. I want this and jitsi-srtp to be multi-platform since (at least on arm64) I'm stuck with these versions and I might end up incompatible with JitsiMeet elsewhere if more time passes. |
@licaon-kter I'm completely for putting pre-built library into existing maven https://github.com/jitsi/jitsi-maven-repository from GitHub Actions. Maybe that's even possible, need to consult with someone from Jitsi Team who is aware of publishing of existing repository. Maybe @damencho or @bbaldino know if it's possible (and how ) to publish to existing maven repo from GH Action runner? |
I don't think that this is how that repository is managed in the first place. If I recall correctly, there was a build server that's pushing things into that repository (unless things have changed since I last was involved). At the time, it was a matter of having configured a proper job in that server, which is indeed where you'd need the help of one of the Jitsi dev team. |
That has now happened. Jitsi Meet mobile has stopped working with JVB1. Openfire Meetings is now stuck with JVB1 and cannot move to JVB2 until this issue is fixed. |
Hey, can you add more detail on what prevents you from upgrading? |
JVB2 does not run in Windows because of the this issue
I can't start making changes to the Openfire JVB plugin without a working JVB |
You should migrate to using websockets, as sctp will be gone at some point: https://jitsi.github.io/handbook/docs/devops-guide/faq#how-to-migrate-away-from-multiplexing-and-enable-bridge-websockets |
I just updated JVB2 to latest code and rebuilt in Windows 10 and this time it worked. The SCTP exception on health checking which caused this PR has gone.
Thank you @bgrozev and @damencho for responding 👍 I will get starting on upgrading the JVB Openfire Plugin to JVB2 |
For the record, the exception that caused this PR was
|
Note that unless you configure web-sockets you will probably see a similar exception when a conference is created, and certainly there will be missing functionality (e.g. following the active speaker, or selecting a participant to view in high quality by clicking on them). |
Everything working fine with websockets instead of sctp on Windows until I tried upgrading to JVB latest stable build 2.0.5390
Is this a regression or am I missing a new parameter somewhere?
|
You can now disable it in the config of jitsi-videobridge ( |
I'm currently trying to migrate to
JVB 2.0
, but my usual dev environment isWindows
. I've noticed thatjitsi-sctp
is no longer supportWindows
after migration to dedicatedMaven
artifact. So I've added support ofWindows
.The summary of changes:
CMake
as it provides cross platform way to compile native libraries.usrsctp
andjniwrapper-native
artifacts are now platform specific and distinguished by corresponding maven classifier. E.g.usrsctp-1.0-SNAPSHOT-windows-x86_32.jar
,usrsctp-1.0-SNAPSHOT-linux-aarch_64.jar
,jniwrapper-native-1.0-SNAPSHOT-x86_64.jar
etc.git
as resources, they stored inmaven
as artifact with platform classifier.