Page MenuHomePhabricator

[secp256k1] remove guava dep
ClosedPublic

Authored by floreslorca on Mar 12 2019, 00:53.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC31f5ed8c61ea: [secp256k1] remove guava dep
Summary

remove overkill depencency to guava

Test Plan

execute previous tests successfully

the JNI part can be built and tested by simply doing

$ ./autogen.sh
$ ./configure --enable-jni --enable-experimental --enable-module-ecdh
$ make check-java

Once https://reviews.bitcoinabc.org/D2686 is passed we can do this automatically

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

floreslorca created this revision.Mar 12 2019, 00:53
Owners added a reviewer: Restricted Owners Package.Mar 12 2019, 00:53
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 12 2019, 00:53
Herald added a subscriber: schancel. · View Herald Transcript
floreslorca edited the test plan for this revision. (Show Details)Mar 12 2019, 01:05
floreslorca edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Mar 12 2019, 20:01

Can we get the java tests fixed before changes are made rather than mix the two together ?

This revision now requires changes to proceed.Mar 12 2019, 20:01

Also, adding a test of the java binding to CI would be beneficial so that we can catch these things.

floreslorca edited the test plan for this revision. (Show Details)Mar 13 2019, 20:34
floreslorca requested review of this revision.Mar 20 2019, 06:09
Fabien added a subscriber: Fabien.Mar 20 2019, 13:24

There seem to be several things in this diff, one is removing the guava dependencies and the others are refactoring (indent and boolean assignations).
You should consider splitting the diff.

floreslorca updated this revision to Diff 7769.Mar 21 2019, 04:40

remove indentation

Fabien requested changes to this revision.Mar 21 2019, 08:57

There is some mix with the previous version in the diff, it does not display the difference from master. Could you please rebase on top of master ?

This revision now requires changes to proceed.Mar 21 2019, 08:57
floreslorca updated this revision to Diff 7796.Mar 22 2019, 20:32

rebase master

huh i did rebase but it still not showing the proper diff

Fabien added a comment.Mar 22 2019, 20:36

Did you rebased a single commit ?

floreslorca updated this revision to Diff 7798.Mar 22 2019, 21:15

remove guava dep in a single commit

floreslorca updated this revision to Diff 7801.Mar 22 2019, 22:44

remove guava in build script

Fabien accepted this revision.Mar 25 2019, 10:48
deadalnix added inline comments.Mar 25 2019, 16:36
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java
42 ↗(On Diff #7801)

AssertFailException seems like a more appropriate exception.

deadalnix accepted this revision.Mar 25 2019, 16:38
deadalnix added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java
42 ↗(On Diff #7801)

Never mind, the name makes it explicit this is about arguments.

This revision is now accepted and ready to land.Mar 25 2019, 16:38
This revision was automatically updated to reflect the committed changes.