Page MenuHomePhabricator

[CMAKE] Add support to build secp256k1 JNI binding and tests
ClosedPublic

Authored by Fabien on Aug 14 2019, 12:00.

Details

Summary

This allow for building the secp256k1 JNI binding from CMake, as well as
running the java test as part of the check-all command.
The JNI binding is disabled by default and require the ECDH module to be
enabled. If the JNI binding is to be built, a shared secp256k1_jni library
is also built.

Test Plan

On Linux:

mkdir buildcmake && cd buildcmake

The default check-all behavior is unaffected:

cmake -GNinja ..
ninja check-all

Check in the output that the java tests are not run.

ls ./src/secp256k1 | grep ".so" # Should return nothing

JNI requires the ECDH module:

rm -rf *
rm -rf *

Should return a CMake error:

The secp256k1 JNI support requires ECDH.  Try again with
  -DSECP256K1_ENABLE_MODULE_ECDH=ON.

The check-all target runs the java tests when JNI is enabled:

rm -rf *
cmake -GNinja .. -DSECP256K1_ENABLE_JNI=ON \
  -DSECP256K1_ENABLE_MODULE_ECDH=ON
ninja check-all

Check in the output that the java tests are run.

ls ./src/secp256k1 | grep ".so" # Should return libsecp256k1_jni.so

It also works when built as standalone:

cd ../src/secp256k1
mkdir buildcmake && cd buildcmake
cmake -GNinja .. -DSECP256K1_ENABLE_JNI=ON \
  -DSECP256K1_ENABLE_MODULE_ECDH=ON
ninja secp256k1-check-java

Check in the output that the java tests are run.

ls . | grep ".so" # Should return libsecp256k1_jni.so

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_jni
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7417
Build 12877: Bitcoin ABC Buildbot
Build 12876: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 14 2019, 12:00
deadalnix requested changes to this revision.Aug 27 2019, 22:52
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
169 ↗(On Diff #10781)

You should avoid defining variable for content that is only used once I makes everything harder to read.

201 ↗(On Diff #10781)

If you always define that target, as suggested in the previous diff, this whole logic becomes much simpler.

261 ↗(On Diff #10781)

You need to be using add_test_to_suite .

This revision now requires changes to proceed.Aug 27 2019, 22:52
Fabien updated this revision to Diff 11160.Sep 9 2019, 15:12
Fabien edited the summary of this revision. (Show Details)

Always build the shared library when JNI is enabled.
This allows for using the CMake built-in BUILD_SHARED_LIBS switch.
No longer depends on D3870 as a consequence.

Fabien added inline comments.Sep 9 2019, 15:16
src/secp256k1/CMakeLists.txt
261 ↗(On Diff #10781)

This facility only works with executables, not with custom commands/targets.

I considered adding a new function to manage custom targets but the command is not a CMake property (you cannot copy it from the target to the test definition), so it doesn't provide any factorization in the end.

deadalnix requested changes to this revision.Sep 11 2019, 11:54
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
172 ↗(On Diff #11160)

secp256k1-shared is a very bad name as it is clearly not libsecp256k1 as a shared lib, but some bridging code for JNI.

174 ↗(On Diff #11160)

Isn't it dangerous? Now we have two lib with the same output name, no? Why is that needed?

200 ↗(On Diff #11160)

Do not define single use variables.

227 ↗(On Diff #11160)

Fix indetation

235 ↗(On Diff #11160)

dito

237 ↗(On Diff #11160)

This should be using add_test_to_suite.

This revision now requires changes to proceed.Sep 11 2019, 11:54
Fabien updated this revision to Diff 11240.Sep 12 2019, 12:07

Address comments, make the build succeed when building a shared lib.

Fabien updated this revision to Diff 11241.Sep 12 2019, 12:19

Missing newline

deadalnix requested changes to this revision.Sep 12 2019, 14:08
deadalnix added inline comments.
cmake/modules/LibraryHelper.cmake
3 ↗(On Diff #11241)

This is the exact reverse as what cmake libraries typically do.

4 ↗(On Diff #11241)

Why is that necessary ? Also, this doesn't seem to be a generally useful function as properties aren't applied transitively. I'd be surprised if cmake was unable to figure out it need to build PIC code for SHARED lib dependencies.

src/secp256k1/CMakeLists.txt
172 ↗(On Diff #11241)

Build another library just like configure does.

198 ↗(On Diff #11241)

This is not used

This revision now requires changes to proceed.Sep 12 2019, 14:08
Fabien added inline comments.Sep 12 2019, 19:40
cmake/modules/LibraryHelper.cmake
4 ↗(On Diff #11241)

The shared library will be built with PIC as cmake will add it automatically because of being shared, but it does not apply to the dependencies.
Even when creating a shared from an object library you still need to set it manually.

src/secp256k1/CMakeLists.txt
198 ↗(On Diff #11241)

This is used internally by add_jar, see https://cmake.org/cmake/help/v3.5/module/UseJava.html

237 ↗(On Diff #11160)

I answered this comment here: https://reviews.bitcoinabc.org/D3871?id=10781#inline-24281:

This facility only works with executables, not with custom commands/targets.
I considered adding a new function to manage custom targets but the command is not a CMake property (you cannot copy it from the target to the test definition), so it doesn't provide any factorization in the end.

Fabien edited the summary of this revision. (Show Details)Sep 12 2019, 19:44
Fabien edited the test plan for this revision. (Show Details)
Fabien edited the summary of this revision. (Show Details)
Fabien updated this revision to Diff 11258.Sep 12 2019, 19:47
Fabien edited the summary of this revision. (Show Details)

Simplify by rebasing on top of D4058.
Using a separated secp256k1_jni lib makes the logic much simpler.

deadalnix requested changes to this revision.Sep 13 2019, 10:55
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
175 ↗(On Diff #11258)

Please add a comment as to why this is needed. I suspect there is a proper way to achieve this - as this is not transitive so it fails in the general case - but we can live with this now.

234 ↗(On Diff #11258)

Why is this target duplicated with two different names ?

This revision now requires changes to proceed.Sep 13 2019, 10:55
Fabien updated this revision to Diff 11323.Sep 14 2019, 17:19

Add a comment regarding the POSITION_INDEPENDENT_CODE property.

src/secp256k1/CMakeLists.txt
234 ↗(On Diff #11258)

I want at a time a target -so I can run the test individually if I want to- and having the test running as part of more global check-secp256k1 as well as check-all. The test name is not a target.

deadalnix requested changes to this revision.Sep 14 2019, 17:23
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
234 ↗(On Diff #11258)

Then you create a target and set the dependencies properly.

This revision now requires changes to proceed.Sep 14 2019, 17:23
Fabien updated this revision to Diff 11325.Sep 14 2019, 19:10

Don't have a test being a duplicate of a target.

Fabien updated this revision to Diff 11326.Sep 14 2019, 19:18

Rename the target to be consistent with other tests.

deadalnix accepted this revision.Sep 14 2019, 19:21
This revision is now accepted and ready to land.Sep 14 2019, 19:21