depends: modernize clang flags for Darwin #27798
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK. |
|
@fanquake and @hebasto have both pointed out to me in discussions that this works even without the By sheer luck though, they manage to put everything in the correct order such that the poisoned paths come last in the search-order. That feels brittle, but maybe good enough. I'd also like to see what the consequences of the |
I tested this in detail today. I minimally patched clang-12 and built from depends with and without the patch. I've verified that at least bitcoind, bitcoiin-qt, and univalue/test/object (one of the c-i offenders) are all identical. config.log shows a few differences which boil down to: -ax_cv_PTHREAD_CLANG_NO_WARN_FLAG=-Qunused-arguments
+ax_cv_PTHREAD_CLANG_NO_WARN_FLAG=noBecause a few tests are thrown off by the warning. And the end-result is: -PTHREAD_CFLAGS='-Qunused-arguments -pthread'
+PTHREAD_CFLAGS='-pthread'So.. only cosmetic, and nothing that changes the final binaries. I propose that we take this commit as-is, with one on top to keep c-i from failing for this particular warning. So we just need to stick a I think that makes sense because eventually (once clang17+ becomes the norm) everything will work perfectly, and before then there may just be a few harmless warnings. @fanquake Thoughts? If you agree, where's the best place to disable the Werror? |
I think it's best to just suppress in |
theuni
force-pushed
the
darwin-cross-modernize-flags
branch
from
1c0c344 to
296a32e
Compare
Done. Scoped to LDFLAGS because that should be enough. We'll see if the c-i likes it. |
|
Failures are unrelated, macOS test is now happy. |
clang <=17 warns on -nostdlibinc, which causes an error on our -Werror builds. Note that this breaks the "-fPIE" check in configure because it relies on catching warnings, but that is not a problem for macOS.
Fixes builds with llvm >= 11 in guix by working around the problem. As a bonus, this is much cleaner and more maintainable than what we had before.
theuni
force-pushed
the
darwin-cross-modernize-flags
branch
from
296a32e to
cbee1d7
Compare
|
Guix Build: 597dfa270c4fc66692222de6f88862b2df5889ab95e6912814a0eb4fbdc08ef7 guix-build-cbee1d70918b/output/aarch64-linux-gnu/SHA256SUMS.part
9132ca580774acbd977d74a78cd6f0fbf7d1006a16cc9249c953ba3720833085 guix-build-cbee1d70918b/output/aarch64-linux-gnu/bitcoin-cbee1d70918b-aarch64-linux-gnu-debug.tar.gz
9835f60101b801cebb2f04157041b59b01d78cc9b732adc6c6b07ec52ab4242f guix-build-cbee1d70918b/output/aarch64-linux-gnu/bitcoin-cbee1d70918b-aarch64-linux-gnu.tar.gz
1dc24ddd70c76e1ca33d76724ca780aa35408cec5533194083bebd695524eeff guix-build-cbee1d70918b/output/arm-linux-gnueabihf/SHA256SUMS.part
b20349a3db7e45e942d83795c7cd0df40dd179729c37cad9d3d063f7a8e216fc guix-build-cbee1d70918b/output/arm-linux-gnueabihf/bitcoin-cbee1d70918b-arm-linux-gnueabihf-debug.tar.gz
b83d8d4cf207ec990a8a313ff894d20d5ceb66f14400ff8d8cbf42ae6761ac86 guix-build-cbee1d70918b/output/arm-linux-gnueabihf/bitcoin-cbee1d70918b-arm-linux-gnueabihf.tar.gz
26cdfe05fc51c248995a8a681cde88b5117b3c60df283ca6e5f146b2ac2eac13 guix-build-cbee1d70918b/output/arm64-apple-darwin/SHA256SUMS.part
3ce6abf6036e3651afa920724266eb0160778633beef34e193868e441c4619ae guix-build-cbee1d70918b/output/arm64-apple-darwin/bitcoin-cbee1d70918b-arm64-apple-darwin-unsigned.dmg
147738bf6a27712f3d55815f92c288626bf7d348bd9492b087b0883d71ed3ac9 guix-build-cbee1d70918b/output/arm64-apple-darwin/bitcoin-cbee1d70918b-arm64-apple-darwin-unsigned.tar.gz
05dd239ff2f10212595b60ab786717f597dcbf111a1200450a92d69ec9f46b8f guix-build-cbee1d70918b/output/arm64-apple-darwin/bitcoin-cbee1d70918b-arm64-apple-darwin.tar.gz
8c3bd6d8f3f44a5587040e7bdd162823091402ee73aeb1bb2ab68b4837770899 guix-build-cbee1d70918b/output/dist-archive/bitcoin-cbee1d70918b.tar.gz
21cb5c1fa7dde9bb0c6d664b6a03c2327f10a1e6abbf678a0239b4a7e55be931 guix-build-cbee1d70918b/output/powerpc64-linux-gnu/SHA256SUMS.part
cdfb60ad7f2b238a51d220be388f4e5ca22338aa571f3310beda3a5b06a34828 guix-build-cbee1d70918b/output/powerpc64-linux-gnu/bitcoin-cbee1d70918b-powerpc64-linux-gnu-debug.tar.gz
4ff45ea00ba4fd34676aaea40b830bacb15f91e7ec8282a4e4c50757b84aa9b5 guix-build-cbee1d70918b/output/powerpc64-linux-gnu/bitcoin-cbee1d70918b-powerpc64-linux-gnu.tar.gz
568a714dcd7a61377f1f2e750fd1aee31e03e5112819157f9cbc12a476e6a42b guix-build-cbee1d70918b/output/powerpc64le-linux-gnu/SHA256SUMS.part
a7bbc58da1ff3895ed97e09751da881c0a0c0175bce53fec656764096ff4b6fa guix-build-cbee1d70918b/output/powerpc64le-linux-gnu/bitcoin-cbee1d70918b-powerpc64le-linux-gnu-debug.tar.gz
7e96548895e240fa36e6d07871021d445b23b8dbc776d9494938a4fe1bff8aaf guix-build-cbee1d70918b/output/powerpc64le-linux-gnu/bitcoin-cbee1d70918b-powerpc64le-linux-gnu.tar.gz
ee9cbece1e8b5fde078f846bd43ad4cbb1e1e327a2405365e1225d4352a61a2b guix-build-cbee1d70918b/output/riscv64-linux-gnu/SHA256SUMS.part
2150bd12cae0cdd3de889d64af292602ea0a82c34979b917d98349a67b215614 guix-build-cbee1d70918b/output/riscv64-linux-gnu/bitcoin-cbee1d70918b-riscv64-linux-gnu-debug.tar.gz
075afbd5f88d32e0aed1b659fb6118f113e1a14fc004a03ea238f31aca5c3880 guix-build-cbee1d70918b/output/riscv64-linux-gnu/bitcoin-cbee1d70918b-riscv64-linux-gnu.tar.gz
cdd31a69d1b00a6565a4642608f5d23010cba9fc5006548fabc36a874e284f0b guix-build-cbee1d70918b/output/x86_64-apple-darwin/SHA256SUMS.part
9b08280e2f8299830b0bab052071a9c6af3accf27dc6601420eec1f504cd3bee guix-build-cbee1d70918b/output/x86_64-apple-darwin/bitcoin-cbee1d70918b-x86_64-apple-darwin-unsigned.dmg
b19b8185f1525aba6e11acd68603d05de012e9589a1a650b1747bb8c28e5c5eb guix-build-cbee1d70918b/output/x86_64-apple-darwin/bitcoin-cbee1d70918b-x86_64-apple-darwin-unsigned.tar.gz
7b30ac37734950e47e62f5df7249a7dbda17bcde17f0f87abf256f96023f16bd guix-build-cbee1d70918b/output/x86_64-apple-darwin/bitcoin-cbee1d70918b-x86_64-apple-darwin.tar.gz
e58a695934e4a4425af64bd75c34662f88dfb4b18873bc62a1c3847c25130c34 guix-build-cbee1d70918b/output/x86_64-linux-gnu/SHA256SUMS.part
89c771455f0b72cd7f110eacf15fb694b871bbb70f7d23a9e5f5deec18d39c97 guix-build-cbee1d70918b/output/x86_64-linux-gnu/bitcoin-cbee1d70918b-x86_64-linux-gnu-debug.tar.gz
436dc7634b41ec09e2426d0cf8bea238994d9a4e4fcc074772919db0ac96434e guix-build-cbee1d70918b/output/x86_64-linux-gnu/bitcoin-cbee1d70918b-x86_64-linux-gnu.tar.gz
63743df509e31704539b5ee87036a4a3c5933ae3bfecf825fedb15365021f010 guix-build-cbee1d70918b/output/x86_64-w64-mingw32/SHA256SUMS.part
b138acae44967c121749b01ac91fbbeb41b0b6d336f13b8de54dbfaac8b9b9a1 guix-build-cbee1d70918b/output/x86_64-w64-mingw32/bitcoin-cbee1d70918b-win64-debug.zip
2aae3ce0dee19e3a2dac884d1e18ea44bfbdaeffb37cce30c81693c6236fa937 guix-build-cbee1d70918b/output/x86_64-w64-mingw32/bitcoin-cbee1d70918b-win64-setup-unsigned.exe
bf0512130cbcf7da7b35370db62a741d3acdc5d19be465dbce849e60ff565377 guix-build-cbee1d70918b/output/x86_64-w64-mingw32/bitcoin-cbee1d70918b-win64-unsigned.tar.gz
2bdb86b1a8cff65684f01652fda85046d02f8a0f7beff975415649bc64e196c4 guix-build-cbee1d70918b/output/x86_64-w64-mingw32/bitcoin-cbee1d70918b-win64.zip |
There was a problem hiding this comment.
ACK cbee1d7 - tested Guix and the depends cross-compile. Would like to move this along, to unblock #27676, which itself might be a blocker for #27897. Note that macOS might seem somewhat in flux for the moment, but once we finish the migration to LLVM Clang + LLD, things will be must simpler, and ultimately more maintainable.
|
There was a problem hiding this comment.
ACK cbee1d7
This indeed seems "more modern". I did check that this still compiles on my systems, did not verify if the binaries actually run though.
Summary: > depends: fix osx build with clang 16 > > For some reason the previous syntax worked with clang 15 and below, but > clang 16 requires that the option and value are properly separated. > depends: remove redundant stdlib option > > Use of -stdlib++-isystem gets rid of any system c++ header include paths and > negates the need for this option. In newer versions of clangs the combo > produces a warning. > depends: modernize clang flags > > Fixes builds with llvm >= 11 in guix by working around the problem. As a bonus, > this is much cleaner and more maintainable than what we had before. This is a backport of [[bitcoin/bitcoin#27328 | core#27328]], [[bitcoin/bitcoin#27721 | core#27721]] and [[bitcoin/bitcoin#27798 | core#27798]] The change to ci/test/00_setup_env_mac.sh in [[bitcoin/bitcoin#27798 | core#27798]] to ignore -Wunused-command-line-argument warnings seems equivalent to D3792 Test Plan: check that gitian-osx still works Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15579
This is a cleaner and simpler alternative to #25098. Inspired by this conversation. The diff is large but the change itself is quite small.
Fixes builds with llvm >= 11 in guix by working around the problem. As a bonus, this is much cleaner and more maintainable than what we had before.
See the updated comment for more info. At a high level: rather than playing tricks and trying to work around clang's default includes, disable them and re-add what we want.