-
Notifications
You must be signed in to change notification settings - Fork 5
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
cmake: Add macOS deploy
target and enable it in Guix
#129
Conversation
cp -rf --target-directory=. "${DISTSRC}/contrib/windeploy" | ||
cp -rf --target-directory=. contrib/windeploy |
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.
This change was unneeded.
My Guix builds:
|
cmake/script/macos_zip.sh
Outdated
# file COPYING or https://opensource.org/license/mit/. | ||
|
||
if [ -n "$SOURCE_DATE_EPOCH" ]; then | ||
find . -exec touch --date="@$SOURCE_DATE_EPOCH" {} + |
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.
FreeBSD's touch
does not have --date
. However both Linux and FreeBSD have -d YYYY-MM-DDThh:mm:SS[.frac][tz]
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.
Switched from --date
to -d
in the recent push.
cmake/module/Maintenance.cmake
Outdated
add_custom_command( | ||
OUTPUT dist/${osx_volname}.zip | ||
WORKING_DIRECTORY dist | ||
COMMAND ${PROJECT_SOURCE_DIR}/cmake/script/macos_zip.sh ${ZIP_COMMAND} ${osx_volname}.zip | ||
VERBATIM | ||
) |
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.
macos_zip.sh
has just 2 find
commands. It would be easier to follow if
add_custom_command(macos_zip.sh)
is replaced by
add_custom_command(find touch ...)
add_custom_command(find ZIP_COMMAND ...)
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.
This part might be much improved / simplified after bitcoin#29761.
Process linker flags properly.
99935f1
to
786c9f1
Compare
Fixed a couple of bugs. Thanks to @m3dwards for pointing at one of them offline. Undrafted. |
@@ -282,6 +282,9 @@ $(host_prefix)/share/toolchain.cmake : toolchain.cmake.in $(host_prefix)/.stamp_ | |||
-e 's|@CPPFLAGS@|$(strip $(host_CPPFLAGS))|' \ | |||
-e 's|@CPPFLAGS_RELEASE@|$(strip $(host_release_CPPFLAGS))|' \ | |||
-e 's|@CPPFLAGS_DEBUG@|$(strip $(host_debug_CPPFLAGS))|' \ | |||
-e 's|@LDFLAGS@|$(strip $(host_LDFLAGS))|' \ |
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.
In bce7bea: What ended up pointing out this error? I'm suprised that all builds (regardless of Guix) weren't broken if this was missing? Or I guess up until this point, nobody has tested setting LDFLAGS in a CMake build with depends?
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.
Another weird observation.
#129 (comment) for 99935f1 was produced on Ubuntu 23.10. But it must fail to pass check_MACHO_sdk
in the symbol-check.py
.
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.
I'm suprised that all builds (regardless of Guix) weren't broken...
Isn't darwin
the only host that sets linker flags in depends?
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.
Isn't darwin the only host that sets linker flags in depends?
Anyone building depends for any HOST
with make -C depends LDFLAGS="some_flag"
would have broken up until this point?
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.
Everyone has such an opportunity now :)
My updated Guix builds:
|
Process linker flags properly.
Add `deploy` target for macOS
786c9f1
to
984c548
Compare
Addressed @TheCharlatan's offline comment regarding |
My updated Guix builds:
|
Guix builds (x86):
Tested the binaries on macos 11.1, they worked as expected. |
set(macos_app "Bitcoin-Qt.app") | ||
# Populate Contents subdirectory. | ||
configure_file(${PROJECT_SOURCE_DIR}/share/qt/Info.plist.in ${macos_app}/Contents/Info.plist) | ||
file(CONFIGURE OUTPUT ${macos_app}/Contents/PkgInfo CONTENT "APPL????") | ||
# Populate Contents/Resources subdirectory. | ||
file(CONFIGURE OUTPUT ${macos_app}/Contents/Resources/empty.lproj CONTENT "") | ||
configure_file(${PROJECT_SOURCE_DIR}/src/qt/res/icons/bitcoin.icns ${macos_app}/Contents/Resources/bitcoin.icns COPYONLY) | ||
file(CONFIGURE OUTPUT ${macos_app}/Contents/Resources/Base.lproj/InfoPlist.strings | ||
CONTENT "{ CFBundleDisplayName = \"@PACKAGE_NAME@\"; CFBundleName = \"@PACKAGE_NAME@\"; }" | ||
) |
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.
This appears to run as part of the configure step and so we have an incomplete app bundle called Bitcoin-Qt.app created in the build directory.
Should these steps be part of their own custom command and only called as part of the deploydir and deploy targets? And perhaps the file cleaned up after the zip is created?
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.
Should these steps be part of their own custom command and only called as part of the deploydir and deploy targets?
I've considered this option while working on this PR and chosen this implementation for the following reasons:
- the content of all files above is known at the configuration stage
- it is simpler / more readable
I'd be happy to consider benefits of the alternative approach, if any.
And perhaps the file cleaned up after the zip is created?
Why?
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.
Why?
As a noob and after running the build I see this bundle in the build root and think it's the app bundle and not just a template used to produce a real one in dist
.
It's only a NIT and just something unobvious to me so I had to investigate what it was.
I'm getting an error when I run natively on macos:
|
Seems like this is only an issue if something was leftover from a previous autotools build. Cleaned my repository and it worked fine again. |
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 984c548
Also tested natively on macos 12.7
Post merge ACK Tested native MacOS builds (m1) and cross compiled from linux x86 for Mac arm. Inspected the app bundles and they were identical and ran the same as compared with native and cross compiled make builds on master. Guix Builds: 46dd97cf462d4debd29d895de1586e6f1874688c331fa48b099bf09271770a5b bitcoin-984c548086d2-aarch64-linux-gnu-debug.tar.gz
65d9f061a41adb39e8115be2faab7a616d2ee44266ecdf8965837a0ff96e6ede bitcoin-984c548086d2-aarch64-linux-gnu.tar.gz
74de989391f1c38eb710c0fc2b12a84e8fd621e7526acd423c1405a33fab3f7d bitcoin-984c548086d2-arm-linux-gnueabihf-debug.tar.gz
6021423ae5bda75438e1bb692670baa8f368c9ae680ea0b5f2ef12dd0e0c7c38 bitcoin-984c548086d2-arm-linux-gnueabihf.tar.gz
15569069d5e6019dfa43b82403322673bdb97abc85a342aa83a7f843a2f1de6a bitcoin-984c548086d2-arm64-apple-darwin-unsigned.tar.gz
9583a1731629ea1504d37320ff3fdae39a819d63c2ab9670208777c32444eede bitcoin-984c548086d2-arm64-apple-darwin-unsigned.zip
d8e0b331f18908492ec3c8c45308deafa786f31f76991bd0452b3402ed7cda98 bitcoin-984c548086d2-arm64-apple-darwin.tar.gz
d095cd5d6a17e5253deb5a3e0f2a29368879eb593faf46cc522c348275ab3336 bitcoin-984c548086d2.tar.gz
718d5d1e9bce5c3db44b1ea2ed9dfe2856845c22872461b32efdf0ab61acd4c4 bitcoin-984c548086d2-powerpc64-linux-gnu-debug.tar.gz
e3b7c03a866088f2af7d986be3f079b724991eead68e3f62c7fed3bf1849a896 bitcoin-984c548086d2-powerpc64-linux-gnu.tar.gz
bd7197a6b1d3e4008fc5fa1542cc802a89304861eb09786768355ac15346c564 bitcoin-984c548086d2-riscv64-linux-gnu-debug.tar.gz
0710c40838e824dfedee4fbf9245b3d9fa15f2bdca2cf9dd08416357d019fffb bitcoin-984c548086d2-riscv64-linux-gnu.tar.gz
1823cdb3c67ab3d2ef5379bf6e0df55734dc7dd0a68d3192bafa7a9cec778174 bitcoin-984c548086d2-x86_64-apple-darwin-unsigned.tar.gz
1a29814243d019f96e02c8c19fab063f4237f18272d9e67fdf4c4abb0d407525 bitcoin-984c548086d2-x86_64-apple-darwin-unsigned.zip
2eab8a1771ea61a818358fb736e78dbbb9a1a7e75d44e9c3d58bb2081cd3351d bitcoin-984c548086d2-x86_64-apple-darwin.tar.gz
9bb78afd34517972fb9c258ed291704ae129508cde0cffd38f62a7b1dae25ee1 bitcoin-984c548086d2-x86_64-linux-gnu-debug.tar.gz
a5f5fd0f3ced0d387b9c950b6fa40c21dea5da9596a53a98f98d9cce2a19416e bitcoin-984c548086d2-x86_64-linux-gnu.tar.gz
1e6f043b8fbfc1f3022ee3cac05328d1dbdaabd023bb54bff2cb7218606e423f bitcoin-984c548086d2-win64-debug.zip
e623eb4427926b43a32842b1f34f5a2ed9803794f44bc51fa02ae7d760655143 bitcoin-984c548086d2-win64-setup-unsigned.exe
90e570cfa6498474d8f14a238470701d7b405348aa29c6d9ebb3aa07d2f20de8 bitcoin-984c548086d2-win64-unsigned.tar.gz
cb8abbe41247e028a97e9beb9715f1d7bbc845e754d6ba0b989f0543380edf7f bitcoin-984c548086d2-win64.zip |
What to test:
cmake --build build && cmake --build build -t deploydir
and the followingcmake --build build -t deploy
when cross-compiling for macOS and on macOS natively.This PR includes changes from bitcoin#29733.
UPD. Also related: bitcoin#29761.