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
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- cmake_jni
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 7468 Build 12979: Bitcoin ABC Buildbot (legacy) Build 12978: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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 . |
Comment Actions
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 ↗ | (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. |
| 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:
|
Comment Actions
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 ? |
Comment Actions
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. |