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.
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGINGdc9787703f2f: [CMAKE] Add support to build secp256k1 JNI binding and tests
rABCdc9787703f2f: [CMAKE] Add support to build secp256k1 JNI binding and tests
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 Passed - Unit
No Test Coverage - Build Status
Buildable 7142 Build 12329: Bitcoin ABC Buildbot (legacy) Build 12328: arc lint + arc unit
Event Timeline
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.
src/secp256k1/CMakeLists.txt | ||
---|---|---|
261 | 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. |
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. |
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 |
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. |
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:
|
Simplify by rebasing on top of D4058.
Using a separated secp256k1_jni lib makes the logic much simpler.
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 ? |
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. |
src/secp256k1/CMakeLists.txt | ||
---|---|---|
234 ↗ | (On Diff #11258) | Then you create a target and set the dependencies properly. |