Page MenuHomePhabricator

[CMAKE] Allow building secp256k1 as a standalone project
ClosedPublic

Authored by Fabien on Tue, Jan 7, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Tue, Jan 7, 16:00
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Jan 7, 16:00
deadalnix accepted this revision.Tue, Jan 7, 16:08

An update for travis is also in order.

This revision is now accepted and ready to land.Tue, Jan 7, 16:08
deadalnix requested changes to this revision.Tue, Jan 7, 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.Tue, Jan 7, 16:09
deadalnix added inline comments.Tue, Jan 7, 16:10
src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15195)

Ok, append deals with it.

deadalnix added inline comments.Tue, Jan 7, 16:13
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.Tue, Jan 7, 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.Tue, Jan 7, 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.Tue, Jan 7, 16:20
deadalnix added inline comments.Tue, Jan 7, 16:20
src/secp256k1/CMakeLists.txt
8 ↗(On Diff #15195)

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

Fabien added inline comments.Tue, Jan 7, 16:24
src/secp256k1/CMakeLists.txt
9 ↗(On Diff #15195)

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

Fabien edited the summary of this revision. (Show Details)Tue, Jan 7, 16:41
Fabien updated this revision to Diff 15200.Tue, Jan 7, 16:41

Rebase on top of D4846.

deadalnix accepted this revision.Tue, Jan 7, 17:20
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.Tue, Jan 7, 17:20
Fabien updated this revision to Diff 15202.Tue, Jan 7, 17:47

Merge comments.

This revision was automatically updated to reflect the committed changes.

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

Fabien added a comment.Tue, Jan 7, 20:53

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