Bitcoin Core Github
44 subscribers
121K links
Download Telegram
📝 fanquake opened a pull request: "doc: swap CPPFLAGS for APPEND_CPPFLAGS"
(https://github.com/bitcoin/bitcoin/pull/31819)
`APPEND_CPPFLAGS` will be understood by our CMake, whereas `CPPFLAGS` will not. Attempting what is currently documented will just give:
```bash
CMake Warning:
Ignoring extra path from command line:

"CPPFLAGS=-DDEBUG_LOCKCONTENTION"
```
👍 hebasto approved a pull request: "doc: swap CPPFLAGS for APPEND_CPPFLAGS"
(https://github.com/bitcoin/bitcoin/pull/31819#pullrequestreview-2602176228)
ACK ea687d202934ee9aa26912cda21993da219cd418.
📝 fanquake opened a pull request: "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in"
(https://github.com/bitcoin/bitcoin/pull/31820)
Follows up from when the `pc.in` was added.
🚀 glozow merged a pull request: "test: test_inv_block, use mocktime instead of waiting"
(https://github.com/bitcoin/bitcoin/pull/31811)
💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2643498506)
Thank you @hebasto! We had a pretty long discussion about this. I think that hebasto's approach is probably fine and would never be an issue in the real world. I'll admit that I'm being overly paranoid and probably wrong about the guarantees that linkers provide...

But still, I'm not alone in my paranoia. For example, Google has [banned non-constexpr inline header variables](https://groups.google.com/a/chromium.org/g/cxx/c/hmyGFD80ocE/m/qVe-hqAVDAAJ) for Chromium out of similar concerns.

[
...
💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1946784827)
I needed to change this to an arithmetic comparison to make it work because the `wc -l` result includes some white space on my system.

``` suggestion
if (( $(get_file_suffix_count gcda) != 0 )); then
```
🤔 fjahr reviewed a pull request: "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds"
(https://github.com/bitcoin/bitcoin/pull/31588#pullrequestreview-2602169248)
Hm, I tried to test this but wasn't successful so far. Aside from the fix I suggest in my inline comment I still get an error that gcov is not able to find the working dir. That seems to be because the coverage files include the build paths and not the source paths. After a little digging I tried to address this by compiling with `-DAPPEND_CXXFLAGS="-fprofile-prefix-map=${BUILDDIR}/src=${TOPDIR}/src"` but that didn't work and it doesn't seem like a good enough fix anyway.

Does this just work
...
💬 purpleKarrot commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643507434)
Where can I read about the decision to chose `CPPFLAGS` and `APPEND_CPPFLAGS`? I think those names are unfortunate and may cause confusion with cmake's default handling of the `CXXFLAGS` environment variable.
👍 stickies-v approved a pull request: "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in"
(https://github.com/bitcoin/bitcoin/pull/31820#pullrequestreview-2602342964)
ACK f5b9a2f68c95182b139e8a3447b4b5888ecb4f9f
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946890236)
>
👍 theuni approved a pull request: "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in"
(https://github.com/bitcoin/bitcoin/pull/31820#pullrequestreview-2602359582)
utACK f5b9a2f68c95182b139e8a3447b4b5888ecb4f9f
👍 theuni approved a pull request: "guix: remove test-security/symbol-check scripts"
(https://github.com/bitcoin/bitcoin/pull/31818#pullrequestreview-2602389053)
utACK 485de09d9edff6abd6eef9e54e70b21bf6ceb9e4.

I opened #31715 to "fix" some issues with these, but I was _really_ struggling to understand what good those tests were doing at all. I agree they've outlived their utility. At this point they're basically hard-coded things checking for hard-coded things. I proposed those fixes merely to "make tests turn green" as opposed to doing something useful. I agree that just deleting them makes more sense.

Note to other reviewers, this doesn't remove
...
theuni closed a pull request: "build: CMake security checks workarounds"
(https://github.com/bitcoin/bitcoin/pull/31715)
💬 theuni commented on pull request "build: CMake security checks workarounds":
(https://github.com/bitcoin/bitcoin/pull/31715#issuecomment-2643563724)
Closing in favor of #31818.
💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2643586049)
@fjahr: Executing the script with the commands mentioned in the PR description works for me on Ubuntu 22.04.03 LTS, didn't try it on any other machine yet. Which operating system are you running? (Some macOS I presume?) Some versions from my machine that would be relevant for comparison:
```
$ cat /etc/lsb-release | tail -n1
DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"
$ gcovr --version | head -n1
gcovr 5.0
$ gcc --version | head -n1
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
$ bash --version
...
💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1946935711)
The meaning of `BITCOINKERNEL_STATIC` here is overloaded. Just to differentiate the use-cases, I'd prefer to have a 3rd define here:

`#elif defined(_WIN32) && !defined(BITCOIN_BUILD_INTERNAL) && !defined(BITCOINKERNEL_STATIC)`

`BITCOINKERNEL_STATIC`: I'm a user of the kernel building against a static kernel.
`BITCOIN_BUILD_INTERNAL`: I'm a component of Core who's not building against the kernel at all, I just happen to be seeing this header.

The former should stay as-is for bitcoin-cha
...
🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2602352724)
Rebased 99b7b857d3b8b9ce9bd7501e2480583369740c55 -> 4ca2462bf516846c5552378345e243bc819553a3 ([`pr/subtree.8`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.8) -> [`pr/subtree.9`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.8-rebase..pr/subtree.9)) to fix conflict with #31800. Also applied review suggestions from vasild, renamed Libmultiprocess_EXTERNAL_MPGEN cmake option to MPGEN_EXECUTABLE as suggested
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946903444)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942670069

Thanks, updated
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946892750)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942473099

Thanks, fixed