deploy: remove some tools when cross-compiling for macOS #29890
Merged
Conversation
This file contains hidden or 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
|
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. 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
reviewed
Apr 16, 2024
| INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) STRIP=$(STRIP) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) | ||
| OTOOL=$(OTOOL) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) |
There was a problem hiding this comment.
bitcoin/contrib/macdeploy/macdeployqtplus
Line 212 in c7567d9
There was a problem hiding this comment.
bitcoin/contrib/macdeploy/macdeployqtplus
Line 231 in c7567d9
There was a problem hiding this comment.
Did you read the description? Neither of these code paths are hit when cross compiling.
There was a problem hiding this comment.
Did you read the description?
I did.
Neither of these code paths are hit when cross compiling.
It is not obvious from reading the macdeployqtplus code. Maybe add a few comments to make it clear?
This could only be called in code paths that cannot be hit.
This is only needed when compiling on macOS. This means we can also better scope the usage of `-headerpad_max_install_names`.
If we aren't using install_name_tool when cross-compiling, we don't need to test for / add it to LDFLAGS when that is the case.
|
Added a commit to decrease the scope of using |
Sjors
reviewed
Apr 17, 2024
There was a problem hiding this comment.
Just tested that both the guix build and local make deploy still work on Intel macOS 14.4.1.
751ede1b4f680d44c97a9aab396e0a485e3a47c88ecc30ec8b83e53784fc3f50 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/SHA256SUMS.part
871cf387d5d60efc0ec9e50f975a9f44b2e2f9b7d92d1f2744affc3c8a5e1655 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.tar.gz
6a8de4ac9647549d146a53a9167b00f72ac09168939284b76fa9b6cf81595fea guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.zip
6a00443340b2b8d13ed86a5d3947845ec2da20cfb0f0f80cce1434a0a4581557 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin.tar.gz
ec0320586c07bcc6b80505ed6b3de6307fd99d43b505919bb4f95ff27f1d8991 guix-build-1a9aa8d4eedf/output/dist-archive/bitcoin-1a9aa8d4eedf.tar.gz
14a0907bf9c91a2ca19b5a9b53071159be36d82d69c287ffd6a621695f4a637d guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/SHA256SUMS.part
862b59e90473c37d4f95d083fccf12416ddc92f9177a2ffb71180a11b3432d9b guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.tar.gz
ddcb16882dde50880f9dffc6ac7ab5ac01b220bbedad4c16e9ff15dcf3b30cef guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.zip
720a0689c916e878a5ff050b6183e993af751950b906593d4cbaf4b22a22466c guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin.tar.gz
Makefile.am
Outdated
| @@ -126,7 +126,7 @@ $(OSX_ZIP): deploydir | |||
| cd $(APP_DIST_DIR) && find . | sort | $(ZIP) -X@ $@ | |||
|
|
|||
| $(APP_DIST_DIR)/$(OSX_APP)/Contents/MacOS/Bitcoin-Qt: $(OSX_APP_BUILT) $(OSX_PACKAGING) | |||
| INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) STRIP=$(STRIP) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) | |||
| INSTALL_NAME_TOOL=$(INSTALL_NAME_TOOL) OTOOL=$(OTOOL) $(PYTHON) $(OSX_DEPLOY_SCRIPT) $(OSX_APP) $(OSX_VOLNAME) -translations-dir=$(QT_TRANSLATION_DIR) | |||
There was a problem hiding this comment.
78b6b5c: note for other reviewers
BUILD_OS is set to darwin when either (see configure.ac):
- we're not cross-compiling and
TARGET_OS=darwin; or - we are cross-compiling and
$build_os=darwin
BUILD_DARWIN is set when [test "$BUILD_OS" = "darwin"]. So the !BUILD_DARWIN code path here is taken when we are cross compiling (from a non-darwin system).
After this commit $STRIP is only used for Windows stuff.
It's not clear to me what $STRIP is/was doing. So I'll just check if this PR doesn't break anything for me.
TheCharlatan
approved these changes
Apr 20, 2024
There was a problem hiding this comment.
ACK 1a9aa8d
Guix builds (x86_64):
871cf387d5d60efc0ec9e50f975a9f44b2e2f9b7d92d1f2744affc3c8a5e1655 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.tar.gz
6a8de4ac9647549d146a53a9167b00f72ac09168939284b76fa9b6cf81595fea guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin-unsigned.zip
6a00443340b2b8d13ed86a5d3947845ec2da20cfb0f0f80cce1434a0a4581557 guix-build-1a9aa8d4eedf/output/arm64-apple-darwin/bitcoin-1a9aa8d4eedf-arm64-apple-darwin.tar.gz
ec0320586c07bcc6b80505ed6b3de6307fd99d43b505919bb4f95ff27f1d8991 guix-build-1a9aa8d4eedf/output/dist-archive/bitcoin-1a9aa8d4eedf.tar.gz
14a0907bf9c91a2ca19b5a9b53071159be36d82d69c287ffd6a621695f4a637d guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/SHA256SUMS.part
862b59e90473c37d4f95d083fccf12416ddc92f9177a2ffb71180a11b3432d9b guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.tar.gz
ddcb16882dde50880f9dffc6ac7ab5ac01b220bbedad4c16e9ff15dcf3b30cef guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin-unsigned.zip
720a0689c916e878a5ff050b6183e993af751950b906593d4cbaf4b22a22466c guix-build-1a9aa8d4eedf/output/x86_64-apple-darwin/bitcoin-1a9aa8d4eedf-x86_64-apple-darwin.tar.gz
|
Apparently, CMake expects to find the From https://github.com/hebasto/bitcoin/actions/runs/8885213906/job/24396062348: |
|
Ported to the CMake-based build system in hebasto#180. |
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 30, 2024
772769a fixup! cmake: Add `Maintenance` module (Hennadii Stepanov) fa36e33 fixup! cmake: Add platform-specific linker flags (Hennadii Stepanov) c6dc19f fixup! build: Generate `toolchain.cmake` in depends (Hennadii Stepanov) cddeb04 fixup! cmake: Add root `CMakeLists.txt` file (Hennadii Stepanov) Pull request description: This PR ports bitcoin#29890 and fixes cross-compiling for macOS been broken since the recent sync/rebase [PR](#179). ACKs for top commit: m3dwards: Giving a utACK 772769a Tree-SHA512: f0f10317b1fd5e46d1b7f340f4efb9f1b27a6a7a9b11191736c8edf32c278ba6d9ca4ef38d03a78cb40d58637e7e9746020b264a3f7eeb9903c23cb726f18fbf
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 7, 2024
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 7, 2024
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 15, 2024
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 16, 2024
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Nov 17, 2024
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Nov 17, 2024
, bitcoin#28622, bitcoin#28880, bitcoin#29185, bitcoin#29170, bitcoin#29233, bitcoin#29298, bitcoin#29598, bitcoin#29732, bitcoin#29890, bitcoin#29739, bitcoin#30074, bitcoin#30198, bitcoin#29072 (toolchain backports: part 2) 1506d9d merge bitcoin#29072: use `-no_exported_symbols` on macOS (Kittywhiskers Van Gogh) 9247960 merge bitcoin#30198: qt 5.15.14 and fix macOS build with Clang 18 (Kittywhiskers Van Gogh) 5585e7a merge bitcoin#30074: use ENV flags in get_arch (Kittywhiskers Van Gogh) decd420 merge bitcoin#29739: swap cctools otool for llvm-objdump (Kittywhiskers Van Gogh) 0f8c420 merge bitcoin#29890: remove some tools when cross-compiling for macOS (Kittywhiskers Van Gogh) 936da1a merge bitcoin#29732: qt 5.15.13 (Kittywhiskers Van Gogh) c294b47 revert: patch qt to make placeholders differ from actual text (Kittywhiskers Van Gogh) af7090c merge bitcoin#29598: don't use -h with touch on OpenBSD (Kittywhiskers Van Gogh) ebf8ff2 merge bitcoin#29298: patch libtool out of libnatpmp/miniupnpc (Kittywhiskers Van Gogh) 070b876 merge bitcoin#29233: depends move macOS C(XX) FLAGS out of C & CXX (Kittywhiskers Van Gogh) d838481 revert dash#2398: Force fvisibility=hidden when compiling on macos (Kittywhiskers Van Gogh) 59a18f9 merge bitcoin#29170: add macho branch protection check (Kittywhiskers Van Gogh) cb024d9 merge bitcoin#29185: remove `--enable-lto` (Kittywhiskers Van Gogh) 6d75a81 merge bitcoin#28880: switch to using LLVM 17.x for macOS builds (Kittywhiskers Van Gogh) 7b0a1f2 merge bitcoin#28622: use macOS 14 SDK (Xcode 15.0) (Kittywhiskers Van Gogh) 02eb735 merge bitcoin#24948: fix typo in permissions (Kittywhiskers Van Gogh) 2739107 merge bitcoin#24534: make gen-sdk deterministic (Kittywhiskers Van Gogh) ab10bf9 merge bitcoin#24241: cleanup doc on need of Developer Account to obtain macOS SDK (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6384 * Dependency for #6389 * The Qt patch introduced in [dash#5596](#5596), `fix_qt_placeholders.patch`, was a portion of a suggested workaround for QTBUG-92199 ([source](https://bugreports.qt.io/browse/QTBUG-92199?focusedId=669719&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-669719)) but since then, a fix ([here](https://codereview.qt-project.org/c/qt/qtbase/+/434310)) has made its way to 5.15.12 and we are upgrading to 5.15.14 from 5.15.11. So we can safely remove this patch. ## 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 - [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 1506d9d PastaPastaPasta: utACK 1506d9d Tree-SHA512: df8e4ea0ce9e7b269d248518698f0566b5eca1a54cdfb53f5b213b90fb5177e5a5df44eaeb6f3fc014cd93351c9245736bb2fd52bc2af4ae274d8fa93e601b07
Fabcien
added a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 30, 2024
Summary: ``` Neither of these tools are actually used when we are cross-compiling for macOS. They are used when we have to adjust non-static libs during a deploy after building on a macOS machine. ``` Backport of [[bitcoin/bitcoin#29890 | core#29890]]. Test Plan: Run the GUIX osx build. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D17258
roqqit
pushed a commit
to doged-io/doged
that referenced
this pull request
Dec 19, 2024
Summary: ``` Neither of these tools are actually used when we are cross-compiling for macOS. They are used when we have to adjust non-static libs during a deploy after building on a macOS machine. ``` Backport of [[bitcoin/bitcoin#29890 | core#29890]]. Test Plan: Run the GUIX osx build. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D17258
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.
Neither of these tools are actually used when we are cross-compiling for macOS. They are used when we have to adjust non-static libs during a deploy after building on a macOS machine. Simplies #29739 (will be rebased on top).
Guix (aarch64):