deploy: remove some tools when cross-compiling for macOS #29890

fanquake · 2024-04-16T10:59:36Z

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):

8f29bce75d7f574306a0e38d793e0e4e145b547a4b9e5a755a54976121d8ac41  guix-build-5afd3c302051/output/arm64-apple-darwin/SHA256SUMS.part
9ba01fe46be715adcbe80f39dc7dbe449f32ca9d9b660da698f933aef3e6d80b  guix-build-5afd3c302051/output/arm64-apple-darwin/bitcoin-5afd3c302051-arm64-apple-darwin-unsigned.tar.gz
37719437e951449341d0e10dcc4afe93e955d59de5312ce6351e1fa01b4927ac  guix-build-5afd3c302051/output/arm64-apple-darwin/bitcoin-5afd3c302051-arm64-apple-darwin-unsigned.zip
06a79fc871dcd4290f5f7e7e9de19a5a535203d20279f4555d1c319d07abe2d0  guix-build-5afd3c302051/output/arm64-apple-darwin/bitcoin-5afd3c302051-arm64-apple-darwin.tar.gz
98d2b8b37197dcad36a04eb2f3ff2130b859220a17b83a4186a78dcf0af4eafd  guix-build-5afd3c302051/output/dist-archive/bitcoin-5afd3c302051.tar.gz
df63ff44ef41565ff13ce6dde5485173a18d5866ebc316df86f9ebd91fda18f5  guix-build-5afd3c302051/output/x86_64-apple-darwin/SHA256SUMS.part
28362ce9e80d5e78db198efa5f89434fbe76ca91df5fde7455da4d50ceb8523a  guix-build-5afd3c302051/output/x86_64-apple-darwin/bitcoin-5afd3c302051-x86_64-apple-darwin-unsigned.tar.gz
534745b679eb9e8e408dd251a6bf0829e62e12f7a41772b8a57a044ded14208c  guix-build-5afd3c302051/output/x86_64-apple-darwin/bitcoin-5afd3c302051-x86_64-apple-darwin-unsigned.zip
f53d0c9a1bb83d548c7d274c7d39653a3989fb1b4efec49e73dd1cac7c92074c  guix-build-5afd3c302051/output/x86_64-apple-darwin/bitcoin-5afd3c302051-x86_64-apple-darwin.tar.gz

DrahtBot · 2024-04-16T10:59:39Z

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	TheCharlatan

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29739 (depends: swap some cctools tools for LLVM tools by fanquake)
#29450 (build: replace custom MAC_OSX macro with standard __APPLE__ for consistent macOS detection by paplorinc)

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 · 2024-04-16T11:09:30Z

Makefile.am

-	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)


bitcoin/contrib/macdeploy/macdeployqtplus

Line 212 in c7567d9

installnametoolbin=os.getenv("INSTALL_NAME_TOOL", "install_name_tool")

bitcoin/contrib/macdeploy/macdeployqtplus

Line 231 in c7567d9

stripbin=os.getenv("STRIP", "strip")

Did you read the description? Neither of these code paths are hit when cross compiling.

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?

DrahtBot · 2024-04-16T21:51:14Z

Guix builds (on x86_64)

File	commit `c7567d9` (master)	commit `efdc442` (master and this pull)
SHA256SUMS.part	`0d0fc9910c71801e...`	`d77ae186fd3020ba...`
*-aarch64-linux-gnu-debug.tar.gz	`f8af7013dcae9a1c...`	`dfa447d84f245661...`
*-aarch64-linux-gnu.tar.gz	`ddeb7ce74fdd2329...`	`f6aa8560aaa25ec6...`
*-arm-linux-gnueabihf-debug.tar.gz	`99f5761d802f7da6...`	`63ccceae45b60e74...`
*-arm-linux-gnueabihf.tar.gz	`0f4b3e515f238cd7...`	`cf995da64a1bf17e...`
*-arm64-apple-darwin-unsigned.tar.gz	`c5ef39c6ad8a8281...`	`9b1462f84dc9b665...`
*-arm64-apple-darwin-unsigned.zip	`5356b31cbbd15b64...`	`a9492d66a37d8952...`
*-arm64-apple-darwin.tar.gz	`5b16c5013a4843ca...`	`df7acdd9e4f7401e...`
*-powerpc64-linux-gnu-debug.tar.gz	`c81b1f327aff5535...`	`3b6bcba20c39ed3e...`
*-powerpc64-linux-gnu.tar.gz	`c9d03e5d063377d3...`	`3f15a2e8a0701953...`
*-riscv64-linux-gnu-debug.tar.gz	`9aa7cf7fe6a34a2e...`	`f5b61822ade1fbb1...`
*-riscv64-linux-gnu.tar.gz	`1483a98592c23b57...`	`48f80d76ad1bf8d2...`
*-x86_64-apple-darwin-unsigned.tar.gz	`7bdfa268f4002a33...`	`739956f0ca7262a6...`
*-x86_64-apple-darwin-unsigned.zip	`aa9e70d6d9a79650...`	`cdea52c43b62e6c3...`
*-x86_64-apple-darwin.tar.gz	`af5be2b0f4df0085...`	`304203da0939431a...`
*-x86_64-linux-gnu-debug.tar.gz	`44e3cb211881f7e4...`	`b15a7b3e279a8d93...`
*-x86_64-linux-gnu.tar.gz	`01dbf3d65f727130...`	`d246d8f51593c097...`
*.tar.gz	`f05a0240184f515d...`	`1bc3bff268e37c47...`
guix_build.log	`462fee383aac06ad...`	`590c7ef60ae96d96...`
guix_build.log.diff		`9811e906d3810302...`

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.

