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
rSTAGING31f5ed8c61ea: [secp256k1] remove guava dep
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
Branch
guava
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5207
Build 8477: Bitcoin ABC Buildbot (legacy)
Build 8476: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 12 2019, 00:53
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.

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.

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

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

Did you rebased a single commit ?

remove guava dep in a single commit

remove guava in build script

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Util.java
42 ↗(On Diff #7801)

AssertFailException seems like a more appropriate exception.

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.