cmake: Make installation optional #1263

pull CyberTailor wants to merge 1 commits into bitcoin-core:master from CyberTailor:master changing 2 files +50 −45
  1. CyberTailor commented at 7:53 am on April 9, 2023: contributor
    Useful for embedding secp256k1 in a subproject.
  2. CyberTailor force-pushed on Apr 9, 2023
  3. real-or-random commented at 8:02 am on April 9, 2023: contributor
    cc @hebasto @theuni
  4. 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 :)

  5. 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?
  6. 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.

  7. 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)
  8. 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.

  9. 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+.

  10. 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?

  11. real-or-random commented at 8:49 am on April 20, 2023: contributor
    and needs rebase :)
  12. cmake: Make installation optional 
    Useful for embedding secp256k1 in a subproject.
    47ac3d63cd
  13. CyberTailor force-pushed on Apr 20, 2023
  14. theuni approved
  15. 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.

  16. real-or-random approved
  17. real-or-random commented at 3:37 pm on April 20, 2023: contributor
    utACK 47ac3d63cd5e00a2d50cb489461c8bc349d37912
  18. hebasto approved
  19. hebasto commented at 3:38 pm on April 20, 2023: member
    ACK 47ac3d63cd5e00a2d50cb489461c8bc349d37912, tested on Ubuntu 23.04.
  20. 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? :)

  21. 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
  22. CyberTailor commented at 3:44 pm on April 20, 2023: contributor
    Later, when 3.21 could be safely set as the minimum supported version
  23. real-or-random merged this on Apr 20, 2023
  24. real-or-random closed this on Apr 20, 2023
  25. 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)

  26. hebasto cross-referenced this on Apr 20, 2023 from issue cmake: Bugfix and other improvements after bumping CMake up to 3.13 by hebasto
  27. hebasto cross-referenced this on Apr 20, 2023 from issue cmake: Some improvements using `PROJECT_IS_TOP_LEVEL` variable by hebasto
  28. 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.

  29. 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

  30. real-or-random referenced this in commit 222ecaf661 on Apr 27, 2023
  31. sipa referenced this in commit b4eb644b6c on May 12, 2023
  32. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
  33. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  34. sipa referenced this in commit 901336eee7 on Jun 21, 2023
  35. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  36. 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.
  37. 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.

  38. 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.

  39. 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


CyberTailor real-or-random hebasto theuni

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me