Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Remove --enable-threadlocal #30137

Merged
merged 3 commits into from
May 21, 2024

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented May 18, 2024

Includes a commit from #30099.
Closes #29952.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 18, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK laanwj, hebasto, maflcko, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@laanwj
Copy link
Member

laanwj commented May 18, 2024

Concept ACK.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25126121769

@hebasto
Copy link
Member

hebasto commented May 18, 2024

Concept ACK.

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@hebasto
Copy link
Member

hebasto commented May 20, 2024

Concept ACK.

I forgot to mention that it would be great to ditch it before CMake :)

hebasto and others added 3 commits May 21, 2024 10:27
The assumption in the commit 188ca75
about the broken `thread_local` implementation in GCC was misguided
because the initial failure was not due to GCC, but a bug in the Wine
runtime, as evidenced, for example, here:
 - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=917307
 - https://bugs.freedesktop.org/show_bug.cgi?id=108662

Consequently, it is safe to re-enable `thread_local` support for
MinGW-w64 builds.
@laanwj
Copy link
Member

laanwj commented May 21, 2024

Code review ACK 17fe948
This removes, to my knowledge, all mentions of thread-local in the build system or comments.
Will test the guix binary on windows.

@DrahtBot DrahtBot requested review from hebasto and theuni May 21, 2024 11:05
@fanquake fanquake marked this pull request as ready for review May 21, 2024 11:15
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 17fe948.

Guix build:

x86_64
b5dd1ec801cb549590bb922867b9254de51c3688220ce4d29ae03077e0047448  guix-build-17fe948cce2e/output/dist-archive/bitcoin-17fe948cce2e.tar.gz
9bc0b56bf0440f04937646ded534e47df860c4ca2b9b96699c1f5b431ae71508  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/SHA256SUMS.part
3e1cceb01e86b7e47d68ae100b60747333a88aad5b352c7c3e9113d700465f1c  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-debug.zip
e0c36335cfc1c399d40231adffd1d8afd33261dc59c0523823c50ab6ee00f5e8  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-setup-unsigned.exe
8a0044e4e5357e4e0eb1577a704a4cc0ddb46617151eee607842010c120738a8  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-unsigned.tar.gz
de79709714a39ebcb5988fad5add91e7ed71e9fc313143f0cca5dcc6814dd34b  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64.zip

@maflcko
Copy link
Member

maflcko commented May 21, 2024

utACK 17fe948

@fanquake
Copy link
Member Author

Guix Build (aarch64):

