build: use -isysroot over --sysroot on macOS
#21793
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tested ACK 6777543 on macOS 11.2.3 (Big Sur) arm-apple-darwin20.3.0. |
fanquake
force-pushed
the
macos_use_isysroot
branch
from
6777543 to
1f3932e
Compare
hebasto
reviewed
Apr 28, 2021
There was a problem hiding this comment.
Approach ACK 1f3932e.
Tested for builds with depends:
- macOS Big Sur 11.3 (20E232)
% otool -l src/qt/bitcoin-qt | grep -A 7 -B 1 BUILD_VERSION
Load command 9
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk 11.3
ntools 1
tool 3
version 650.9
- macOS Mojave 10.14.6 (18G9028)
$ otool -l src/qt/bitcoin-qt | grep -A 7 -B 1 BUILD_VERSION
Load command 9
cmd LC_BUILD_VERSION
cmdsize 32
platform macos
sdk 10.14
minos 10.14
ntools 1
tool ld
version 450.3
The dark mode works in both cases above.
nit: This change could be a scripted-diff.
Our docs should be updated as well.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
hebasto
approved these changes
Apr 28, 2021
There was a problem hiding this comment.
ACK 1f3932e
nit: Why not being consistent about absence/presence a space after -isysroot? Clang docs suggests the former.
Additionally to my previous tests, tested cross-compiling on Linux Mint 20.1 (x86_64):
- this PR
$ depends/x86_64-apple-darwin18/native/bin/x86_64-apple-darwin18-otool -l src/qt/bitcoin-qt | grep -A 7 -B 1 BUILD_VERSION
Load command 9
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk 505.0
ntools 1
tool 3
version 530.0
- build: macOS toolchain bump #19817 rebased on top of this PR
$ depends/x86_64-apple-darwin18/native/bin/x86_64-apple-darwin18-otool -l src/qt/bitcoin-qt | grep -A 7 -B 1 BUILD_VERSION
Load command 9
cmd LC_BUILD_VERSION
cmdsize 32
platform 1
minos 10.14
sdk 10.15.6
ntools 1
tool 3
version 609.0
The dark mode works in both cases above.
Clangs Darwin driver will infer the deployment target from the SDK and use other SDK info when parsing arguments to the linker.
It seems works in that way since llvm clang v8.0.0.
jarolrod
reviewed
Apr 28, 2021
There was a problem hiding this comment.
ACK 1f3932e
Tested this PR by performing the following for each supported macOS version (11, 10.15, 10.14) and checking SDK values and if dark mode works:
- Native Depends Build
- Native Depends Build with Toolchain Bump
- Cross Compile from Linux
- Cross Compile with Toolchain Bump
I have the branch with this PR + Toolchain Bump here: https://github.com/jarolrod/bitcoin/tree/21793_and_19817
fanquake
force-pushed
the
macos_use_isysroot
branch
from
1f3932e to
cf971c9
Compare
Addressed both. |
hebasto
approved these changes
Apr 29, 2021
There was a problem hiding this comment.
re-ACK cf971c9, only rebased and addressed comments since my previous review.
Guix build:
$ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
fcb1b98f431d6467ea2fd77ba7d05b24012d84959845945fa2bed4aaeab92fad guix-build-cf971c9ea0e8/output/dist-archive/bitcoin-cf971c9ea0e8.tar.gz
0982cc7b542cf0e6e03a48d30008945ffb2083625d72a907dd02a675c9cb1309 guix-build-cf971c9ea0e8/output/x86_64-apple-darwin18/bitcoin-cf971c9ea0e8-osx-unsigned.dmg
5d3087fb46f458eb34f28e07aa39db67c8b48420de319dc6aaea7e7bade78247 guix-build-cf971c9ea0e8/output/x86_64-apple-darwin18/bitcoin-cf971c9ea0e8-osx-unsigned.tar.gz
a4bdee56b3bf2bff2ae3e4061a173e5e9b2f4d264161dceb4ef6496c87ed4f89 guix-build-cf971c9ea0e8/output/x86_64-apple-darwin18/bitcoin-cf971c9ea0e8-osx64.tar.gz
Gitian build:
Generating report
cfc2a1ac9187bf7438ec1e45e057ffdf6811b9b0503107a2d3e8727f81083001 bitcoin-cf971c9ea0e8-osx-unsigned.dmg
b01df9e37555c31608f2e6df6bccdceb53064dd067733d31e1edcbdacdaa57dd bitcoin-cf971c9ea0e8-osx-unsigned.tar.gz
f3b3a9cf79e8b6671a88e97de2b886fb853445919cd333ba9e4fd0ebf193ee99 bitcoin-cf971c9ea0e8-osx64.tar.gz
fcb1b98f431d6467ea2fd77ba7d05b24012d84959845945fa2bed4aaeab92fad src/bitcoin-cf971c9ea0e8.tar.gz
8c436ea239b03f75aad8f5062dd751c72acbe03ec48bf4a5503d49ffd69b0edb bitcoin-core-osx-22-res.yml
Done.
UPDATE: another gitian build (with guaranteed clear cache):
Generating report
cfc2a1ac9187bf7438ec1e45e057ffdf6811b9b0503107a2d3e8727f81083001 bitcoin-cf971c9ea0e8-osx-unsigned.dmg
b01df9e37555c31608f2e6df6bccdceb53064dd067733d31e1edcbdacdaa57dd bitcoin-cf971c9ea0e8-osx-unsigned.tar.gz
f3b3a9cf79e8b6671a88e97de2b886fb853445919cd333ba9e4fd0ebf193ee99 bitcoin-cf971c9ea0e8-osx64.tar.gz
fcb1b98f431d6467ea2fd77ba7d05b24012d84959845945fa2bed4aaeab92fad src/bitcoin-cf971c9ea0e8.tar.gz
2cd1011b1c37a6fd403884e66175376313285a3bd9119995fe04df975080b98a bitcoin-core-osx-22-res.yml
Done.
|
Guix builds: bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
fcb1b98f431d6467ea2fd77ba7d05b24012d84959845945fa2bed4aaeab92fad guix-build-cf971c9ea0e8/output/dist-archive/bitcoin-cf971c9ea0e8.tar.gz
0982cc7b542cf0e6e03a48d30008945ffb2083625d72a907dd02a675c9cb1309 guix-build-cf971c9ea0e8/output/x86_64-apple-darwin18/bitcoin-cf971c9ea0e8-osx-unsigned.dmg
5d3087fb46f458eb34f28e07aa39db67c8b48420de319dc6aaea7e7bade78247 guix-build-cf971c9ea0e8/output/x86_64-apple-darwin18/bitcoin-cf971c9ea0e8-osx-unsigned.tar.gz
a4bdee56b3bf2bff2ae3e4061a173e5e9b2f4d264161dceb4ef6496c87ed4f89 guix-build-cf971c9ea0e8/output/x86_64-apple-darwin18/bitcoin-cf971c9ea0e8-osx64.tar.gzGitian builds (Done twice, second with a cleared cache): # macOS:
71fd96f0d87488a473c577bac1248085c48c4d925a6c3be09a5ea0adce6b54ae bitcoin-cf971c9ea0e8-osx-unsigned.dmg
994a5175ce8dcba226976a708dd43cc459f891e11857c783f54b3e8c9f1570c4 bitcoin-cf971c9ea0e8-osx-unsigned.tar.gz
ae732d49acc56a44ddee1aa228d6c177df1473a824f65b780d15fd8613cc9801 bitcoin-cf971c9ea0e8-osx64.tar.gz
fcb1b98f431d6467ea2fd77ba7d05b24012d84959845945fa2bed4aaeab92fad src/bitcoin-cf971c9ea0e8.tar.gz
13c6f297c299f45a9a7ea6f7f5284afdd155d58c6e9cbc12a47064e1e4d22b2a bitcoin-core-osx-22-res.yml |
|
tACK cf971c9 I built depends on macOS 11.3 Big Sur (XCode 12.5, Intel CPU) with the whole kitchen-sink
Also cross-compiled on Linux. (I used the previous push 1f3932e on macOS, and cf971c9 on Linux) |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 1, 2021
cf971c9 build: use -isysroot over --sysroot on macOS (fanquake) Pull request description: Not only does this seem to be the more correct behaviour when targeting Darwin, but if you use `-isysroot`, Clangs Darwin driver will [infer the deployment target](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L1652) from the SDK and use other SDK info when parsing arguments to the linker. In the case of [`-platform_version`](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L2656), which is added if the linker is [new enough](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L342), the version tuple is constructed from the SDKInfo, and SDKInfo, as far as I can tell, only exists when `-isysroot` has been passed, see [parseSDKSettings](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L1785) As a result, the SDK version field in the `LC_BUILD_VERSION` command is filled out. i.e when building master: ```bash cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 10.14 sdk n/a ntools 1 tool 3 version 650.9 ``` vs this PR: ```bash cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 10.14 sdk 11.3 ntools 1 tool 3 version 650.9 ``` This, from what I understand, will fix the issue we are having with Qt deciding wether or not to enable features like "Dark mode" on macOS, see bitcoin#21771, however I have not tested that. Thus this is an alternative to bitcoin#21782. Our usage of `--sysroot` was added in bitcoin#17118. ```bash -isysroot <dir> Set the system root directory (usually /) ``` ACKs for top commit: Sjors: tACK cf971c9 hebasto: re-ACK cf971c9, only rebased and addressed comments since my [previous](bitcoin#21793 (review)) review. Tree-SHA512: f01138179fb85083b5505bbaa48810451098ffa4da5d3c9b673785448790aa76f2e64b2aab6e698f6ee378a21f70626445a3fabee7c61dbfc44e96f3e3964656
This was referenced May 1, 2021
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
May 5, 2021
Sanity checking that this info is present, and the version we expect is worthwhile, as it can determine runtime behaviour. See bitcoin#21771 and bitcoin#21793.
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Jul 20, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Jul 20, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Jul 20, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Jul 20, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 1, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 24, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 24, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 24, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 24, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 25, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 25, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 26, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 26, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 26, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 27, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Aug 30, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 1, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 1, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 2, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 3, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 3, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 3, 2021
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Sep 4, 2021
DeckerSU
added a commit
to DeckerSU/KomodoOcean
that referenced
this pull request
Nov 16, 2021
TheComputerGenie
pushed a commit
to TheComputerGenie/KomodoOcean
that referenced
this pull request
Jan 5, 2022
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
May 4, 2022
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not only does this seem to be the more correct behaviour when targeting Darwin, but if you use
-isysroot, Clangs Darwin driver will infer the deployment target from the SDK and use other SDK info when parsing arguments to the linker. In the case of-platform_version, which is added if the linker is new enough, the version tuple is constructed from the SDKInfo, and SDKInfo, as far as I can tell, only exists when-isysroothas been passed, see parseSDKSettingsAs a result, the SDK version field in the
LC_BUILD_VERSIONcommand is filled out. i.e when building master:cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 10.14 sdk n/a ntools 1 tool 3 version 650.9vs this PR:
cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 10.14 sdk 11.3 ntools 1 tool 3 version 650.9This, from what I understand, will fix the issue we are having with Qt deciding wether or not to enable features like "Dark mode" on macOS, see #21771, however I have not tested that. Thus this is an alternative to #21782.
Our usage of
--sysrootwas added in #17118.