Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABCce8bfb5331ba: [SECP256k1] Add the CMake/Ninja build to Travis
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
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.Wed, Jan 8, 15:07
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Jan 8, 15:07

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.Wed, Jan 8, 17:29
This revision now requires changes to proceed.Wed, Jan 8, 17:29
Fabien updated this revision to Diff 15256.Thu, Jan 9, 08:15

Address feedback.

deadalnix requested changes to this revision.Thu, Jan 9, 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.Thu, Jan 9, 11:35
Fabien planned changes to this revision.Thu, Jan 9, 15:33
Fabien edited the summary of this revision. (Show Details)Thu, Jan 9, 15:52
Fabien updated this revision to Diff 15276.Thu, Jan 9, 16:07

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.Thu, Jan 9, 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.Thu, Jan 9, 16:41
Fabien requested review of this revision.Thu, Jan 9, 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.

Fabien updated this revision to Diff 15289.Fri, Jan 10, 12:14

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.
Fabien updated this revision to Diff 15290.Fri, Jan 10, 12:17

Add mistakenly deleted space back.

deadalnix accepted this revision.Fri, Jan 10, 13:14
This revision is now accepted and ready to land.Fri, Jan 10, 13:14
This revision was automatically updated to reflect the committed changes.