build: add option for external mpgen binary

[theuni]

The parent project or user can supply EXTERNAL_MPGEN, which will override the one set here. If not set, the internally built one is used instead.

This is useful for cross builds, especially when using libmultiprocess as a subdirectory. Parent projects can similarly define a cache var.

Note that the internal binary is still built, but it can be skipped by building the multiprocess target directly.

This is the simplest impl of this I could come up with. Trying to pass an optional option for the binary into target_capnp_sources is not very ergonomic with CMake, so I've opted for just using a global here instead.

It would probably make sense to disable the internal mpgen target if the external option is used, but that adds a good bit of complexity, so I haven't done it here. Happy to do it as a follow-up if desired.

[theuni]

Now that #140 has been merged, after this and #141 (or some solution for that problem), we can enable an in-tree built for libmultiprocess with no need for the target build in depends.

[ryanofsky]

Code review ACK 75cf04a Looks great!

I will probably merge this Monday unless it would help to have it merged sooner, in case @hebasto has feedback.

I think it is good that specifying EXTERNAL_MPGEN does not remove the mpgen build target. But if it is being built unnecessarily (doesn't seem like it is) that would be good to fix. In general I don't like options that enable and disable build targets (see #74), and think all targets that can be built should be available, built when needed, and not built when not needed. Defining targets conditionally turns cmakelists files into spaghetti and makes them more difficult to maintain, and harder to understand and use.

[ryanofsky]

I can also update the depends build in bitcoin/bitcoin#31741 to use this if you didn't already do that. (EDIT: nvm I see you have a branch already)

[hebasto]

Approach ACK 75cf04a.

[hebasto]

1. The value of EXTERNAL_MPGEN must be a full path to satisfy the if(EXIST...) condition. Why not make this more explicit by using the FILEPATH type and mentioning in the help string?

2. As a user-facing cache variable, it should have a library-specific prefix, like this one:https://github.com/chaincodelabs/libmultiprocess/blob/1e06ff07cdeadc78b2c022750f7b5cbc88d56b1d/CMakeLists.txt#L18

3. style nit: A full stop at the end of the help string?

[hebasto]

I'm fairly certain that a generation expression isn't necessary here:

Suggested change

	set(MPGEN_BINARY $<TARGET_FILE:Libmultiprocess::mpgen>)
	set(MPGEN_BINARY Libmultiprocess::mpgen)

[ryanofsky]

I'm planning to merge this soon in its current form to be able to clear the backlog of PRs in this repository and update bitcoin/bitcoin#31741, but hebasto's suggestions in #142 (review) make sense and I implemented them in a followup

[hebasto]

@ryanofsky

In general I don't like options that enable and disable build targets (see #74), and think all targets that can be built should be available, built when needed, and not built when not needed.

I'm curious—how is this approach applicable to scenarios where different targets require their own dependencies? In that case, the lack of available dependencies for one target would prevent building another one, right?

(I assume, that your phrase "In general" extends this approach scope beyond this project).

[ryanofsky]

@ryanofsky

I'm curious—how is this approach applicable to scenarios where different targets require their own dependencies? In that case, the lack of available dependencies for one target would prevent building another one, right?

Ideally, I think there should be WITH_XXXX options that control whether or not cmake searches for each XXXX dependency and targets are not declared if their dependencies are not present.

[hebasto]

First, I apologise for using this PR as a forum for general discussion.

@ryanofsky
I'm curious—how is this approach applicable to scenarios where different targets require their own dependencies? In that case, the lack of available dependencies for one target would prevent building another one, right?

Ideally, I think there should be WITH_XXXX options that control whether or not cmake searches for each XXXX dependency and targets are not declared if their dependencies are not present.

I'm still not convinced, though. Consider a general case with a set of targets, T, a set of dependencies, D, and a non-trivial mapping from T to D.

When a user wishes to build a subset of targets, T', they must somehow to calculate the corresponding subset of dependencies, D'—a task that does not appear to be intended for users.

For an additional example from the LLVM project, simply grep for option(LLVM_BUILD_.

[ryanofsky]

re: #142 (comment)

First, I apologise for using this PR as a forum for general discussion.

No problem, I encourage making issues here just to discuss things, and it seems fine to use this PR, even though opening a new issue could be a little better in the future.

When a user wishes to build a subset of targets, T', they must somehow to calculate the corresponding subset of dependencies, D'—a task that does not appear to be intended for users.

I don't know what use case you are thinking of but this doesn't correspond to any scenario I can remember from my experience.

Usually I want to build all targets, or I want to build one or two specific targets. When I want to build all targets I want to be able to run "make all". When I want to build individual targets I want to run "make thing1 thing2". In neither of these cases do I want to resort to using cmake and toggling targets on and off. I want to run cmake once and then run "make" after that to build the things I need when I need them.

So BUILD_xxx opttions seem mostly useless to me I don't really understand why someone would want them.

But WITH_xxx options are incredibly useful because they allow the project to use a lot of dependencies while still being usable on systems that don't have all the dependencies installed. So when I do run cmake (again, hopefully just one time) it is helpful to be able to turn on/off dependencies depending on what is available in my current environment. It is also useful to be able to see what extra dependencies are available that I might want to install to enable more features.