Bitcoin Core Github
44 subscribers
120K links
Download Telegram
๐Ÿ’ฌ Ferrydh commented on issue "ci: Cleanup ci/test/01_base_install.sh":
(https://github.com/bitcoin/bitcoin/issues/27321#issuecomment-1484136940)
่ฟ™ๆ˜ฏๆฅ่‡ชQQ้‚ฎ็ฎฑ็š„ๅ‡ๆœŸ่‡ชๅŠจๅ›žๅค้‚ฎไปถใ€‚
 
ๆ‚จๅฅฝ๏ผŒๆˆ‘ๆœ€่ฟ‘ๆญฃๅœจไผ‘ๅ‡ไธญ๏ผŒๆ— ๆณ•ไบฒ่‡ชๅ›žๅคๆ‚จ็š„้‚ฎไปถใ€‚ๆˆ‘ๅฐ†ๅœจๅ‡ๆœŸ็ป“ๆŸๅŽ๏ผŒๅฐฝๅฟซ็ป™ๆ‚จๅ›žๅคใ€‚
๐Ÿ’ฌ hebasto commented on pull request "util: implement `noexcept` move assignment & move ctor for `prevector`":
(https://github.com/bitcoin/bitcoin/pull/27334#issuecomment-1484141370)
Concept ACK. Thanks for picking [this](https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1108186877) up.
๐Ÿ’ฌ martinus commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1148585918)
I've seen this happening to computers before, but never when bitcoind was running. Usually that happens when time synchronization goes wrong for whatever reason.
๐Ÿ’ฌ martinus commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1148594367)
nit: maybe add `+ 2` to the reserve because when the next `if` is true 2 more will be added. Otherwise the next `emplace_back` will most likely cause a capacity change
๐Ÿ’ฌ vstoyanov commented on issue "ci: Cleanup ci/test/01_base_install.sh":
(https://github.com/bitcoin/bitcoin/issues/27321#issuecomment-1484167539)
False alarm. The `bash -c` hack also worked for dnf the same way it did for apt, so now it is both not a big change and has no SuppressWarnings BS. I could still push the arrayification of $PACKAGES and $CI_BASE_PACKAGES though?
๐Ÿ’ฌ martinus commented on pull request "fix initialization in FastRandomContext: removes undefined behavior & uninitialized read":
(https://github.com/bitcoin/bitcoin/pull/23169#issuecomment-1484180043)
Now that #26153 was merged this is no relevant
โœ… martinus closed a pull request: "fix initialization in FastRandomContext: removes undefined behavior & uninitialized read"
(https://github.com/bitcoin/bitcoin/pull/23169)
๐Ÿ’ฌ hebasto commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1484197246)
Updated 09383b612216b63f2aec0f1fffc889989c66f212 -> 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 ([pr26642.12](https://github.com/hebasto/bitcoin/commits/pr26642.12) -> [pr26642.13](https://github.com/hebasto/bitcoin/commits/pr26642.13), [diff](https://github.com/hebasto/bitcoin/compare/pr26642.12..pr26642.13)):

- addressed @martinus's [comment](https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1148594367)
๐Ÿ’ฌ hebasto commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1148626538)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1484197246).
๐Ÿ’ฌ Ayush170-Future commented on issue "ci: Cleanup ci/test/01_base_install.sh":
(https://github.com/bitcoin/bitcoin/issues/27321#issuecomment-1484198989)
I guess SuppressWarnings was not a good practice. It's great that you found a workaround.

Instead of giving package strings as an array, I believe the bash -c technique should be sufficient here. @MarcoFalke, I think you wanted it that way as well.
๐Ÿ’ฌ theuni commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1484213695)
> It fixes building `make -C depends HOST=x86_64-apple-darwin FORCE_USE_SYSTEM_CLANG=1` with clang-16.

Yes, exactly this thanks.

This can be verified very simply outside of depends:
Broken:
> $ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang -c test.c -o test.o -Xclang -internal-externc-isystem/tmp
> error: unknown argument: '-internal-externc-isystem/tmp'

Working:
> $ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang -c test.c -o test.o -Xclang -intern
...
๐Ÿ‘ TheCharlatan approved a pull request: "depends: fix osx build with clang 16"
(https://github.com/bitcoin/bitcoin/pull/27328)
ACK 87afcb0029b8dab933c122fb8f7263c2e7272731
๐Ÿ’ฌ Empact commented on pull request "Torcontrol opt check":
(https://github.com/bitcoin/bitcoin/pull/25136#issuecomment-1484225313)
@vstoyanov No need to ask for permission to contribute. I'd say go for it.
๐Ÿ“ EthanHeilman opened a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
This PR is designed to address the issue https://github.com/bitcoin/bitcoin/issues/27332. The MSVC build is failing because of two bugs in how the build is configured.

The issue
====

When running `msbuild build_msvc\bitcoin.sln -property:Configuration=Release -maxCpuCount -verbosity:minima`l the build fails with following two errors.

* `C:\Users\e0\Documents\GitHub\bitcoin\src\httpserver.cpp(637,9): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,const char **,uint16_
...
๐Ÿ’ฌ hernanmarino commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1148657332)
Is there a chance that this is not the case in any situation ?
๐Ÿ’ฌ hernanmarino commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148659470)
Aren't there other scripts relying on the exported aliases ? Won't they fail ?
๐Ÿ‘ hernanmarino approved a pull request: "build, qt: Fix handling of `CXX=clang++` when building `qt` package"
(https://github.com/bitcoin/bitcoin/pull/27314)
tested ACK
๐Ÿ‘ hernanmarino approved a pull request: "log: Check that the timestamp string is non-empty to avoid undefined behavior"
(https://github.com/bitcoin/bitcoin/pull/27317)
๐Ÿ’ฌ EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1148665626)
I don't know. I would prefer a solution more in line with how this was solved in automake builds but I don't have time to learn enough about msbuild to implement it and it is currently broken in the default case.
๐Ÿ’ฌ EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1148666703)
In fact the build in CI is now failing because it expects the other function signature.

`C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\httpserver.cpp(635,56): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,char **,uint16_t *)': cannot convert argument 2 from 'const char **' to 'char **' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]`