Page MenuHomePhabricator

[SECP256k1] Add the CMake/Ninja build to Travis
ClosedPublic

Authored by Fabien on Jan 8 2020, 15:07.

Details

Summary

This adds a CMake build script to be run by Travis.

Since some configuration switches are not part of the CMake options,
only the relevant combinations are used for the CMake builds.

Depends on D4855.

Test Plan

Run the travis build.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I don't think the contrib folder on secp256k1 is meant to be used this way.

src/secp256k1/contrib/travis/build_cmake.sh
3 ↗(On Diff #15224)

We really need a linter to make these consistent. Also, why isn't the linter yelling at you for local related things?

6 ↗(On Diff #15224)

I don't think export here is correct.

deadalnix requested changes to this revision.Jan 8 2020, 17:29
This revision now requires changes to proceed.Jan 8 2020, 17:29
deadalnix requested changes to this revision.Jan 9 2020, 11:35
deadalnix added inline comments.
src/secp256k1/.travis.yml
67 ↗(On Diff #15256)

That should probably be two different build plans.

This revision now requires changes to proceed.Jan 9 2020, 11:35
Fabien planned changes to this revision.Jan 9 2020, 15:33

Use separated build plans for cmake.
Only run the supported configurations on cmake.
Convert EXTRAFLAGS to cmake syntax as CMAKE_EXTRA.

deadalnix requested changes to this revision.Jan 9 2020, 16:41
deadalnix added inline comments.
src/secp256k1/.travis.yml
41 ↗(On Diff #15276)

The whole point of the matrix thing is that you can leverage it to go combinatorial, and you don't need to do it yourself.

84 ↗(On Diff #15276)

Same here, you can just leverage the matrix thing to do the combinatorial for you.

src/secp256k1/travis/build_cmake.sh
12 ↗(On Diff #15276)

That doesn't look like a flag that cmake uses.

This revision now requires changes to proceed.Jan 9 2020, 16:41
Fabien requested review of this revision.Jan 9 2020, 16:51
Fabien added inline comments.
src/secp256k1/.travis.yml
41 ↗(On Diff #15276)

There is no way to expand the matrix if this is not natively supported as a keyword, such as os or compiler.
There are several feature requests for this but it's not implemented yet. This one is the most recent I found but there are several others.

src/secp256k1/travis/build_cmake.sh
12 ↗(On Diff #15276)

It disables the use of the GMP library, since it's a cache variable.
At the moment there is no "--with-bignum" cmake equivalent.

Roll back to previous version. Rationale:
Pros:

  • It improves readability and maintenability a lot.
  • Running duplicated test is free, so it's not a big deal. Once we reach feature parity the duplicated tests will disappear.

Cons:

  • If the autotools buid fails, it can hide a cmake failure since it's the same test plan.

Add mistakenly deleted space back.

This revision is now accepted and ready to land.Jan 10 2020, 13:14