ā
maflcko closed an issue: "RFC: Replacing `tinyformat` with `{fmt}`"
(https://github.com/bitcoin/bitcoin/issues/33942)
(https://github.com/bitcoin/bitcoin/issues/33942)
š¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575193582)
> In addition to all of that, I think that will be a good step toward migrating to C++20 in the future, and we will be able to depend on the standard format library itself.
C++20 is already used, so I'll go ahead and close this for now.
I don't think there is value in switching `tinyformat` to `{fmt}` and then again to `std::format`, with all the churn.
If the switch is done, it should be straight to `std::format`.
So I'll close this for now, but the discussion can continue.
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575193582)
> In addition to all of that, I think that will be a good step toward migrating to C++20 in the future, and we will be able to depend on the standard format library itself.
C++20 is already used, so I'll go ahead and close this for now.
I don't think there is value in switching `tinyformat` to `{fmt}` and then again to `std::format`, with all the churn.
If the switch is done, it should be straight to `std::format`.
So I'll close this for now, but the discussion can continue.
š¬ hebasto commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575194223)
> Fixes #33859
Iām not convinced it does.
On the master branch @ 0690514d4f72aac251ee0b876cded9187d42c63e:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx CXX=clang++
native_capnp_cxx=g++
```
With this PR, the behaviour is unchanged:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx CXX=clang++
native_capnp_cxx=g++
```
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575194223)
> Fixes #33859
Iām not convinced it does.
On the master branch @ 0690514d4f72aac251ee0b876cded9187d42c63e:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx CXX=clang++
native_capnp_cxx=g++
```
With this PR, the behaviour is unchanged:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx CXX=clang++
native_capnp_cxx=g++
```
š¬ diegoviola commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3575197345)
> Instead of linking to a bunch of threads, it could make sense to just say that the issue has been fixed upstream in Qt6, so that the workaround is no longer needed?
The `Qt::WindowStaysOnTopHint` workaround was never going to work on Wayland. Clients don't get to dictate whether they stay on top or not. The compositor does, and you (the user) are supposed to control the compositor through configuration. This is a fundamental difference that a lot of people don't seem to understand.
It se
...
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3575197345)
> Instead of linking to a bunch of threads, it could make sense to just say that the issue has been fixed upstream in Qt6, so that the workaround is no longer needed?
The `Qt::WindowStaysOnTopHint` workaround was never going to work on Wayland. Clients don't get to dictate whether they stay on top or not. The compositor does, and you (the user) are supposed to control the compositor through configuration. This is a fundamental difference that a lot of people don't seem to understand.
It se
...
š¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575205474)
For further context: The current minimum required GCC version is 12.1 (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler), however `std::format` only comes with GCC libstdc++ 13.1
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575205474)
For further context: The current minimum required GCC version is 12.1 (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler), however `std::format` only comes with GCC libstdc++ 13.1
š¬ maflcko commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575210686)
@psam21 Is this LLM generated? Have you tested this yourself?
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575210686)
@psam21 Is this LLM generated? Have you tested this yourself?
š¬ hebasto commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575216394)
The title says "depends: Fix native_capnp to respect build_CC and build_CXX", but it seems working as expected on the master:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx build_CXX=clang++
native_capnp_cxx=clang++
```
(https://github.com/bitcoin/bitcoin/pull/33937#issuecomment-3575216394)
The title says "depends: Fix native_capnp to respect build_CC and build_CXX", but it seems working as expected on the master:
```
$ gmake --no-print-directory -C depends print-native_capnp_cxx build_CXX=clang++
native_capnp_cxx=clang++
```
š¬ fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559669334)
Some of these are folders, but this will also work.
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559669334)
Some of these are folders, but this will also work.
š¬ fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559669730)
Done.
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559669730)
Done.
š¬ hulxv commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575220797)
> For further context: The current minimum required GCC version is 12.1 (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler), however `std::format` only comes with GCC libstdc++ 13.1
For that, I thought about switching first to `{fmt}` and after that, we can switch to `std::format` easily when we bump the minimum required GCC version
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575220797)
> For further context: The current minimum required GCC version is 12.1 (https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler), however `std::format` only comes with GCC libstdc++ 13.1
For that, I thought about switching first to `{fmt}` and after that, we can switch to `std::format` easily when we bump the minimum required GCC version
š¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575238806)
Yes, I understand, but there are no meaningful user-visible benefits:
* Performance does not matter, because formatting isn't done in hot loops
* A few KiB of binary size don't matter, because the binary is several MiB in size
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575238806)
Yes, I understand, but there are no meaningful user-visible benefits:
* Performance does not matter, because formatting isn't done in hot loops
* A few KiB of binary size don't matter, because the binary is several MiB in size
š¬ fanquake commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3575244180)
> I think having two options to control such a minor feature is unintuive and overkill. If that's what people like, no objection as it really isn't important. But my preference would be to put everything in the -asmap option:
I agree.
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3575244180)
> I think having two options to control such a minor feature is unintuive and overkill. If that's what people like, no objection as it really isn't important. But my preference would be to put everything in the -asmap option:
I agree.
š fanquake merged a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893)
(https://github.com/bitcoin/bitcoin/pull/33893)
š¬ hulxv commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575297752)
But from the developers' view, I think having features like compile-time error-checking and easy API and [other features](https://github.com/fmtlib/fmt?tab=readme-ov-file#features) will be helpful to have. At least it will make it a little bit easier. Maybe this is a little thing, but nice to have
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575297752)
But from the developers' view, I think having features like compile-time error-checking and easy API and [other features](https://github.com/fmtlib/fmt?tab=readme-ov-file#features) will be helpful to have. At least it will make it a little bit easier. Maybe this is a little thing, but nice to have
š¬ maflcko commented on issue "RFC: Replacing `tinyformat` with `{fmt}`":
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575329532)
There is compile-time error-checking already implemented for tinyformat in this codebase, so there is also no developer benefit of using `std::format` or `{fmt}`.
(https://github.com/bitcoin/bitcoin/issues/33942#issuecomment-3575329532)
There is compile-time error-checking already implemented for tinyformat in this codebase, so there is also no developer benefit of using `std::format` or `{fmt}`.
š¬ fanquake commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3575373453)
Please rebase this.
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3575373453)
Please rebase this.
š fanquake approved a pull request: "ci: Run GUI unit tests in cross-Windows task"
(https://github.com/bitcoin/bitcoin/pull/33919#pullrequestreview-3504833470)
ACK fa7ea497c3ef9366805e520205f2acf04d4d347b - didn't test.
(https://github.com/bitcoin/bitcoin/pull/33919#pullrequestreview-3504833470)
ACK fa7ea497c3ef9366805e520205f2acf04d4d347b - didn't test.
š fanquake merged a pull request: "ci: Run GUI unit tests in cross-Windows task"
(https://github.com/bitcoin/bitcoin/pull/33919)
(https://github.com/bitcoin/bitcoin/pull/33919)
š fanquake approved a pull request: "depends: Update Qt download link"
(https://github.com/bitcoin/bitcoin/pull/33918#pullrequestreview-3504837208)
ACK 50cbde3295b4b45d3ef2e5f787a33eea91ef38b5
(https://github.com/bitcoin/bitcoin/pull/33918#pullrequestreview-3504837208)
ACK 50cbde3295b4b45d3ef2e5f787a33eea91ef38b5
ā
fanquake closed an issue: "depends: fallback server missing Qt downloads"
(https://github.com/bitcoin/bitcoin/issues/33898)
(https://github.com/bitcoin/bitcoin/issues/33898)