cmake: add target_capnp_sources headers target

[ryanofsky]

Modify target_capnp_sources function to create a custom cmake target depending on generated capnp headers, that can be listed as an explicit dependency of other c++ targets when cmake's implicit dependency tracking for included files doesn't work.

This might help fix the build problem encountered bitcoin/bitcoin#30510 (comment)

[sedited]

lgtm ACK 66e12f1

[hebasto]

Modify target_capnp_sources function to create a custom cmake target depending on generated capnp headers, that can be listed as an explicit dependency of other c++ targets when cmake's implicit dependency tracking for included files doesn't work.

This might help fix the build problem encountered bitcoin/bitcoin#30510 (comment)

What are the steps to reproduce the problem? Does it depend on the CMake version?

[ryanofsky]

What are the steps to reproduce the problem? Does it depend on the CMake version?

I don't think it depends on cmake version. The problem was the cmake was attempting to compile the src/test/ipc_test.cpp file before the build/src/ipc/capnp/mining.capnp.h file was generated by the target_capnp_sources(bitcoin_ipc ... capnp/mining.capnp) rule in src/ipc/CMakeLists.txt

The target_capnp_sources function calls add_custom_command calls which lists mining.capnp.h as an output of the custom build step, so if the src/test/ipc_test.cpp were including ipc/capnp/mining.capnp.h directly, I think there would not be a problem.

But the problem is mining.capnp.h is not included directly, but indirectly through another generated header (build/src/test/ipc_test.capnp.h) and there is no way cmake could automatically detect one generated header including another generated header before the headers are generated. So a workaround is required to tell cmake that ipc_test.cpp.o depends on headers generated as part of the bitcoin_ipc library, so that is what add_dependencies(bitcoin_ipc_test bitcoin_ipc_headers) does. This PR was needed to define the bitcoin_ipc_headers target.

[ryanofsky]

I never reproduced this problem locally, but assume it would be possible to reproduce in clean build (probably not in an incremental build). I only saw it on CI.

[hebasto]

@ryanofsky

Thank you for your detailed explanation.

What are the steps to reproduce the problem? Does it depend on the CMake version?

I don't think it depends on cmake version. The problem was the cmake was attempting to compile the src/test/ipc_test.cpp file before the build/src/ipc/capnp/mining.capnp.h file was generated by the target_capnp_sources(bitcoin_ipc ... capnp/mining.capnp) rule in src/ipc/CMakeLists.txt

I managed to reproduce the issue by building only the bitcoin_ipc_test target. I believe that the correct fix is to add ipc/capnp/mining.capnp to the target_capnp_sources() command arguments.

However, the case where multiple independent targets consume the same generated files still needs to be addressed.

Could you please consider a more general approach, based on the practice documented by CMake?

With the suggested approach, the library users won't be responsible for adding extra ad-hoc target-level dependencies.

[ryanofsky]

Could you please consider a more general approach, based on the practice documented by CMake?

Thanks, this is interesting. I see 3 differences between current 66e12f1 and suggested ac63ce6. Suggested change:

1. Generates multiple custom targets named after individual capnp files instead of a single custom target named after the library
2. Makes custom targets depend on all generated files not just one particular one.
3. Calls add_dependencies on the library target containing the generated files.

I think the first change is ok but somewhat awkward. The second and third changes also seem ok, but I don't think they actually affect anything.

---

The current fix for bug encountered bitcoin/bitcoin#30510 (comment) is just a single line:

add_dependencies(bitcoin_ipc_test bitcoin_ipc_headers)

added in 7f334dbb63851da132eee2a27f00cd62f0443588 from bitcoin/bitcoin#30510.

This line lets files and headers used by the bitcoin_ipc_test library include headers generated as part of bitcoin_ipc library, by making sure the headers are generated before they are included.

If I adopted the suggested change in ac63ce6, this line would need to change to due (1) above:

string(MAKE_C_IDENTIFIER "ipc/mining.capnp" id)
add_dependencies(bitcoin_ipc_test generate_${id}_files)

which seems more awkward and fragile to me, though it could potentially help parallelization since bitcoin_ipc_test library would only depend on one of the custom build steps instead of all of them.

---

Because of the affect on the bitcoin_ipc_test, I don't really like change (1) adding generate_${id}_files targets as a replacement for the current code. But I'd be fine with change (1) as an addition if you think it'd be useful.

Changes (2) and (3) also seem ok and may make the build more correct and standard, though I don't think they can have practical effect, and are a little more verbose.

I'd be happy to merge these changes on top of the current fix to clean it up and make it more standard, though I don't think it would be good to remove the current fix because it would extra complexity it would add to src/CMakeLists.txt file in bitcoin. Also it would be good if these changes were implemented in 3 commits instead of 1 to more easily understand what they are doing.

[ryanofsky]

I believe that the correct fix is to add ipc/capnp/mining.capnp to the target_capnp_sources() command arguments.

I think I can imagine a solution like this working but it seems a little complicated. (Note just for clarity that missing dependency is on the generated mining.capnp.h file which is different than the static mining.capnp file.) I think adding a file level dependency directly wouldn't work, because the libraries are built by different CMakeLists.txt files. But maybe the target_capnp_sources function could have an option to accept file dependencies that it converts into custom target dependencies and adds on the caller's behalf.

[hebasto]

Could you please consider a more general approach, based on the practice documented by CMake?

Thanks, this is interesting. I see 3 differences between current 66e12f1 and suggested ac63ce6.

