💬 hebasto commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2315037182)
Since https://github.com/bitcoin/bitcoin/pull/30454 has been merged, this PR is rebased and its description updated.

Ready for review now :)

💬 hebasto commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734503675)
I'd keep this line. Otherwise, on Windows, build artifacts from pre-CMake branches, such as 28.x, will linger and be ignored by git.

🤔 hebasto reviewed a pull request: "build: remove old MSVC build system"
(https://github.com/bitcoin/bitcoin/pull/30731#pullrequestreview-2265980571)
Concept ACK, obviously :)

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1734510436)
Thanks! Fixed in latest force-push.

👍 hebasto approved a pull request: "build: remove old MSVC build system"
(https://github.com/bitcoin/bitcoin/pull/30731#pullrequestreview-2266016439)
ACK 04fb085f6bddc90e806deefd90c72104763f055a.

💬 laanwj commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#issuecomment-2315098816)
There's a mention left in the top level gitignore:
```
.gitignore:!/build_msvc
```

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1734529498)
Yup, noticed it earlier as well, but decided to keep for now. Could drop if more people agree.

As noted in the docs, `vector` is currently used to signal that a size-prefix should be added during serialization, so if the conversion to accept `std::byte` in more places happens before adaptation to accept `std::array` for size-prefixed data, `_hex_v` will become necessary.

It also feels more complete to have all `byte`/`uint8_t` and `array`/`vector` combinations.

💬 hebasto commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#issuecomment-2315104927)
> There's a mention left in the top level gitignore:
>
> ```
> .gitignore:!/build_msvc
> ```

https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734503675

💬 laanwj commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734535495)
Isn't the point to ignore build artifacts though? Sure, they might linger but better than accidentally checking them into git.

💬 hebasto commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734544895)
I'm not sure if "accidentally checking them into git" is a concern we should worry about. However, after two years of switching between CMake and non-CMake branch, I've been convinced to keep the source tree clean.

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1734548531)
> so if the conversion to accept std::byte in more places happens before adaptation to accept std::array for size-prefixed data, _hex_v will become necessary.

That is a good point, so we are indeed expecting to use it in the future. In that case, since it's already reviewed, let's keep it in and you can mark this as resolved.

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2315130614)
re-ACK a096215c9c71a2ec03e76f1fd0bcdda0727996e0

💬 laanwj commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734553073)
But build directories that aren't called `build_msvc` can still linger aorund in the same way. i think it's strange to keep this around but ok.

💬 fanquake commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734556967)
@laanwj fwiw I agree with you, and think it should be dropped. Just couldn't be bothered having the argument.

🤔 tdb3 reviewed a pull request: "test: add test for specifying custom pidfile via `-pid`"
(https://github.com/bitcoin/bitcoin/pull/30724#pullrequestreview-2266069102)
Concept ACK

`"bitcoind.pid"` is also mentioned in `feature_filelock.py`. Could add a commit that deduplicates this.

Created an example commit here:
https://github.com/tdb3/bitcoin/commit/c2483b63b49af24fe263c4b33334c202b19e4156

Thought about maybe making some like a
```python
@property
def pid_file_path(self) -> Path:
return self.chain_path / "bitcoind.pid"
```
but with `bitcoind -pid=someotherfile` being an option, I thought it might be confusing/unexpected to have `pid_fi
...

💬 laanwj commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734560814)
> I'm not sure if "accidentally checking them into git" is a concern we should worry about

i mean i personally use `git add .` sometimes to check all changes and additions in, relying that the `.gitignore` prevents any build artifacts from being checked in (because otherwise they're ignored all over the tree). So for some people it's probably a concern. Sure i check before pushing to github but sometimes it could result in extra work.

> @laanwj fwiw I agree with you, and think it should be
...

🤔 maflcko requested changes to a pull request: "build: Remove Autotools-based build system"
(https://github.com/bitcoin/bitcoin/pull/30664#pullrequestreview-2250026559)
I think this should properly remove all no-longer needed stuff.

Also, I think this should wait a few days, to allow everyone some time to upgrade and test parity, etc. See https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2314866592

💬 maflcko commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1734555907)
Did you see this comment?

💬 maflcko commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1724585221)
Note to myself: Test whether this is still needed when building from a tarball

💬 maflcko commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1734559384)
Same for all other stuff that is no longer needed, like bear:

```
ci/test/03_test_script.sh: # accepted in src/.bear-tidy-config
doc/developer-notes.md:make clean && bear --config src/.bear-tidy-config -- make -j $(nproc)
```

...