77a67096a2225cafa44637d69e9618cb275b26eeda00585dfa246dfbec7fc818  guix-build-17fe948cce2e/output/aarch64-linux-gnu/SHA256SUMS.part
57867960d6c8597648cc49efe4165e543b1181ad0a3a97a16f66bd41b93afe8b  guix-build-17fe948cce2e/output/aarch64-linux-gnu/bitcoin-17fe948cce2e-aarch64-linux-gnu-debug.tar.gz
76c872bcb98e8fc46a5cb2ebd13c489caa6b7d3b0b6db92c467aae27b30308fc  guix-build-17fe948cce2e/output/aarch64-linux-gnu/bitcoin-17fe948cce2e-aarch64-linux-gnu.tar.gz
5093f7155c56e94ad994dcbe586fef1b8cfedc214e037a4dfdb1b2012059b5c9  guix-build-17fe948cce2e/output/arm-linux-gnueabihf/SHA256SUMS.part
06f2bb56d6aae8b1dc70e5d09a111739a7d34b8b07797f674f6c17fbf7ec74ab  guix-build-17fe948cce2e/output/arm-linux-gnueabihf/bitcoin-17fe948cce2e-arm-linux-gnueabihf-debug.tar.gz
83a5f2bcb1ecd4e7cf1cc85db394396aaf46b92252555f42237a44cde0bc9b96  guix-build-17fe948cce2e/output/arm-linux-gnueabihf/bitcoin-17fe948cce2e-arm-linux-gnueabihf.tar.gz
13c6c28083906e8ac81d4f320a6d4d9f7508919530d33f371596baaea8eba066  guix-build-17fe948cce2e/output/arm64-apple-darwin/SHA256SUMS.part
0dcf263fb0c3945af210977dd5a363e3cff162c4a7b47d8fed3cef4b708b9365  guix-build-17fe948cce2e/output/arm64-apple-darwin/bitcoin-17fe948cce2e-arm64-apple-darwin-unsigned.tar.gz
3c9da1723c61bcab2e6a856f0a7cbb3ad53cea7b18b8761b1d50d0cfd3012305  guix-build-17fe948cce2e/output/arm64-apple-darwin/bitcoin-17fe948cce2e-arm64-apple-darwin-unsigned.zip
7b6debab244c7c50e6eb47c180bc149de511f8b2304870c5e2347fdac3107fa1  guix-build-17fe948cce2e/output/arm64-apple-darwin/bitcoin-17fe948cce2e-arm64-apple-darwin.tar.gz
b5dd1ec801cb549590bb922867b9254de51c3688220ce4d29ae03077e0047448  guix-build-17fe948cce2e/output/dist-archive/bitcoin-17fe948cce2e.tar.gz
5743e13fa3b8505a66412f1845e8b0d8f908822b417b4d94412fb381b30db64a  guix-build-17fe948cce2e/output/powerpc64-linux-gnu/SHA256SUMS.part
c394fe9822cba9275d17dc040772325e8c4ae091b3dc4137a3b3856af9358028  guix-build-17fe948cce2e/output/powerpc64-linux-gnu/bitcoin-17fe948cce2e-powerpc64-linux-gnu-debug.tar.gz
5d5ed565449d736520ee8d8d2f15bb55c99f6bea2b5a8bebc30aca75055f67c7  guix-build-17fe948cce2e/output/powerpc64-linux-gnu/bitcoin-17fe948cce2e-powerpc64-linux-gnu.tar.gz
f5565a2e56d65c262f561b9ea509afd22f4d6ff55907cc259fe114dd6880f7d8  guix-build-17fe948cce2e/output/riscv64-linux-gnu/SHA256SUMS.part
8821cd02599d6d26858d7414147ad93e8c7c9f6ff272fde44162dc291332f3ad  guix-build-17fe948cce2e/output/riscv64-linux-gnu/bitcoin-17fe948cce2e-riscv64-linux-gnu-debug.tar.gz
ea4844a3048eda3c31339754ef42efc30a5b2def1b776ffab990add0c523e6a2  guix-build-17fe948cce2e/output/riscv64-linux-gnu/bitcoin-17fe948cce2e-riscv64-linux-gnu.tar.gz
cd0e7b8739c7039205dfc714df6889646a4f5bb55e0cb1819797990bf96fc8f0  guix-build-17fe948cce2e/output/x86_64-apple-darwin/SHA256SUMS.part
d680caba616d24ef7ec28a7100d093ae428908d1b12ea27352fb575269a49c87  guix-build-17fe948cce2e/output/x86_64-apple-darwin/bitcoin-17fe948cce2e-x86_64-apple-darwin-unsigned.tar.gz
fb234d002cb3c68e7bba82cbb10677e4473f24a617f09e2c17f0b69dd2c47fa3  guix-build-17fe948cce2e/output/x86_64-apple-darwin/bitcoin-17fe948cce2e-x86_64-apple-darwin-unsigned.zip
15c310bef26654d0a9a7098a54593fb58266d7de2d5be28a307d98e64716fe83  guix-build-17fe948cce2e/output/x86_64-apple-darwin/bitcoin-17fe948cce2e-x86_64-apple-darwin.tar.gz
4ba52b40fad3b0ccb3a4c9a812a5dc4aab0047b2a908dd18f58bf417a71c60e3  guix-build-17fe948cce2e/output/x86_64-linux-gnu/SHA256SUMS.part
f5eba6a58f87896795928c5010cbee34453b4fa223e37a6b0cb7da65327f8afd  guix-build-17fe948cce2e/output/x86_64-linux-gnu/bitcoin-17fe948cce2e-x86_64-linux-gnu-debug.tar.gz
af9816b48a68116a61ba2e2af1a22f5aa508b7f699acecfb80b31ef8da976c69  guix-build-17fe948cce2e/output/x86_64-linux-gnu/bitcoin-17fe948cce2e-x86_64-linux-gnu.tar.gz
9bc0b56bf0440f04937646ded534e47df860c4ca2b9b96699c1f5b431ae71508  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/SHA256SUMS.part
3e1cceb01e86b7e47d68ae100b60747333a88aad5b352c7c3e9113d700465f1c  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-debug.zip
e0c36335cfc1c399d40231adffd1d8afd33261dc59c0523823c50ab6ee00f5e8  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-setup-unsigned.exe
8a0044e4e5357e4e0eb1577a704a4cc0ddb46617151eee607842010c120738a8  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64-unsigned.tar.gz
de79709714a39ebcb5988fad5add91e7ed71e9fc313143f0cca5dcc6814dd34b  guix-build-17fe948cce2e/output/x86_64-w64-mingw32/bitcoin-17fe948cce2e-win64.zip

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 17fe948

