Hennadii Stepanov
Hennadii Stepanov
> I believe `MAIN_DEPENDENCY` is MSVC related, so it wouldn't show up in the `build.ninja`. Did it help with something there? [It](https://cmake.org/cmake/help/latest/command/add_custom_command.html): > is treated just like any value given...
> Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly. Set the necessary compile options at the source file level, rather than the target...
> @hebasto, can you elaborate why you think the source-specific compile options are hard to reason about? Sure. Consider this minimal example: ``` cmake_minimum_required(VERSION 3.22) project(test CXX) add_library(interface INTERFACE) target_compile_options(interface...
> Why is that hard to reason about? People unfamiliar with such peculiarities might not realise that compiler options specified as a source's property take precedence over other compiler options.
> I consider the current approach to be a leftover autootools artifact. I agree. Concept ACK. > It looks like the crc32c lib would benefit from this approach as well....
https://github.com/bitcoin/bitcoin/pull/31880 provides an extended alternative to this PR. Since this one has already been reviewed, I'm OK with merging it as is.
> ... as well as a required switch to lxd in the CI. Can you point to any documentation that states this requirement?
Also see https://github.com/google/leveldb/pull/1028.
> I think we might be after `-Wl,--no-allow-shlib-undefined`... From my experience, it fails to catch an issue like one I demoed in https://github.com/bitcoin-core/secp256k1/issues/1556.
> Looking for Concept (N)ACKs. Once ACKed, I'll add the same functionality to the Autotools-based build system. Added a commit for the Autotools-based build system.