Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmake: Add macOS deploy target and enable it in Guix #129

Merged
merged 4 commits into from
Mar 31, 2024
Merged

cmake: Add macOS deploy target and enable it in Guix #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bce7bea
fixup! build: Generate `share/toolchain.cmake` in depends
hebasto Mar 27, 2024
6ac45b7
refactor! cmake: Redefine/adjust per-configuration flags
hebasto Mar 28, 2024
fb23c32
fixup! cmake: Add `Maintenance` module
hebasto Mar 27, 2024
984c548
fixup! cmake: Migrate Guix build scripts to CMake
hebasto Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,9 @@ if(CMAKE_CROSSCOMPILING)
if(DEFINED ENV{CXXFLAGS})
deduplicate_flags(CMAKE_CXX_FLAGS)
endif()
if(DEFINED ENV{LDFLAGS})
deduplicate_flags(CMAKE_EXE_LINKER_FLAGS)
endif()
endif()

add_subdirectory(src)
Expand All @@ -507,6 +510,7 @@ include(Maintenance)
setup_split_debug_script()
add_maintenance_targets()
add_windows_deploy_target()
add_macos_deploy_target()


include(GetTargetInterface)
Expand Down
61 changes: 61 additions & 0 deletions cmake/module/Maintenance.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,64 @@ function(add_windows_deploy_target)
add_custom_target(deploy DEPENDS ${CMAKE_BINARY_DIR}/bitcoin-win64-setup.exe)
endif()
endfunction()

function(add_macos_deploy_target)
if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND TARGET bitcoin-qt)
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@\"; }"
)
Comment on lines +94 to +103
Copy link

@m3dwards m3dwards Mar 29, 2024 •
edited
Loading

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?

Copy link
Owner Author

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?

Copy link

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.


add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/${macos_app}/Contents/MacOS/Bitcoin-Qt
COMMAND ${CMAKE_COMMAND} --install ${CMAKE_BINARY_DIR} --config $<CONFIG> --component GUI --prefix ${macos_app}/Contents/MacOS --strip
COMMAND ${CMAKE_COMMAND} -E rename ${macos_app}/Contents/MacOS/bin/$<TARGET_FILE_NAME:bitcoin-qt> ${macos_app}/Contents/MacOS/Bitcoin-Qt
COMMAND ${CMAKE_COMMAND} -E rm -rf ${macos_app}/Contents/MacOS/bin
VERBATIM
)

string(REPLACE " " "-" osx_volname ${PACKAGE_NAME})
if(CMAKE_CROSSCOMPILING)
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/dist/${macos_app}/Contents/MacOS/Bitcoin-Qt
COMMAND INSTALL_NAME_TOOL=${CMAKE_INSTALL_NAME_TOOL} OTOOL=${OTOOL} STRIP=${CMAKE_STRIP} ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/macdeploy/macdeployqtplus ${macos_app} ${osx_volname} -translations-dir=${QT_TRANSLATIONS_DIR}
DEPENDS ${CMAKE_BINARY_DIR}/${macos_app}/Contents/MacOS/Bitcoin-Qt
VERBATIM
)
add_custom_target(deploydir
DEPENDS ${CMAKE_BINARY_DIR}/dist/${macos_app}/Contents/MacOS/Bitcoin-Qt
)

find_program(ZIP_COMMAND zip REQUIRED)
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/dist/${osx_volname}.zip
WORKING_DIRECTORY dist
COMMAND ${PROJECT_SOURCE_DIR}/cmake/script/macos_zip.sh ${ZIP_COMMAND} ${osx_volname}.zip
VERBATIM
)
add_custom_target(deploy
DEPENDS ${CMAKE_BINARY_DIR}/dist/${osx_volname}.zip
)
add_dependencies(deploy deploydir)
else()
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/${osx_volname}.zip
COMMAND ${PYTHON_COMMAND} ${CMAKE_SOURCE_DIR}/contrib/macdeploy/macdeployqtplus ${macos_app} ${osx_volname} -translations-dir=${QT_TRANSLATIONS_DIR} -zip
DEPENDS ${macos_app}/Contents/MacOS/Bitcoin-Qt
VERBATIM
)
add_custom_target(deploydir
DEPENDS ${CMAKE_BINARY_DIR}/${osx_volname}.zip
)
add_custom_target(deploy
DEPENDS ${CMAKE_BINARY_DIR}/${osx_volname}.zip
)
endif()
endif()
endfunction()
10 changes: 10 additions & 0 deletions cmake/script/macos_zip.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/bin/sh
# Copyright (c) 2024-present The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or https://opensource.org/license/mit/.

if [ -n "$SOURCE_DATE_EPOCH" ]; then
find . -exec touch -d "@$SOURCE_DATE_EPOCH" {} +
fi