@fanquake fanquake merged commit 2ec0a28 into bitcoin:master May 21, 2024
16 checks passed
@fanquake fanquake deleted the remove_build_thread_local branch May 21, 2024 20:17
@DrahtBot
Copy link
Contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File commit a786fd2
(master)
commit 6498951
(master and this pull)
SHA256SUMS.part 14f282b5bf7ebcd4... d2b11fe03f620927...
*-aarch64-linux-gnu-debug.tar.gz b2364625629230d1... 2dc133001ba32c41...
*-aarch64-linux-gnu.tar.gz b05fc0561baa0966... ca83cce58855e8fd...
*-arm-linux-gnueabihf-debug.tar.gz 4545637daf89882c... f5a547a8e76ad429...
*-arm-linux-gnueabihf.tar.gz 27115f4c7e251175... 98807d914e281f9b...
*-arm64-apple-darwin-unsigned.tar.gz cf3950f9d05fb1c0... eaacda993a335185...
*-arm64-apple-darwin-unsigned.zip b1c32e004722a3b4... 063170bd94f3b1ed...
*-arm64-apple-darwin.tar.gz f09a38acef4b141b... 55ad1ef99ba5c4d9...
*-powerpc64-linux-gnu-debug.tar.gz 75cfe8d73120b00d... a7df880ae406cbe7...
*-powerpc64-linux-gnu.tar.gz d4f8c1296886fac4... c7e83c889c0d3a43...
*-riscv64-linux-gnu-debug.tar.gz a8f56aee14e85f29... a455fad048c5e7ee...
*-riscv64-linux-gnu.tar.gz d8a4ae4295e6aa9f... 14d3728c6525ad17...
*-x86_64-apple-darwin-unsigned.tar.gz ff9ae232120c85f9... 42fdf06b2a1dfdba...
*-x86_64-apple-darwin-unsigned.zip 9917159a6fd59954... e7d6a12616f63b91...
*-x86_64-apple-darwin.tar.gz 61112274cace86c4... de4fd675e3ef30f3...
*-x86_64-linux-gnu-debug.tar.gz 557d05bc585a2910... 54eabd6e7f0e4a49...
*-x86_64-linux-gnu.tar.gz 4928355769a494d7... da0144b6aeabbc01...
*.tar.gz 49c2acd89613cf65... e296b8be8a64650c...
guix_build.log 88d5371b29f8bb75... f2b4d1ac803fde2b...
guix_build.log.diff 4ced442c8b170aa4...

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 17fe948

Comment on lines -55 to 53
#if !defined(HAVE_THREAD_LOCAL)
// This test doesn't apply to platforms where we don't have thread_local.
return;
#endif

std::set<std::string> names = RenameEnMasse(100);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this it will be possible to remove export NOWARN_CXXFLAGS="-Wno-error=unreachable-code" from dev environment on FreeBSD, thanks!

hebasto added a commit to hebasto/bitcoin that referenced this pull request May 25, 2024
af7c6ed fixup! cmake: Build `bitcoin_util` static library (Hennadii Stepanov)

Pull request description:

  This PR ports bitcoin#30137.

  The `target_compile_definitions` command is to be deleted during the next sync/rebase round. Such an early porting is needed for #93 in order to delete the only non-config related generator expression from the compile definitions, which simplifies the summary code a lot.

  Effectively, this commit has been split from #93.

Top commit has no ACKs.

Tree-SHA512: 0062abb77fea3c1f6800452a0a0c9bff39cc357b09e3f2a0e25eaa9961faad515b493dd3f6f217edcb5d43830cd16d953b99ae52c02a3a2fe56fc911dcf79154
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rethink thread_local (take 2)
7 participants