💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836332387)
> Is the plan to also delete any unused code from this header?

I haven't consider it yet.

> it maybe already doesn't work with c++20?

CI jobs that uses C++20 are passing so far.

💬 sipa commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836337134)
> Is the plan to also delete any unused code from this header?

If at all possible, I'd prefer for it to be subtreed, so that updating to newer upstream versions is easy.

💬 fanquake commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836341684)
I was actually thinking the opposite. I think a subtree is overkill here, and, upstream isn't exactly active (some random merges but no development), so I'm not sure if there will be many new versions.

I'd rather take the small(er) portion of any code we need, and have some better integrated/reviewed. Hopefully that will help avoid randomly pulling code from upstream, that isn't well developed or reviewed.

💬 fanquake commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836346184)
Note that the last version from upstream was > 3 years ago.

💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836346980)
> I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed.

Maybe a subtree from our own fork? (to avoid bringing testing stuff to the main repo)

💬 fanquake commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836353115)
> Maybe a subtree from our own fork?

I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn't be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.

👍 ryanofsky approved a pull request: "depends: Build the `native_capnp` and `capnp` packages with CMake"
(https://github.com/bitcoin/bitcoin/pull/28856#pullrequestreview-1760050677)
Code review ACK d1604d4b1d1ee8df279a1776303e167cc3d06193. I left a question and suggestion, but they are not critical. Overall seems good to switch from autotools to cmake here. I'm actually not sure why I didn't write this using cmake originally, since the cmake build has been around since capnproto 0.5. It also seems good to get rid of the special --disable-shared flag for android, since that was inconsistent with other platforms.

I think I understand how this change works around the mingw3
...

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#discussion_r1412254941)
In commit "depends: Build `capnp` package with CMake" (2cdb2f8bd6aaff64803482e4128da34db34effa8)

Is this needed? It seems like a potential bug in libmultiprocess if its build system is getting confused by the pkgconfig install files.

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#discussion_r1412257852)
In commit "depends: Build `capnp` package with CMake" (2cdb2f8bd6aaff64803482e4128da34db34effa8)

This change seems like a good way to speed things up by only building the libmultiprocess runtime library, not the code generation tool in the non-native cross build (assuming that is the intention here). But it would be good to write this in a comment so it is clear what is going on.

💬 TheCharlatan commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836362178)
Re https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1836341684

> I'd rather take the small(er) portion of any code we need, and have something better integrated/reviewed.

Looking at the coverage report of the header, there seems to be some potential for pruning out (89.2% function coverage and 67.4% line coverage),

💬 hebasto commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#discussion_r1412304189)
> Is this needed?

No, it isn't. However, we usually try to minimize a prefix size by dropping unneeded stuff.

👍 stickies-v approved a pull request: "init: don't delete PID file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28946#pullrequestreview-1760140851)
ACK 2b28f7df2b7dd140e52e93ef3e1a330db8da32a9

💬 stickies-v commented on pull request "init: don't delete PID file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28946#discussion_r1412308897)
nit: I think this more clearly explains the rationale without being too explicit about implementation
```suggestion
//! True if this process created the pid file, so we don't delete it too early if multiple processes were started
*/
```

💬 fanquake commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836390190)
> but I don't understand how this would fix either of the two problems the closed PR

You are right, this PR doesn't fix at least the first problem, which is configure failing to find libmultiprocess:
```bash
checking for libmultiprocess... no
configure: error: --enable-multiprocess=yes option specified but libmultiprocess library was not found. May need to install libmultiprocess library, or specify install path with PKG_CONFIG_PATH environment variable. Running 'pkg-config --debug libmult
...

💬 ryanofsky commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836421425)
> You are right, this PR doesn't fix at least the first problem, which is configure failing to find libmultiprocess:

Make sense, I think the first commit from #28846 is a good fix for that problem.

💬 achow101 commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#issuecomment-1836478642)
ACK f23ba24aa079d68697d475789cd21bd7b5075550

💬 achow101 commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1836489552)
ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

🚀 achow101 merged a pull request: "bugfix, Change up submitpackage results to return results for all transactions"
(https://github.com/bitcoin/bitcoin/pull/28848)

💬 fanquake commented on pull request "depends: Build the `native_capnp` and `capnp` packages with CMake":
(https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1836491942)
Yea, happy for https://github.com/bitcoin/bitcoin/pull/28846/commits/156366f10aa38c612e26a1f93d8503786f3d3a34 to be cherry-picked in here.

🚀 achow101 merged a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784)