fanquake · 2024-04-17T10:41:06Z

Added a commit to decrease the scope of using -Wl,-headerpad_max_install_names.

Sjors

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

Sjors · 2024-04-17T13:15:43Z

Makefile.am

@@ -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)


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

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

hebasto · 2024-04-29T21:20:56Z

Apparently, CMake expects to find the install_name_tool when cross-compiling for macOS.

From https://github.com/hebasto/bitcoin/actions/runs/8885213906/job/24396062348:

 -- The C compiler identification is Clang 17.0.6
CMake Error at /usr/share/cmake-3.25/Modules/CMakeFindBinUtils.cmake:233 (message):
  Could not find install_name_tool, please check your installation.

hebasto · 2024-04-30T13:32:57Z

Ported to the CMake-based build system in hebasto#180.

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

fanquake added the DrahtBot Guix build requested label Apr 16, 2024

hebasto reviewed Apr 16, 2024

View reviewed changes

fanquake mentioned this pull request Apr 16, 2024

build: swap cctools otool for llvm-objdump #29739

Merged

DrahtBot removed the DrahtBot Guix build requested label Apr 16, 2024

fanquake added 3 commits April 17, 2024 11:40

build: don't pass strip to macOS deploy if cross-compiling

78b6b5c

This could only be called in code paths that cannot be hit.

build: don't use install_name_tool for macOS deploy when cross-compiling

3bee514

This is only needed when compiling on macOS. This means we can also better scope the usage of `-headerpad_max_install_names`.

build: better scope usage of -Wl,-headerpad_max_install_names

1a9aa8d

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.

fanquake force-pushed the llvm_tools_v2 branch from 5afd3c3 to 1a9aa8d Compare April 17, 2024 10:40

Sjors reviewed Apr 17, 2024

View reviewed changes

DrahtBot mentioned this pull request Apr 17, 2024

build: replace custom MAC_OSX macro with standard __APPLE__ for consistent macOS detection #29450

Open

bitcoin deleted a comment from Joudyadam Apr 17, 2024

TheCharlatan approved these changes Apr 20, 2024

View reviewed changes

laanwj added macOS Build system labels Apr 21, 2024

fanquake added the Needs CMake port label Apr 24, 2024

fanquake merged commit d48d55e into bitcoin:master Apr 25, 2024
16 checks passed

fanquake deleted the llvm_tools_v2 branch April 25, 2024 13:22

hebasto mentioned this pull request Apr 29, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#179

Closed

hebasto mentioned this pull request Apr 30, 2024

cmake: Port PR29890 from the master branch hebasto/bitcoin#180

Merged

hebasto removed the Needs CMake port label Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deploy: remove some tools when cross-compiling for macOS #29890

deploy: remove some tools when cross-compiling for macOS #29890

fanquake commented Apr 16, 2024

DrahtBot commented Apr 16, 2024 •

edited

hebasto Apr 16, 2024

hebasto Apr 16, 2024

fanquake Apr 16, 2024

hebasto Apr 16, 2024 •

edited

DrahtBot commented Apr 16, 2024

fanquake commented Apr 17, 2024

Sjors left a comment

Sjors Apr 17, 2024

TheCharlatan left a comment

hebasto commented Apr 29, 2024

hebasto commented Apr 30, 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)

deploy: remove some tools when cross-compiling for macOS #29890

deploy: remove some tools when cross-compiling for macOS #29890

Conversation

fanquake commented Apr 16, 2024

DrahtBot commented Apr 16, 2024 • edited

Code Coverage

Reviews

Conflicts

hebasto Apr 16, 2024

Choose a reason for hiding this comment

hebasto Apr 16, 2024

Choose a reason for hiding this comment

fanquake Apr 16, 2024

Choose a reason for hiding this comment

hebasto Apr 16, 2024 • edited

Choose a reason for hiding this comment

DrahtBot commented Apr 16, 2024

Guix builds (on x86_64)

fanquake commented Apr 17, 2024

Sjors left a comment

Choose a reason for hiding this comment

Sjors Apr 17, 2024

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

hebasto commented Apr 29, 2024

hebasto commented Apr 30, 2024

DrahtBot commented Apr 16, 2024 •

edited

hebasto Apr 16, 2024 •

edited