Skip to content

cmake: EXTERNAL_MPGEN cleanups #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 3, 2025
Merged

cmake: EXTERNAL_MPGEN cleanups #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ if(Libmultiprocess_ENABLE_CLANG_TIDY)
set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}")
endif()

set(EXTERNAL_MPGEN "" CACHE STRING "Use the supplied mpgen binary rather than the one built internally")
Copy link
Contributor

@theuni theuni Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I purposefully avoided namespacing this because I was assuming that parent projects would always want to expose the same option and have it be automatically inherited here. Now they'll have to map it.

Not a big deal to me either way, I guess.

Copy link
Collaborator Author

@ryanofsky ryanofsky Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, code which sets this in depends is implemented in bitcoin/bitcoin@99b7b85 from bitcoin/bitcoin#31741 and seems to work reasonably well

Copy link
Contributor

@theuni theuni Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For depends, sure, it doesn't matter. But when doing a non-depends cross-build, ccmake shows:

CAPNPC_CXX_EXECUTABLE
CAPNP_EXECUTABLE
...
Libmultiprocess_EXTERNAL_MPGEN

which is rather inconsistent and non-obvious to builders imo. Seems to me Capnp has it right. For a Core, I wouldn't expect to see that namespace.

Copy link
Collaborator Author

@ryanofsky ryanofsky Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This suggestion just came from #142 (comment), and I'm happy to rename this to something else or make a different change. Given the other names maybe MPGEN_EXECUTABLE would be a good name for this.

Copy link
Collaborator Author

@ryanofsky ryanofsky Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hebasto might have more ideas too.

Copy link
Contributor

@theuni theuni Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This suggestion just came from #142 (comment), and I'm happy to rename this to something else or make a different change. Given the other names maybe MPGEN_EXECUTABLE would be a good name for this.

Fwiw, I originally picked EXTERNAL_CAPNP to mirror (what I thought was) the way Capnp handles it. But I see now that Capnp's is just a bool and CAPNP_EXECUTABLE is the actual path variable. So ACK to MPGEN_EXECUTABLE instead if we go down this path.

Copy link
Member

@hebasto hebasto Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ACK to MPGEN_EXECUTABLE instead if we go down this path.

So do I.

set(Libmultiprocess_EXTERNAL_MPGEN "" CACHE FILEPATH "If specified, should be full path to an external mpgen binary to use rather than the one built internally.")

include("cmake/compat_config.cmake")
include("cmake/pthread_checks.cmake")
Expand Down
10 changes: 5 additions & 5 deletions cmake/TargetCapnpSources.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ function(target_capnp_sources target include_prefix)
)

set(MPGEN_BINARY "")
if(EXTERNAL_MPGEN)
set(MPGEN_BINARY "${EXTERNAL_MPGEN}")
if(Libmultiprocess_EXTERNAL_MPGEN)
set(MPGEN_BINARY "${Libmultiprocess_EXTERNAL_MPGEN}")
if(NOT EXISTS "${MPGEN_BINARY}")
message(FATAL_ERROR "EXTERNAL_MPGEN: \"${MPGEN_BINARY}\" does not exist.")
message(FATAL_ERROR "Libmultiprocess_EXTERNAL_MPGEN: \"${MPGEN_BINARY}\" does not exist.")
endif()
elseif(TARGET Libmultiprocess::multiprocess)
set(MPGEN_BINARY $<TARGET_FILE:Libmultiprocess::mpgen>)
Copy link
Contributor

@theuni theuni Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted this a bunch of different ways, and with some approach this was necessary, but I forget why. I assume this is fine, thanks for simplifying.

set(MPGEN_BINARY Libmultiprocess::mpgen)
else()
message(FATAL_ERROR "No usable mpgen. Set EXTERNAL_MPGEN or enable the internal target.")
message(FATAL_ERROR "No usable mpgen. Set Libmultiprocess_EXTERNAL_MPGEN or enable the internal target.")
endif()

set(generated_headers "")
Expand Down