💬 davidgumberg commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1735211603)
In commit: `lint: Remove python lint rules that are SyntaxError`

Nit: I believe all of F701-F707 result in `SyntaxError` and should be removed

<details>

<summary> The following examples resulted in SyntaxError's for Python 2.7.18 </summary>

f701.py:
```
def func(x):
if x > 5:
break

for i in range(10):
func(i) # <--- call function
```

f702.py:
```
def func(x):
if x > 5:
continue

for i in range(10):
func(i)
```

f704.py:
``
...

💬 l0rinc commented on pull request "test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2316141282)
@maflcko, would it make sense for me to rebase this or should I just close it at this stage?

💬 jonatack commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2316141749)
ACK 797eede7609ca0d4d788e62f35e70e85103a19dc

LGTM provided we're migrating to `std::span` and modulo the tidy CI iwyu checks on the updated files.

💬 fanquake commented on pull request "ci: Re-add configs removed in cmake migration":
(https://github.com/bitcoin/bitcoin/pull/30740#discussion_r1735217975)
Sure, it's likely fine, just inconsistent with the other CIs.

✅ l0rinc closed a pull request: "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`"
(https://github.com/bitcoin/bitcoin/pull/30619)

💬 sdaftuar commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1735220144)
Should this say:
`cmake -B build -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang`
instead?

When I follow these instructions as written, I get a Cmake Warning that the CMAKE_CC_COMPILER variable was not used by the project.

💬 jonatack commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2316147152)
ACK 797eede7609ca0d4d788e62f35e70e85103a19dc

Doesn't conflict with the changes in https://github.com/bitcoin/bitcoin/pull/29119 and LGTM provided we're migrating to `std::span` and modulo the tidy CI iwyu checks on the updated files (the task hasn't run yet at the time of writing this).

💬 jonatack commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/29119#issuecomment-2316149074)
Concept ACK, provided we're migrating to `std::span`.

💬 l0rinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1735225920)
Indeed, that's what we have in other cases: https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md?plain=1#L11

📝 hebasto opened a pull request: "doc: Fix typo in `build-unix.md`"
(https://github.com/bitcoin/bitcoin/pull/30744)
Addresses https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1735220144.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1735228312)
Thanks! Fixed in https://github.com/bitcoin/bitcoin/pull/30744.

👍 l0rinc approved a pull request: "doc: Fix typo in `build-unix.md`"
(https://github.com/bitcoin/bitcoin/pull/30744#pullrequestreview-2267169775)
ACK e78551baec1bd63192112a816cef8c207c049415

💬 TheCharlatan commented on pull request "doc: update dev note examples for CMake":
(https://github.com/bitcoin/bitcoin/pull/30739#issuecomment-2316198304)
Do you want to add https://github.com/TheCharlatan/bitcoin/commit/17f6ecd1470c07a3bf26a08e6d1a55ed2f8486ca? I guess we should update all scripts and examples to use the `build` directory for executing the binaries?

💬 davidgumberg commented on pull request "ci: Use C++23 in one task":
(https://github.com/bitcoin/bitcoin/pull/30735#discussion_r1735271207)
A note for other readers:
`Wno-error=deprecated-declarations` is here because the leveldb subtree [currently uses](https://github.com/bitcoin-core/leveldb-subtree/issues/43) `std::aligned_storage`. ([deprecated in C++23](https://wg21.link/P1413R3))

Patches introducing features deprecated in C++20 or earlier will still be caught by other CI tasks.

💬 davidgumberg commented on pull request "ci: Use C++23 in one task":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2316226568)
ACK https://github.com/bitcoin/bitcoin/pull/30735/commits/fac587ea070fe1354708aacce33ebb9cebd35e5b

tested with:
```
cmake -B build -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_CC_COMPILER=clang -DAPPEND_CXXFLAGS='-std=c++23'
```

💬 davidgumberg commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1735290003)
This check doesn't seem like a terrible idea, but not supported by ruff.

💬 davidgumberg commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1735290664)
This seems reasonable to me, but not available without the `--preview` argument

💬 laanwj commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2316258427)
it is true that `getconf _NPROCESSORS_ONLN` works on macos out of the box, while `nproc` isn't installed by default, but the identifier is so ugly and hard to type i find it an questionable improvement documentation-wise

💬 l0rinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2316261602)
> i find it an questionable improvement documentation-wise

Yeah, would require more thorough doc updates to clarify that - agree, not worth it

💬 davidgumberg commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1735299198)
In commit: [lint: Remove python whitespace and shadowing lint rules](https://github.com/bitcoin/bitcoin/pull/30723/commits/fafb0f62175798c5d5c174213c1ff1ccf5cea6ca)

+1 on dropping checks that result in `IndentError`

some of the other checks removed here seem reasonable to me e.g. E125, and E275, but not available/hidden behind preview flag in ruff so I'm ~0 on whether or not to remove the rest of them