cmake: Make installation optional

CyberTailor commented at 7:53 am on April 9, 2023: contributor

Useful for embedding secp256k1 in a subproject.

CyberTailor force-pushed on Apr 9, 2023

real-or-random commented at 8:02 am on April 9, 2023: contributor

cc @hebasto @theuni

hebasto commented at 8:05 am on April 9, 2023: member

Useful for embedding secp256k1 in a subproject.

I’ve been thinking about the same :)

real-or-random commented at 11:20 am on April 9, 2023: contributor

What’s the rationale behind this? I guess if you’re not using it as a subproject, you can simply omit make install, but that doesn’t work in for subproject because there’s only a single make invocation for the parent project? So if you make install the parent project, you’d automatically get make install here?

CyberTailor commented at 4:14 pm on April 9, 2023: contributor

What’s the rationale behind this?

To call add_subdirectory() just for the static library, not installing it after.

So if you make install the parent project, you’d automatically get make install here?

Yes, unless you use EXCLUDE_FROM_ALL option, but in that case you wouldn’t also get tests.

hebasto commented at 9:13 am on April 10, 2023: member

FWIW, I’ve been maintaining a repo with usage examples since reviewing of #1113.

There is a “subtree” branch there.

There are two options to include a subtree:

add_subdirectory(src/secp256k1) includes all subtree’s targets into a project’s default ones, including tests and an install target.
add_subdirectory(src/secp256k1 EXCLUDE_FROM_ALL) does not include a subtree install target into a project’s default ones. However, the parent project can call install(TARGETS secp256k1).

Here is an example of a similar option in Google’s LevelDB project:

0option(LEVELDB_INSTALL "Install LevelDB's header and library" ON)

theuni commented at 5:50 pm on April 10, 2023: contributor

I believe PROJECT_IS_TOP_LEVEL and friends were introduced to help with this type of logic?

A boolean variable indicating whether the most recently called project() command in the current scope or above was in the top level CMakeLists.txt file.

hebasto commented at 5:54 pm on April 10, 2023: member

I believe PROJECT_IS_TOP_LEVEL and friends were introduced to help with this type of logic?

Indeed. For CMake 3.21+.

real-or-random commented at 7:07 pm on April 10, 2023: contributor

Concept ACK

I believe PROJECT_IS_TOP_LEVEL and friends were introduced to help with this type of logic?

Indeed. For CMake 3.21+.

Would it be a good idea to make the default depend on PROJECT_IS_TOP_LEVEL if the cmake version is at least 3.21?

real-or-random commented at 8:49 am on April 20, 2023: contributor

and needs rebase :)

cmake: Make installation optional

Useful for embedding secp256k1 in a subproject.

47ac3d63cd

CyberTailor force-pushed on Apr 20, 2023

theuni approved

theuni commented at 2:30 pm on April 20, 2023: contributor

ACK 47ac3d63cd5e00a2d50cb489461c8bc349d37912.

Would it be a good idea to make the default depend on PROJECT_IS_TOP_LEVEL if the cmake version is at least 3.21?

Would prefer to see this done here, but no need to block this PR I guess.

real-or-random approved

real-or-random commented at 3:37 pm on April 20, 2023: contributor

utACK 47ac3d63cd5e00a2d50cb489461c8bc349d37912

hebasto approved

hebasto commented at 3:38 pm on April 20, 2023: member

ACK 47ac3d63cd5e00a2d50cb489461c8bc349d37912, tested on Ubuntu 23.04.

real-or-random commented at 3:41 pm on April 20, 2023: contributor

Would it be a good idea to make the default depend on PROJECT_IS_TOP_LEVEL if the cmake version is at least 3.21?

Would prefer to see this done here, but no need to block this PR I guess.

To be honest, my current policy is to merge everything that has enough ACKs. We see stalled PRs way too often in this repo (I don’t know if it’s worse than in Core, but it’s annoying.) I’ll add this as an item to #1235. @CyberTailor Are you willing to work on this? :)

real-or-random cross-referenced this on Apr 20, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto

CyberTailor commented at 3:44 pm on April 20, 2023: contributor

Later, when 3.21 could be safely set as the minimum supported version

real-or-random merged this on Apr 20, 2023

real-or-random closed this on Apr 20, 2023

real-or-random commented at 3:48 pm on April 20, 2023: contributor

Later, when 3.21 could be safely set as the minimum supported version

Oh, I think the suggestion is to wrap it in if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21)

hebasto cross-referenced this on Apr 20, 2023 from issue cmake: Bugfix and other improvements after bumping CMake up to 3.13 by hebasto

hebasto cross-referenced this on Apr 20, 2023 from issue cmake: Some improvements using `PROJECT_IS_TOP_LEVEL` variable by hebasto

hebasto commented at 10:30 pm on April 20, 2023: member

Later, when 3.21 could be safely set as the minimum supported version

Oh, I think the suggestion is to wrap it in if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21)

See #1284.

theuni commented at 8:33 pm on April 21, 2023: contributor

To be honest, my current policy is to merge everything that has enough ACKs. We see stalled PRs way too often in this repo (I don’t know if it’s worse than in Core, but it’s annoying.) I’ll add this as an item to #1235.

Thanks, this is really helpful. Will keep in mind while reviewing

real-or-random referenced this in commit 222ecaf661 on Apr 27, 2023

sipa referenced this in commit b4eb644b6c on May 12, 2023

hebasto referenced this in commit 49c52ea2b1 on May 13, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

sipa referenced this in commit 901336eee7 on Jun 21, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

hebasto commented at 8:51 pm on July 19, 2023: member

FWIW, another option that could be considered for such a use case is CMAKE_SKIP_INSTALL_RULES.

real-or-random commented at 8:30 am on July 20, 2023: contributor

FWIW, another option that could be considered for such a use case is CMAKE_SKIP_INSTALL_RULES.

As I understand, that covers all projects/subdirectories, so it’s not an exact replacement for SECP256K1_INSTALL.

hebasto commented at 8:35 am on July 20, 2023: member

FWIW, another option that could be considered for such a use case is CMAKE_SKIP_INSTALL_RULES.

As I understand, that covers all projects/subdirectories, so it’s not an exact replacement for SECP256K1_INSTALL.

Yes. It is similar to CMake’s BUILD_SHARED_LIBS and our own SECP256K1_DISABLE_SHARED.

However, I’m currently researching ways to integrate libsecp256k1 into downstream projects in https://github.com/hebasto/secp256k1-CMake-example. And I doubt if any project-specific overrides are required at all. However, it is a WIP, and I’ll report later.

jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick

cmake: Make installation optional #1263