Thanks for your feedback.

I just wanted to clarify that my concerns are about cases like this:

add_library(alpha alpha.cpp)
target_capnp_sources(alpha ${PROJECT_SOURCE_DIR}
      alpha.capnp
      common.capnp
)

add_library(beta beta.cpp)
target_capnp_sources(beta ${PROJECT_SOURCE_DIR}
      beta.capnp
      common.capnp
)

The CMake documentation explicitly warns against such usage for files generated from common.capnp.

However, I agree that the current implementation is a less invasive fix for bitcoin/bitcoin#30510 (comment).

---

I think adding a file level dependency directly wouldn't work, because the libraries are built by different CMakeLists.txt files.

FWIW, the following diff applied to the bitcoin/bitcoin#30510 @ bitcoin/bitcoin@b95bb21:

--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -340,9 +340,9 @@ if(WITH_MULTIPROCESS)
       test/ipc_test.cpp
     )
     target_capnp_sources(bitcoin_ipc_test ${PROJECT_SOURCE_DIR}
+      ipc/capnp/mining.capnp
       test/ipc_test.capnp
     )
-    add_dependencies(bitcoin_ipc_test bitcoin_ipc_headers)
   endif()
 endif()
 

works, and I don't see any reasons why it shouldn't.

[ryanofsky]

works, and I don't see any reasons why it shouldn't.

Thanks for providing the diff. I didn't understand what was being suggested earlier. That approach didn't occur to me and I agree it would be an indirect fix for the problem because instead of telling build system that bitcoin_ipc_test library depends on a header generated by the bitcoin_ipc library, it could just generate the header and other files itself. I think this approach might be racy though because bitcoin_ipc generate build steps might try to overwrite the generated header file while the bitcoin_ipc_test compilation step is reading from it. Also this approach might be a little less efficient because the files would be generated twice instead of once which would waste a little effort and could affect file mtimes causing files to be rebuilt unnecessarily.

I just wanted to clarify that my concerns are about cases like this:

I agree that case is concerning but I think it is just an incorrect usage of the target_capnp_sources function. The point of the function is to generate c++ headers and source files for each of the .capnp files specified, which is something that should only be done once per capnp file, not multiple times. If we wanted the target_capnp_sources to accept arguments specifying additional .capnp files that the library depends on I think that could be a good idea. Like

target_capnp_sources(common ${PROJECT_SOURCE_DIR} common.capnp)
target_capnp_sources(alpha ${PROJECT_SOURCE_DIR} alpha.capnp DEPENDS common.capnp)
target_capnp_sources(beta ${PROJECT_SOURCE_DIR} beta.capnp DEPENDS beta.capnp)

This is actually what I thought you were suggesting earlier, and I think it could be a nice approach. It would would be a little more complicated than current approach, though. And one thing that would be make it awkward with current cmake configuration is that target_capnp_sources functions are being called in different directories with different paths, so would need to normalize the paths before passing them to MAKE_C_IDENTIFIER.

[hebasto]

works, and I don't see any reasons why it shouldn't.

Thanks for providing the diff. I didn't understand what was being suggested earlier. That approach didn't occur to me and I agree it would be an indirect fix for the problem because instead of telling build system that bitcoin_ipc_test library depends on a header generated by the bitcoin_ipc library, it could just generate the header and other files itself. I think this approach might be racy...

That's true for the current target_capnp_sources() implementation. However, my implementation aims to resolve this issue.

Also this approach might be a little less efficient because the files would be generated twice instead of once which would waste a little effort and could affect file mtimes causing files to be rebuilt unnecessarily.

It won't happen after amending my implementation with your suggestion "to normalize the paths before passing them to MAKE_C_IDENTIFIER".

I just wanted to clarify that my concerns are about cases like this:

I agree that case is concerning but I think it is just an incorrect usage of the target_capnp_sources function. The point of the function is to generate c++ headers and source files for each of the .capnp files specified, which is something that should only be done once per capnp file, not multiple times. If we wanted the target_capnp_sources to accept arguments specifying additional .capnp files that the library depends on I think that could be a good idea. Like

target_capnp_sources(common ${PROJECT_SOURCE_DIR} common.capnp)
target_capnp_sources(alpha ${PROJECT_SOURCE_DIR} alpha.capnp DEPENDS common.capnp)
target_capnp_sources(beta ${PROJECT_SOURCE_DIR} beta.capnp DEPENDS beta.capnp)

Looks nice! Do you think this direction is worth our further effort?

[ryanofsky]

I think I see what you are saying about the ac63ce6 avoiding generating the same files twice due to the if(NOT TARGET generate_${id}_files) check. Though I think an additional normalization step may still be needed because of different relative paths in different CMakeLists.txt files.

Do you think this direction is worth our further effort?

I'm ok with current approach and don't problems or real drawbacks with it, but I'd also be happy to change to a different approach if you prefer that. The current approach does seem like this simplest approach you could take that is still correct. And I like how it lets dependencies be specified at library level not the individual target level. So as long as bitcoin_ipc_test library depends on bitcoin_ipc library, it can include all files from the bitcoin_ipc library without having to individually list them in cmake file. This feels like a pretty good way to encapsulate dependencies, though I can also see reasons to want to be more granular.

Either way seems fine to me and I'm not planning to work on changes here, but would happy to review any.

[hebasto]

The current approach does seem like this simplest approach you could take that is still correct.

So, let's keep it that way until it serves its purpose.

Thank you for the discussion :)