cmake: Use PUBLIC_HEADER target property in installation logic #1679

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:250604-install changing 2 files +52 −63
  1. hebasto commented at 12:35 pm on June 4, 2025: member
    This PR updates the installation logic to the idiomatic approach.
  2. cmake, move-only: Move module option processing to `src/CMakeLists.txt`
    This change simplifies the following commit.
    c32715b2a0
  3. cmake: Use `PUBLIC_HEADER` target property
    This change simplifies the installation logic.
    6f67151ee2
  4. in src/CMakeLists.txt:13 in ec41a35568 outdated
     5+# Processing must be done in a topological sorting of the dependency graph
     6+# (dependent module first).
     7+if(SECP256K1_ENABLE_MODULE_ELLSWIFT)
     8+  add_compile_definitions(ENABLE_MODULE_ELLSWIFT=1)
     9+  set_property(TARGET secp256k1 APPEND PROPERTY PUBLIC_HEADER ${PROJECT_SOURCE_DIR}/include/secp256k1_ellswift.h)
    10+endif()
    


    hebasto commented at 12:36 pm on June 4, 2025:
    add_compile_definitions() still affects all targets created in the same directory scope, regardless of whether those targets are created before or after add_compile_definitions() is called.
  5. hebasto force-pushed on Jun 4, 2025
  6. real-or-random added the label build on Jun 4, 2025
  7. real-or-random added the label refactor/smell on Jun 4, 2025
  8. real-or-random commented at 6:31 am on June 6, 2025: contributor
    Does this make a difference, or is it just more idiomatic? The docs for PUBLIC_HEADER look very specific to Apple and specific to shared libraries, but according to the discussion in the comments of https://stackoverflow.com/a/54285948, it’s just that the docs are confusing… “File sets” seem to be better, but unfortunately, they require CMake 3.23…
  9. hebasto commented at 7:19 am on June 6, 2025: member

    Does this make a difference, or is it just more idiomatic? The docs for PUBLIC_HEADER look very specific to Apple and specific to shared libraries, but according to the discussion in the comments of https://stackoverflow.com/a/54285948, it’s just that the docs are confusing… “File sets” seem to be better, but unfortunately, they require CMake 3.23…

    From the docs for install(TARGETS <target>... [...]):

    PUBLIC_HEADER

    • Any PUBLIC_HEADER files associated with a library are installed in the destination specified by the PUBLIC_HEADER argument on non-Apple platforms. Rules defined by this argument are ignored for FRAMEWORK libraries on Apple platforms because the associated files are installed into the appropriate locations inside the framework folder.

    Additionally, from Professional CMake: A Practical Guide 21st Edition, Section 35.5.2. Explicit Public And Private Headers:

    If the target is not a framework, or if building for a non-Apple platform, no special handling is performed at build time. When the target is installed, files listed in these properties are also installed, but with their paths removed.

    Here is an example of using PUBLIC_HEADER in a sibling project:

    0set_target_properties(multiprocess PROPERTIES
    1    PUBLIC_HEADER "${MP_PUBLIC_HEADERS}")
    2install(TARGETS multiprocess EXPORT LibTargets
    3  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT lib
    4  PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mp COMPONENT lib)
    

    cc @ryanofsky

  10. real-or-random commented at 9:45 am on June 6, 2025: contributor
    Concept ACK, this seems indeed more idiomatic

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: 2025-06-08 17:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me