Hennadii Stepanov

Results 1422 comments of 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.