Page MenuHomePhabricator

[CMAKE] Allow building secp256k1 as a standalone project
ClosedPublic

Authored by Fabien on Jan 7 2020, 16:00.

Details

Summary

This updates the module path to match the directory structure from
https://github.com/Bitcoin-ABC/secp256k1.

Depends on D4846.

Test Plan

Build as part of the bitcoin-abc project and standalone from the github
repo.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cmake_secp256k1_standalone_module_path
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 8768
Build 15518: Default Diff Build & Tests
Build 15517: arc lint + arc unit

Event Timeline

An update for travis is also in order.

This revision is now accepted and ready to land.Jan 7 2020, 16:08
deadalnix requested changes to this revision.Jan 7 2020, 16:09
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15195)

Wait, why is this removed?

9 ↗(On Diff #15195)

That part seems unecessary.

This revision now requires changes to proceed.Jan 7 2020, 16:09
src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15195)

Ok, append deals with it.

src/secp256k1/CMakeLists.txt
9 ↗(On Diff #15195)

More on that, you can selectively build secp256k1 while running cmake on the whole project. This does not seems to be serving a use case, and can do very strange stuff on the standalone version, as there may be modules there from some other random project on that filesystem.

Fabien requested review of this revision.Jan 7 2020, 16:17

Travis is my next item, I need the build to succeed first :)

src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15195)

It is not, I just switched to list(APPEND).

9 ↗(On Diff #15195)

This is needed for building secp256k1 standalone from the bitcoin-abc source tree.
Maybe the use case is no longer very relevant since the lib is available as it's own project, but I kept it since it doesn't harm.
If you want me to remove it I'll better do it in another diff, as there is a test for this on the CI.

deadalnix requested changes to this revision.Jan 7 2020, 16:20
deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
9 ↗(On Diff #15195)

But you can build a standalone version fo it from the abc source tree without it. The only use case it serve is to do so from the abs sourc tree while not running cmake on the rest of the abc project.

This is a solution is search of a problem now that we have a specific repository for it.

This revision now requires changes to proceed.Jan 7 2020, 16:20
src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15195)

Saw that after and did D4845 while I was at it.

src/secp256k1/CMakeLists.txt
9 ↗(On Diff #15195)

Agreed, I'll put a diff to remove this and rebase

deadalnix added inline comments.
src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15200)

Note that you have two comment for one thing, so it' probably a bit redundant. Consider simplifying.

# Add path for custom modules when building as a standalone project
list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake/modules)
This revision is now accepted and ready to land.Jan 7 2020, 17:20

@Fabien Should Cmake build instructions be added to the secp256k1 readme at /src/secp256k1/README.md ?
It currently has the autotools build instructions

@Mengerian Yes I'll update the instructions when adding cmake to the Travis build