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

Migrate from libsignal-protocol-java to libsignal #565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vanitasvitae
Copy link
Member

This PR moves away from libsignal-protocol-java which was archived in February of 2022 and instead makes use of libsignal which provides java bindings.

The migration would require a minimum Android API level bump, since the libsignal code now uses java.util.Optional, which is Java 8 (and some Android API level higher than the current min).

I have not done any interoperability- or integration testing yet.
It is also not yet clear, if the ecosystem (OMEMO next) will fork libsignal, or maintain a fork of libsignal-protocol-java instead, so consider this more a feasibility analysis for now :)

@Flowdalic
Copy link
Member

@vanitasvitae we are in the process of release the next version of Smack and it seems like this PR would be a good candidate for the next version. Could I bribe you into rebasing this PR onto the latest master while addressing the sensible suggestions @stokito gave? Thanks in advance :)

@Flowdalic Flowdalic added the Needs rebasing All or some commits need to be squashed label Oct 5, 2024
@stokito
Copy link
Contributor

stokito commented Oct 5, 2024

my comments are not important at all, just some styling. I'll remove them to not spent a time even on reading them

@Flowdalic
Copy link
Member

I'll remove them to not spent a time even on reading them

I am sorry, but this is counter productive. The points you raised are valid and should be addressed before this is merged. For example, the new error prone version will flag the usages of LinkedList (rightly so, in most cases, if I may say).

Please restore your comments.

Copy link
Contributor

@stokito stokito left a comment

Choose a reason for hiding this comment

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

Thank you!

@Flowdalic
Copy link
Member

Flowdalic commented Oct 6, 2024

Thanks for the late night work @vanitasvitae :)

[Undefined reference] org.jivesoftware.smackx.omemo.signal.(SignalOmemoRatchet.java:87)
java.util.Optional

            if (!preKeyMessage.getPreKeyId().isPresent()) {

Android added j.u.Optional with API level 24. Smack is currently 23 and was, until recently 19 (IIRC). Looking at the Android API distribution charts, I think we can raise the API level once more to 24. Especially since I consider the availability of Optional desirable.

However, I am not yet sure why this check starts to fail now, in code that was not changed in this PR. Needs more investigation, but first lunch… ;)

@vanitasvitae
Copy link
Member Author

vanitasvitae commented Oct 6, 2024

One further note: The PR still uses libsignal-client:0:26.0, which is over a year old as of now.
I don't have the time to check newer releases for breaking changes (e.g. pqc stuff) or important security updates right now, but it may be worthwhile for you to check, if Smack may benefit from a newer release of the library.

@Flowdalic
Copy link
Member

Please rebase, Smack's minimum Android SDK level is now 26. That should fix the compilation issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs rebasing All or some commits need to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants