💬 hebasto commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2367649337)
Concept ACK, given the early stage of the current release cycle and the presence of only a single conflicting PR.

💬 fanquake commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2367651708)
Not sure I understand this fix yet, and it seems like there are some other depends changes here, for what I would have expected to be a fixup in libmultiprocess (either in how it's installed, or how we are searching for it in CMake)?

In 38bc0463aaf1f3e20e2f3447af40c29c6d2d6859:
> which in turn facilitates the use of depends
in scenarios where building depends is a separate step, such as on
NixOS.

Building depends is always it's own/a separate step. Not really clear what this means? As f
...

💬 hebasto commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2367667574)
> I think the bug is that commit introduced a (silent) runtime dependency that isn't actually checked/enforced in anyway.

I [agree](https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2331243915).

I'd suggest reverting 199bb09d88e28d951c5068eb65643390dbedd066, as workarounds for https://github.com/bitcoin/bitcoin/issues/30601 and https://github.com/bitcoin/bitcoin/issues/30608 are available.

Additionally, I suspect there is a bug in `src/util/subprocess.h` that needs to be invest
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2367668253)
re-utACK 6ea0eb954bc810da71cd057c116f5c2d359a1f37

Tested against my OPNsense router running miniupnpd 2.3.6 with pf backend, on Intel macOS 13.7 and 15.0, Ubuntu 24.04, FreeBSD 14 and Windows 11.

💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1771053156)
> OK. What can you suggest?

My suggestion is to remove the changes that have been added to facilitate bypassing tests (i.e separating the `LDFLAGS` into `LDFLAGS` and `HARDENED_LDFLAGS`, so that some flags are skipped when tests are performed). If the build environment has changed such that the current set of tests don't pass with the new changes, then the tests should be reworked, not skipped.

💬 Sjors commented on pull request "doc: macOS 15 ships llvm 16":
(https://github.com/bitcoin/bitcoin/pull/30949#issuecomment-2367688021)
The Github CI native macOS 14 build uses Xcode 15:

https://github.com/bitcoin/bitcoin/blob/33adc7521cc8bb24b941d959022b084002ba7c60/.github/workflows/ci.yml#L81-L100

macOS 14 supports Xcode 15: https://developer.apple.com/support/xcode/

So it seems only macOS Ventura 13 needs the manual llvm install, but let me double-check that.

✅ Sjors closed a pull request: "doc: macOS 15 ships llvm 16"
(https://github.com/bitcoin/bitcoin/pull/30949)

💬 Sjors commented on pull request "doc: macOS 15 ships llvm 16":
(https://github.com/bitcoin/bitcoin/pull/30949#issuecomment-2367696187)
The latest point release 15.4 requires macOs 14, but Xcode 15.2 still works on macOS Ventura 13. It shipped with llvm 16 as well.

So this PR is premature, closing.

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2367754148)
> Not sure I understand this fix yet, and it seems like there are some other depends changes here, for what I would have expected to be a fixup in libmultiprocess (either in how it's installed, or how we are searching for it in CMake)?

Dropped the first refactoring commit after the off-line discussion with @fanquake.

✅ Sjors closed an issue: "Warn about / refuse unsupported clang version"
(https://github.com/bitcoin/bitcoin/issues/30947)

💬 Sjors commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367794455)
> we should file a bug upstream, so they can expand the checks they perform

I used cmake version 3.30.3 (via Homebrew).

Even this doesn't trigger an error:

```
add_library(core_interface INTERFACE)
target_compile_features(core_interface INTERFACE cxx_std_20)
```

From googling around I get the impression this check isn't reliable, and you have to check for specific features. E.g. https://stackoverflow.com/questions/75296149/cmake-set-project-to-c-20#comment132865328_75296149

Tha
...

💬 maflcko commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367816284)
> > There is no need to use brew to install clang/llvm. You can just pick the newer XCode: `sudo xcode-select --switch /Applications/Xcode_15.0.app`
>
> That only works if you install Xcode, otherwise you're stuck on whichever version of the command-line tools came with the OS.
>
> You can't install Xcode via the App Store (on macOS 13). You have to download it via the Developer Center. Account required (the free one). You'll need version 15.2, because 15.4 required macOS 14.
>
> So tha
...

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367824247)
> From googling around I get the impression this check isn't reliable, and you have to check for specific features. E.g. https://stackoverflow.com/questions/75296149/cmake-set-project-to-c-20#comment132865328_75296149

That is what CMake does behind the scenes, i.e the tests in https://github.com/Kitware/CMake/tree/master/Tests/CompileFeatures. So if they are missing coverage for a certain compiler/cxx std combo, it's a bug they should address, not something we should have to test ourselves.

💬 Sjors commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367832023)
@fanquake do you mean this file? https://github.com/Kitware/CMake/blob/master/Tests/CompileFeatures/cxx_std_20.cpp

:lock: fanquake locked an issue: "Minar"
(https://github.com/bitcoin/bitcoin/issues/30944)

:lock: fanquake locked an issue: "bitcoin core发送后，链上没有"
(https://github.com/bitcoin/bitcoin/issues/30943)

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367839709)
I'd start by looking at https://github.com/Kitware/CMake/blob/master/Tests/CompileFeatures/CMakeLists.txt, and the various combinations of compiler (versions) + feature tests.

💬 maflcko commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367839797)
Individual compile features are not provided for C++ 17 or later. See https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#low-level-individual-compile-features

And I don't think they are tested either, because code should be using the feature test macros introduced in recent versions of C++.

💬 theStack commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#discussion_r1771176602)
Good point, done.

💬 fanquake commented on issue "Warn about / refuse unsupported clang version":
(https://github.com/bitcoin/bitcoin/issues/30947#issuecomment-2367854771)
It seems like they add cpp feature checks for various compilers if it's otherwise broken/doesn't always report the right version, i.e https://github.com/Kitware/CMake/blob/master/Modules/CMakeCXXCompilerId.cpp.in#L46, maybe they need to do the same for Apple Clang.