cmake: Add mp_headers custom target

[hebasto]

The new mp_headers target acts as a build-graph node for generated Cap'n Proto C++ headers. By providing this custom target, other targets that include the headers can properly order themselves after the generation step without needing to depend on the library target that also uses them.

This convenience target is necessary for proper build dependency management, as the underlying capnp_generate_cpp function is not CODEGEN-aware.

Required for bitcoin/bitcoin#35468.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #288 (Create support branch for CI scripts, documentation, and examples by ryanofsky)
• #276 (build: prepare for subtree split by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ryanofsky]

Code review ACK 941f692. It seems like a good idea to expose a target that will generate the library headers so outside targets can depend on them, and since they are probably needed for most static analysis.

But I'm not sure why PR description says this is required for bitcoin/bitcoin#35468 when it doesn't seem to be referenced there. I wonder if this target is required for anything in particular.

Could also consider making this definition more consistent with target_capnp_sources:

libmultiprocess/cmake/TargetCapnpSources.cmake

Line 118 in 61de697

add_custom_target("${target}_headers" DEPENDS ${generated_headers})

with:

add_custom_target(mp_headers DEPENDS ${MP_PROXY_HDRS})

But current definition seems ok.

[hebasto]

But I'm not sure why PR description says this is required for bitcoin/bitcoin#35468 when it doesn't seem to be referenced there. I wonder if this target is required for anything in particular.

I've updated bitcoin/bitcoin#35468. Please see bitcoin/bitcoin#35468 (comment).

[hebasto]

Could also consider making this definition more consistent with target_capnp_sources:

libmultiprocess/cmake/TargetCapnpSources.cmake

Line 118 in 61de697

add_custom_target("${target}_headers" DEPENDS ${generated_headers})

with:

add_custom_target(mp_headers DEPENDS ${MP_PROXY_HDRS})

I've been thinking about changes in the opposite direction. The _codegen suffix for such build targets seems consistent with CMake's codegen default target.

[Sjors]

The NetBSD jobs failures seem spurious:

  exec: rsync: not found
  rsync: connection unexpectedly closed (0 bytes received so far) [sender]
  rsync error: error in rsync protocol data stream (code 12) at io.c(232) [sender=3.2.7]
  Error: The process '/usr/bin/rsync' failed with exit code 12

[ryanofsky]

I've been thinking about changes in the opposite direction. The _codegen suffix for such build targets seems consistent with CMake's codegen default target.

Yes that could make sense too. It could even make sense to keep both targets since they are intended for different purposes. The _headers targets are intended to be used internally as a way for libraries & executables to depend on headers from the library being defined if some of the headers are generated, without needing to wait for the entire library to be built. I think these could be replaced with file sets in the future. codegen targets seem more useful externally than internally and they should include source files as well as header files. Practically speaking both are interchangeable since source and header files are generated at the same time.

[ryanofsky]

Code review ACK 51ef1d0

[ryanofsky]

Code review ACK 16362f4. Thanks for implementing this. It should make it easier to have control over generated code.