-
Notifications
You must be signed in to change notification settings - Fork 38.7k
depends: remove Darwin ENV unsetting #30451
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
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. ConflictsNo conflicts as of last run. |
|
Concept ACK. |
|
Whoa, this greatly simplifies things. When/why where these removed in guix? I thought they were pretty fundamental there. I wonder if we should check for the use of these in depends and bail out if they're set though. |
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
b9673bf to
23c0b07
Compare
I've added some more cleanup here. We can't remove them entirely (for Darwin) in Guix, because we still need some set during depends, for QT/qmake... However, we can unset them after that, for the main build. |
|
Assuming that #29723 lands first, the commit here adding a patch for librt will be dropped, as that is also patched out of the CMake side in that PR. |
23c0b07 to
298c819
Compare
|
Dropped the (autotools) zeromq commit and rebased on top of #29723. |
|
Guix build (aarch64): 5447a6b0ed155318747354b9db7972716ba5a5c45c956ca39e979c380fe7b25c guix-build-298c8192a71e/output/arm64-apple-darwin/SHA256SUMS.part
9bc5df1ade9d2a8d9c469ffcbc72ff064c57658474a028270fbeeb4fafb7cff4 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin-unsigned.tar.gz
7aa774a9731f69379ae994a60c7fef3e33d07ebcb5f70b0cc0224e0640f638e2 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin-unsigned.zip
5e0be95ea55819731fad709dbbf75345fbac4de9e764baa40111d225fca2ecf7 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin.tar.gz
d7df88817fcc48b0eb80cdc04cd399f34629f4d15c29535a676a0eb8ebe47d9d guix-build-298c8192a71e/output/dist-archive/bitcoin-298c8192a71e.tar.gz
a6c604fcb17443886aa710884767d824a9ebf6f99a2da1ba11124fbe0c379b1c guix-build-298c8192a71e/output/x86_64-apple-darwin/SHA256SUMS.part
2cad928bd4ad6998707a667980ce609c52ef77793415ddf04d9f210a01a3c9fa guix-build-298c8192a71e/output/x86_64-apple-darwin/bitcoin-298c8192a71e-x86_64-apple-darwin-unsigned.tar.gz
bf41537390da432fd5e72428f41de96ba31862aa00773522fb9879d780e27e25 guix-build-298c8192a71e/output/x86_64-apple-darwin/bitcoin-298c8192a71e-x86_64-apple-darwin-unsigned.zip
7e869a3790854c00518c194b04c75189036b9661eeaa2f8a0797da25c3c7c073 guix-build-298c8192a71e/output/x86_64-apple-darwin/bitcoin-298c8192a71e-x86_64-apple-darwin.tar.gz |
298c819 to
bc78b59
Compare
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
contrib/guix/libexec/build.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am reading this right, this sets OBJC vars for non-darwin hosts. Is that intended? I would have expected the opposite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, yea. These should/would only be needed for macOS, and only for Qt-related code. I guess everything still currently builds even with them unset, because when we compile our objc++ code, we would never be looking in any GCC related include/std-lib/related dirs in any case? Will push some changes.
Now that we use the native compiler, and have fixed Qt, and these vars are unset it Guix, we can remove the unsetting from our compiler command here. Fixes bitcoin#21552.
bc78b59 to
bda537f
Compare
sedited
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guix build (aarch64):
e54a5bec288e01beccb7329fe623269decbac58c8fca182005660e2e0eab9937 guix-build-bda537f7c484/output/aarch64-linux-gnu/SHA256SUMS.part
c4121cc8b7367a3fa16667fce7fae752f243e502e82786be1a4defa512b807fc guix-build-bda537f7c484/output/aarch64-linux-gnu/bitcoin-bda537f7c484-aarch64-linux-gnu-debug.tar.gz
f6d64868fb78b9667fa486052ab2ade488caad03df577aa1cc4d79fa542c48ae guix-build-bda537f7c484/output/aarch64-linux-gnu/bitcoin-bda537f7c484-aarch64-linux-gnu.tar.gz
451ff70d68b6925e467dfe3ae69351004f4bfb4973d45ae8f49cd2928f795c4c guix-build-bda537f7c484/output/arm-linux-gnueabihf/SHA256SUMS.part
869f32187db9fb59f600147c57a166dcc53b78fa5297a750ae416cb01cfce8f0 guix-build-bda537f7c484/output/arm-linux-gnueabihf/bitcoin-bda537f7c484-arm-linux-gnueabihf-debug.tar.gz
61efedeaae40a022d3cb48a4389adbfa1ec1982c1165615effec2e1523d75263 guix-build-bda537f7c484/output/arm-linux-gnueabihf/bitcoin-bda537f7c484-arm-linux-gnueabihf.tar.gz
e3f6d6e8e998bf7e0dae5bfa3b8479091bad2ce87615ee8de16130c6300665cc guix-build-bda537f7c484/output/arm64-apple-darwin/SHA256SUMS.part
04ff93a466e8c875c88c57a9d31aece2443cf3f06768f5d87a56235a2c7e46cc guix-build-bda537f7c484/output/arm64-apple-darwin/bitcoin-bda537f7c484-arm64-apple-darwin-unsigned.tar.gz
97f31359809acefcdd71750f93ee821c57834ba04015659f7641509046a95ae2 guix-build-bda537f7c484/output/arm64-apple-darwin/bitcoin-bda537f7c484-arm64-apple-darwin-unsigned.zip
dcdb54e3a62720f67cede2359cb88c6e5b496727b8e33c7fcf61679436b86c6e guix-build-bda537f7c484/output/arm64-apple-darwin/bitcoin-bda537f7c484-arm64-apple-darwin.tar.gz
d2dbfd4d2e22f9b5155b6488773ead04bb8a4e2e087761122a6e08b4ae0ea006 guix-build-bda537f7c484/output/dist-archive/bitcoin-bda537f7c484.tar.gz
bfb792e4ebaf1b7d42a9f4040e751a5417b79acec24dacc364f2a042b6b19788 guix-build-bda537f7c484/output/powerpc64-linux-gnu/SHA256SUMS.part
30df3def1db919437ab1a54d283170d6a728e5199e3e9c8ee73871645c91b628 guix-build-bda537f7c484/output/powerpc64-linux-gnu/bitcoin-bda537f7c484-powerpc64-linux-gnu-debug.tar.gz
184bb34be85a50fa0f6137e17c57fc7c33c37ce10e06d067c24f45bdc49e50e3 guix-build-bda537f7c484/output/powerpc64-linux-gnu/bitcoin-bda537f7c484-powerpc64-linux-gnu.tar.gz
1f92d6f9f9de4bf44b7e1a2a74cdcab20f71297f3271613a55b9ef36924901a9 guix-build-bda537f7c484/output/riscv64-linux-gnu/SHA256SUMS.part
953506894362070665e16744d5676fa9530e3e58c3d06bbcfe4af77035bf96c9 guix-build-bda537f7c484/output/riscv64-linux-gnu/bitcoin-bda537f7c484-riscv64-linux-gnu-debug.tar.gz
63a47f60fe7fec1d2a8403a7891b349b4ffd7a2c91af57426ea4ace14dab1ca5 guix-build-bda537f7c484/output/riscv64-linux-gnu/bitcoin-bda537f7c484-riscv64-linux-gnu.tar.gz
35ca4b44a0f1d633b7a715a13d3447bade13e4405cb10aa6314a4f0f93a10cd8 guix-build-bda537f7c484/output/x86_64-apple-darwin/SHA256SUMS.part
ae9719518493ef8b411613c973cb37ecc7b21117e84e8695407f01ec948cff90 guix-build-bda537f7c484/output/x86_64-apple-darwin/bitcoin-bda537f7c484-x86_64-apple-darwin-unsigned.tar.gz
114a95bf0de85371236c1cd5769e78861396243cdfa75baad2f87572f168e358 guix-build-bda537f7c484/output/x86_64-apple-darwin/bitcoin-bda537f7c484-x86_64-apple-darwin-unsigned.zip
bc57a7676f8bfe4bdb48be5e9a5d29833db65580cbcf66fe91f145e4a3d6cd47 guix-build-bda537f7c484/output/x86_64-apple-darwin/bitcoin-bda537f7c484-x86_64-apple-darwin.tar.gz
e3597c886089cc37feee7b5683c8062b52d9629288b940c8dbdbda6ca432541c guix-build-bda537f7c484/output/x86_64-linux-gnu/SHA256SUMS.part
8b7285eb11ec0df84142ec00aa8b9708baa7a69ef9077c6ec5bb9095db7b83b6 guix-build-bda537f7c484/output/x86_64-linux-gnu/bitcoin-bda537f7c484-x86_64-linux-gnu-debug.tar.gz
eabd4a583dcf08d5712d9853dfc34da3ae9b34e66daa3bea1e919eec8d079459 guix-build-bda537f7c484/output/x86_64-linux-gnu/bitcoin-bda537f7c484-x86_64-linux-gnu.tar.gz
1197b6e04c25512f037cf0467aa08db595f78f0e19f539216ac9a1ece38af1c6 guix-build-bda537f7c484/output/x86_64-w64-mingw32/SHA256SUMS.part
70845f68e1f4444a5b0ae78d829a5cb2add8d808989942ee28aae52d36ff5ae0 guix-build-bda537f7c484/output/x86_64-w64-mingw32/bitcoin-bda537f7c484-win64-debug.zip
9756e593fc216a12a4adfb3bff4147f44c3f5e73979348176843463095b612ab guix-build-bda537f7c484/output/x86_64-w64-mingw32/bitcoin-bda537f7c484-win64-setup-unsigned.exe
f0d8fbce1c4ecc80d65c0d174d49e305b8098ac1b074dcafec8d14e61ba7f859 guix-build-bda537f7c484/output/x86_64-w64-mingw32/bitcoin-bda537f7c484-win64-unsigned.tar.gz
8083b1b4772045331bcf1a9b50676622e639fccf04f16156264a66e006b60905 guix-build-bda537f7c484/output/x86_64-w64-mingw32/bitcoin-bda537f7c484-win64.zip
|
My Guix build: |
sedited
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK bda537f
Tested the gui on macos 12.7.4
4edb2e9 fixup! cmake: Add `Maintenance` module (Hennadii Stepanov) 6196363 fixup! build: Generate `toolchain.cmake` in depends (Hennadii Stepanov) Pull request description: The bitcoin#30451 allows to simplify the CMake code. This PR drops the code that handles ENV unsetting: - in `depends/toolchain.cmake.in` - in `cmake/module/Maintenance.cmake` ACKs for top commit: m3dwards: ACK 4edb2e9 Tree-SHA512: 9fd9e84aa202f410202f2c115e85eee7ff344d84b3f00440197afbd53650362eff23ab008822e2a128867b9b5e4180ef019febc9edecf9d40687007ec0476dd8
…ons, refactor cache keys 106500c ci: add x86_64 macOS cross-compilation build to GitHub Actions (Kittywhiskers Van Gogh) 56d8a8e ci: add SDK (extracted) sources to separate SDK cache (Kittywhiskers Van Gogh) 9841155 ci: cleanup restore keys (Kittywhiskers Van Gogh) 2f28f3e ci: replace `runner.os` with adding CI `Dockerfile` to `hashFiles` (Kittywhiskers Van Gogh) 1d8868b merge bitcoin#30451: remove Darwin ENV unsetting (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6400 * ~~`WINEPREFIX` needs to be overridden because of permissions issues with GitHub Actions ([build](https://github.com/kwvg/dash/actions/runs/11952384181/job/33318706080#step:8:3297))~~ Superseded by [dash#6563](#6563) * [bitcoin#30451](bitcoin#30451) will enable `ccache` to work with macOS cross-compilation builds (i.e. they will no longer be registered as uncacheable). * After merging in [dash#6564](#6564), existing caches resulted in across-the-board build failures ([build](https://github.com/dashpay/dash/actions/runs/13290871849/job/37113644102#step:7:2768), [build](https://github.com/dashpay/dash/actions/runs/13290791962/job/37113418275#step:7:2940), [build](https://github.com/dashpay/dash/actions/runs/13289932901/job/37109498600#step:7:2177)) due to differing compiler versions (GCC 13 build on GCC 11 based cache) and differing glibc versions (2.39 build vs 2.35 cache) due to the change from `jammy` (on which the caches were built) to `noble` (the new base image for CI). To prevent this from occurring in the future, we will add CI's [`Dockerfile`](https://github.com/dashpay/dash/blob/265d9b5f9f5af07ff9da3d6fcc318b783d18caea/contrib/containers/ci/Dockerfile) to the `hashFiles()` used to generate the root of the cache key to ensure that the cache is discarded if it is modified. * `runner.os` doesn't help us as it tells us what platform the _runner_ is on and is independent from what the base image in the container is using, we only care about the latter. * The SDKs were cached separately because * The sources cache is shared between variants and the fastest runner (usually a Linux runner) will set the cache and as the download and extraction of the macOS SDK is only done when expected to target macOS, the cache associated with the shared key will not have the requisite SDKs. Then when that cache key is used to pull the sources cache by the macOS build, it will succeed due to a cache hit and then fail building because the compiler cannot locate the SDK. * The build cache is expected to have a higher churn rate as it is tied to the platform, which will trigger more than necessary downloads of the SDK. * If placed here, the cache would need to be re-evaluated if the version of Xcode has been bumped, which involves modifying `depends/hosts/darwin.mk`. Meaning, that file needs to be included when computing `hashFiles()`. But this also would result in unnecessary cache misses for all the platforms that _aren't_ macOS (because the structure of the build key is the same among all variants). ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 106500c PastaPastaPasta: utACK 106500c; gonna do CI on my repo, and push an empty commit and ensure caches are working as expected Tree-SHA512: b51608d8791a0d87b6d43388d4173e31123fd62fc3ae346907102504a454133e7b862bf3be784a3610ebd2b816b2aaab8c1d4a2f33a4400cc6cf7bce380e9adf
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see bitcoin#30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
Now that we use the native compiler, and have fixed Qt, and these vars
are (almost) unset in Guix, we can remove the unsetting from our compiler
command here.
I couldn't manage to make a darwin-clang-cross only exclusion of
-lmwork properlyfor Qt, so opted for just removing the explicit link entirely. I do not think this should have
any other unwanted side-effects.
Fixes #21552.