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 https://github.com/bitcoin/bitcoin/pull/35468.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
support branch for CI scripts, documentation, and examples by ryanofsky)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Code review ACK 941f6925cacd6bbb27a08a51e8e69f40a091445d. 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 https://github.com/bitcoin/bitcoin/pull/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:
with:
add_custom_target(mp_headers DEPENDS ${MP_PROXY_HDRS})
But current definition seems ok.
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 https://github.com/bitcoin/bitcoin/pull/35468. Please see https://github.com/bitcoin/bitcoin/pull/35468#issuecomment-4634889244.
Could also consider making this definition more consistent with target_capnp_sources:
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.
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
I've been thinking about changes in the opposite direction. The
_codegensuffix for such build targets seems consistent with CMake'scodegendefault 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.
138 | @@ -139,6 +139,10 @@ configure_file(include/mp/config.h.in "${CMAKE_CURRENT_BINARY_DIR}/include/mp/co 139 | # Generated C++ Capn'Proto schema files 140 | capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp) 141 | set_source_files_properties("${MP_PROXY_SRCS}" PROPERTIES SKIP_LINTING TRUE) # Ignored before cmake 3.27 142 | +# Add a convenience custom target that allows generating 143 | +# Cap'n Proto C++ source files, which is useful as 144 | +# the `capnp_generate_cpp` function is not CODEGEN-aware.
In commit "cmake: Add mp_proxy_codegen custom target" (51ef1d097df4d384284e5df923a90c01f64da53b)
Commit title and commit comment and this code comment aren't really in sync with the code change in the commit. Would suggest s/mp_proxy_codegen/mp_headers/ and I also liked this suggested code comment from chatgpt:
# Build-graph node for generated headers. This lets targets that include
# the headers order themselves after generation without depending on the
# library target that also uses them.
add_custom_target(mp_headers DEPENDS ${MP_PROXY_HDRS})
Thanks! Reworked.
Code review ACK 51ef1d097df4d384284e5df923a90c01f64da53b
This 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.
Code review ACK 16362f42d01bab3055f97eceb062775d27d858c0. Thanks for implementing this. It should make it easier to have control over generated code.