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

Remove Java Native Interface #682

Merged
merged 1 commit into from Feb 10, 2020
Merged

Conversation

@jonasnick
Copy link
Contributor

jonasnick commented Oct 29, 2019

This was discussed in #508. The main reasons are that the existing Java Native Interface (JNI) bindings would need way more work to remain useful to Java developers but the maintainers and regular contributors of libsecp are not very familiar with Java (and evidently are motivated enough to improve the situation). We don't know who relies on these bindings with the exception of ACINQ who have their own fork at https://github.com/ACINQ/secp256k1/tree/jni-embed/src/java (@sstone). Bitcoinj can optionally use the libsecp bindings.

Ideally, there would be a separate repository owned by Java developers with just the bindings. Until this exists, Java developers relying on libsecp can use ACINQs fork or an older commit of libsecp.

@elichai

This comment has been minimized.

Copy link
Contributor

elichai commented Nov 5, 2019

Concept ACK. I think https://github.com/rust-bitcoin/rust-secp256k1 is a success. there's no reason Java/Scala can't have their own. the only way it can also be high quality Java/Scala lib is if it's written by active Java developers.

@jonasnick jonasnick force-pushed the jonasnick:remove-jni branch from fec63f8 to 00d5c20 Nov 5, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 5, 2019

Concept ACK. I've never understood why java was "special" in this regard to have its bindings included with secp256k1 itself.

@sstone

This comment has been minimized.

Copy link

sstone commented Nov 6, 2019

This is fine with us (and not entirely unexpected :)). We'll just update (and probably rework a bit) our own bindings.

@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Nov 6, 2019

This is fine with us (and not entirely unexpected :)). We'll just update (and probably rework a bit) our own bindings.

That's great to hear. I think the cleanest thing in the long term will be to separate the bindings from the core library entirely, i.e., not include a verbatim copy the C files in the repo. That should not to be too hard as the changes in your current branch do not touch these files.

@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Nov 27, 2019

needs rebase

@jonasnick jonasnick force-pushed the jonasnick:remove-jni branch from 00d5c20 to 7d9066a Nov 27, 2019
@jonasnick

This comment has been minimized.

Copy link
Contributor Author

jonasnick commented Nov 27, 2019

rebased

@elichai

This comment has been minimized.

Copy link
Contributor

elichai commented Nov 27, 2019

@schildbach does this affect bitcoinj?
@Christewart does this affect bitcoin-s?

@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Dec 20, 2019

@schildbach does this affect bitcoinj?
@Christewart does this affect bitcoin-s?

It would be great to get a statement from you guys. We don't want to to leave anybody in the rain.

@schildbach

This comment has been minimized.

Copy link

schildbach commented Dec 20, 2019

I don't know if anyone is using libsecp256k1 via bitcoinj. You could ask on the bitcoinj mailing list to find out.

@Christewart

This comment has been minimized.

Copy link

Christewart commented Dec 20, 2019

We have forked and maintain our own jni which can be found here and used as a dependency for other JVM based projects

@sipa

This comment has been minimized.

Copy link
Contributor

sipa commented Dec 29, 2019

Code review ACK 7d9066a, if we're reasonably confident this won't break things for users (or at least, that they can easily migrate to any of the other developed bindings).

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

jonasnick commented Jan 5, 2020

I posted a deprecation notice on the bitcoinj mailing list with a 1 month window. https://groups.google.com/forum/#!topic/bitcoinj/IghsPGbH4YI

Copy link
Contributor

real-or-random left a comment

@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Jan 7, 2020

This PR will "resolve" #379, #466, #508, #517, #608.

@jonasnick jonasnick force-pushed the jonasnick:remove-jni branch from 7d9066a to 642cd06 Jan 8, 2020
@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Jan 8, 2020

ACK 642cd06 I read the diff

@jonasnick

This comment has been minimized.

Copy link
Contributor Author

jonasnick commented Feb 7, 2020

The 1 month window is over and no one complained on the bitcoinj mailing list.

@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Feb 10, 2020

Code review ACK 7d9066a, if we're reasonably confident this won't break things for users (or at least, that they can easily migrate to any of the other developed bindings).

ACK 642cd06 I read the diff, and I verified that the diff to 7d9066a, which has been ACKed by sipa, is only the additonal removal of ax_jni_include_dir.m4

real-or-random added a commit that referenced this pull request Feb 10, 2020
642cd06 Remove Java Native Interface (Jonas Nick)

Pull request description:

  This was discussed in #508. The main reasons are that the existing Java Native Interface (JNI) bindings would need way more work to remain useful to Java developers but the maintainers and regular contributors of libsecp are not very familiar with Java (and evidently are motivated enough to improve the situation). We don't know who relies on these bindings with the exception of ACINQ who have their own fork at https://github.com/ACINQ/secp256k1/tree/jni-embed/src/java (@sstone). Bitcoinj can optionally use the libsecp bindings.

  Ideally, there would be a separate repository owned by Java developers with just the bindings. Until this exists, Java developers relying on libsecp can use ACINQs fork or an older commit of libsecp.

ACKs for top commit:
  real-or-random:
    ACK 642cd06 I read the diff
  real-or-random:
    ACK 642cd06 I read the diff, and I verified that the diff to 7d9066a, which has been ACKed by sipa, is only the additonal removal of ax_jni_include_dir.m4

Tree-SHA512: 9e573f2b01897bd5f301707062b41de53424517b537ce0834d9049d003cfd73fa1bcc024b543256016e4c9a1126f7c7fef559b84dc4914083b5a2d0ad5e57ea8
@real-or-random real-or-random merged commit 642cd06 into bitcoin-core:master Feb 10, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@real-or-random

This comment has been minimized.

Copy link
Contributor

real-or-random commented Feb 10, 2020

Merged, thanks for all the "external" feedback!

This PR will "resolve" #379, #466, #508, #517, #608.

@sstone @Christewart I encourage you to look at these issues and PRs to see if there's anything that can be reused for your bindings.

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

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.