Page MenuHomePhabricator
Differential D5092

[SECP256K1] CMake: make the GMP bignum support optional
ClosedPublic
Actions

Authored by Fabien on Jan 28 2020, 20:01.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Commits
rSTAGINGe73e3496d400: [SECP256K1] CMake: make the GMP bignum support optional
rABCe73e3496d400: [SECP256K1] CMake: make the GMP bignum support optional
Summary

It is disabled by default.

Test Plan

Run the Travis build (see
https://travis-ci.org/Fabcien/secp256k1/builds/642963633).

Diff Detail

Repository
rABC Bitcoin ABC
Branch
secp256k1_cmake_bignum
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9178
Build 16308: Default Diff Build & Tests
Build 16307: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 28 2020, 20:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 28 2020, 20:01
teamcity added a comment.Jan 28 2020, 20:01

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 28 2020, 20:06

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9178: Diff 15851.Jan 28 2020, 20:06
Fabien planned changes to this revision.Jan 28 2020, 21:19
Fabien updated this revision to Diff 15856.Jan 28 2020, 21:22

Put BIGNUM=no at the right place, where there is no libgmp.

teamcity added a comment.Jan 28 2020, 21:23

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 28 2020, 21:27

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9180: Diff 15856.Jan 28 2020, 21:27
Fabien added a child revision: D5094: [SECP256K1] Travis: pass extra flags to the CMake build.Jan 28 2020, 21:58
jasonbcox requested changes to this revision.Jan 28 2020, 23:22
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/secp256k1/travis/build_cmake.sh
8 ↗(On Diff #15856)

This change doesn't appear to be related and seems to increase code duplication. What's its purpose?

This revision now requires changes to proceed.Jan 28 2020, 23:22
Fabien requested review of this revision.Jan 29 2020, 06:55
Fabien added inline comments.
src/secp256k1/travis/build_cmake.sh
8 ↗(On Diff #15856)

What the toolchain file does is basically 2 things:

  1. Setting the -m32 flag
  2. Redirecting the library/header search path to the static dependencies (depends dir)

Since secp256k1 has no depends the second point makes no sense, and would prevent cmake from finding libgmp:i386 on the OS file system. Simply adding the -m32 flag is enough and solves the issue.

deadalnix accepted this revision.Jan 29 2020, 14:01
jasonbcox accepted this revision.Jan 29 2020, 16:49
This revision is now accepted and ready to land.Jan 29 2020, 16:49
Fabien updated this revision to Diff 15871.Jan 29 2020, 17:01

Rebase.

teamcity added a comment.Jan 29 2020, 17:01

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 29 2020, 17:06

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9188: Diff 15871.Jan 29 2020, 17:06
Closed by commit rABCe73e3496d400: [SECP256K1] CMake: make the GMP bignum support optional (authored by Fabien). · Explain WhyJan 29 2020, 17:07
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABCe73e3496d400: [SECP256K1] CMake: make the GMP bignum support optional.
Fabien added a commit: rSTAGINGe73e3496d400: [SECP256K1] CMake: make the GMP bignum support optional.Apr 7 2020, 22:41

Revision Contents
Changeset List

PathSize
src/
secp256k1/
4 lines
7 lines
travis/
10 lines

Diff 15851

View Options

src/secp256k1/.travis.yml

Loading...
View Options

src/secp256k1/CMakeLists.txt

Loading...
View Options

src/secp256k1/travis/build_cmake.sh

Loading...