find . | sort | "$1" -X@ "$2"
7 changes: 2 additions & 5 deletions contrib/guix/guix-build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,9 @@ mkdir -p "$VERSION_BASE"

# Default to building for all supported HOSTs (overridable by environment)
# powerpc64le-linux-gnu currently disabled due non-determinism issues across build arches.
# TODO: Re-enable macOS hosts.
# export HOSTS="${HOSTS:-x86_64-linux-gnu arm-linux-gnueabihf aarch64-linux-gnu riscv64-linux-gnu powerpc64-linux-gnu
# x86_64-w64-mingw32
# x86_64-apple-darwin arm64-apple-darwin}"
export HOSTS="${HOSTS:-x86_64-linux-gnu arm-linux-gnueabihf aarch64-linux-gnu riscv64-linux-gnu powerpc64-linux-gnu
x86_64-w64-mingw32}"
x86_64-w64-mingw32
x86_64-apple-darwin arm64-apple-darwin}"

# Usage: distsrc_for_host HOST
#
Expand Down
12 changes: 5 additions & 7 deletions contrib/guix/libexec/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ mkdir -p "$DISTSRC"
# Install built Bitcoin Core to $INSTALLPATH
case "$HOST" in
*darwin*)
make install-strip DESTDIR="${INSTALLPATH}" ${V:+V=1}
cmake --install build --strip --prefix "${INSTALLPATH}" ${V:+--verbose}
;;
*)
cmake --install build --prefix "${INSTALLPATH}" ${V:+--verbose}
Expand All @@ -293,13 +293,12 @@ mkdir -p "$DISTSRC"

case "$HOST" in
*darwin*)
make osx_volname ${V:+V=1}
make deploydir ${V:+V=1}
cmake --build build --target deploy ${V:+--verbose}
mv build/dist/Bitcoin-Core.zip "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"
mkdir -p "unsigned-app-${HOST}"
cp --target-directory="unsigned-app-${HOST}" \
osx_volname \
contrib/macdeploy/detached-sig-create.sh
mv --target-directory="unsigned-app-${HOST}" dist
mv --target-directory="unsigned-app-${HOST}" build/dist
(
cd "unsigned-app-${HOST}"
find . -print0 \
Expand All @@ -308,7 +307,6 @@ mkdir -p "$DISTSRC"
| gzip -9n > "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.tar.gz" \
|| ( rm -f "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.tar.gz" && exit 1 )
)
make deploy ${V:+V=1} OSX_ZIP="${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"
;;
esac
(
Expand Down Expand Up @@ -389,7 +387,7 @@ mkdir -p "$DISTSRC"

case "$HOST" in
*mingw*)
cp -rf --target-directory=. "${DISTSRC}/contrib/windeploy"
cp -rf --target-directory=. contrib/windeploy
Comment on lines -392 to +390
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was unneeded.

(
cd ./windeploy
mkdir -p unsigned
Expand Down
3 changes: 3 additions & 0 deletions depends/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))|' \
Copy link

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?

Copy link
Owner Author

@hebasto hebasto Mar 27, 2024 •
edited
Loading

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.

Copy link
Owner Author

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?

Copy link

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?

Copy link
Owner Author

@hebasto hebasto Mar 27, 2024 •
edited
Loading

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

-e 's|@LDFLAGS_RELEASE@|$(strip $(host_release_LDFLAGS))|' \
-e 's|@LDFLAGS_DEBUG@|$(strip $(host_debug_LDFLAGS))|' \
-e 's|@no_qt@|$(NO_QT)|' \
-e 's|@no_qr@|$(NO_QR)|' \
-e 's|@no_zmq@|$(NO_ZMQ)|' \
Expand Down
10 changes: 10 additions & 0 deletions depends/toolchain.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ set(DEPENDS_COMPILE_DEFINITIONS @CPPFLAGS@)
set(DEPENDS_COMPILE_DEFINITIONS_RELWITHDEBINFO @CPPFLAGS_RELEASE@)
set(DEPENDS_COMPILE_DEFINITIONS_DEBUG @CPPFLAGS_DEBUG@)

if(NOT DEFINED CMAKE_EXE_LINKER_FLAGS_INIT)
set(CMAKE_EXE_LINKER_FLAGS_INIT "@LDFLAGS@")
endif()
if(NOT DEFINED CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO_INIT)
set(CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO_INIT "@LDFLAGS_RELEASE@")
endif()
if(NOT DEFINED CMAKE_EXE_LINKER_FLAGS_DEBUG_INIT)
set(CMAKE_EXE_LINKER_FLAGS_DEBUG_INIT "@LDFLAGS_DEBUG@")
endif()

set(CMAKE_AR "@AR@")
set(CMAKE_RANLIB "@RANLIB@")
set(CMAKE_STRIP "@STRIP@")
Expand Down
Loading