From 8aaea5c54bc3a0ad654af19f20558ef162df8971 Mon Sep 17 00:00:00 2001 From: Yee Cheng Chin Date: Mon, 27 Feb 2023 13:31:29 -0800 Subject: [PATCH] Remove Sparkle.framework when configured with --disable-sparkle Previously, when configured with `--disable-sparkle`, the Sparkle.framework would still get linked and copied into the app bundle. This is because Xcode doesn't have an easy way to disable individual build phase. We would either have to make a new build target, or dynamically patch the project file in order for that to not happen. Package managers like Homebrew and Nix have to either manually delete the framework as a post-build step, or manually patch the pbxproj file as a pre-build step to make sure the framework never got linked/copied. Also, when building MacVim on macOS 10.9-10.12, where Sparkle 2 is not supported, even if you build with `--disable-sparkle`, the app still wouldn't run since Sparkle 2's framework still got copied into the app and on launch the OS will detect that it's not compatible with the OS version. To fix this, just add a new env var and have the build cleanup script delete the entire Sparkle.framework as a post-build step, if `--disable-sparkle` was previously configured. One thing to note is that currently, even with this, the MacVim binary still has a weak linking dependency to the non-existent Sparkle.framework (can be seen by doing `otool -L MacVim.app/Contents/MacOS/MacVim`) but it should be fine. A proper way to fix this is to manually link Sparkle instead of letting Xcode do it for us, but considering that this is a relatively niche use case, and it still works regardless, there isn't a pressing need to do so. Also, add CI check to make sure when `--disable-sparkle` is used, the actual MacVim binary has 0 references to Sparkle symbols. This makes sure that removing the Sparkle.framework from the app bundle is actually safe. --- .github/workflows/ci-macvim.yaml | 26 ++++++++++++---- src/MacVim/MacVim.xcodeproj/project.pbxproj | 2 +- src/MacVim/scripts/cleanup-after-build | 33 ++++++++++++++------- src/auto/configure | 2 +- src/configure.ac | 2 +- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci-macvim.yaml b/.github/workflows/ci-macvim.yaml index 0357422a0b..0b012ee72e 100644 --- a/.github/workflows/ci-macvim.yaml +++ b/.github/workflows/ci-macvim.yaml @@ -242,6 +242,26 @@ jobs: ${VIM_BIN} -u NONE -i NONE --not-a-term -esNX -V1 -S ci/if_ver-2.vim -c quit - name: Smoketest + run: | + set -o verbose + + # Make sure there isn't any dynamic linkage to third-party dependencies in the built binary, as we should only use + # static linkage to avoid dependency hell. Test that all those dylib's are in /usr/lib which is bundled with macOS and not third-party. + if otool -L ${VIM_BIN} | grep '\.dylib\s' | grep -v '^\s*/usr/lib/'; then + echo 'Found external dynamic linkage!'; false + fi + + # Make sure that --disable-sparkle flag will properly exclude all references to Sparkle symbols. This is + # necessary because we still use weak linking to Sparkle when that flag is set and so references to Sparkle + # wouldn't fail the build (we just remove Sparkle.framework from the built app after the fact). + if ${{ matrix.publish == false }}; then + # Currently we pass --disable-sparkle flag when publish==false + if objdump -t ${MACVIM_BIN} | grep "_SPU\|_SUUpdate"; then + echo 'Found references to Sparkle even when using --disable-sparkle'; false + fi + fi + + - name: Smoketest (publish) if: matrix.publish run: | set -o verbose @@ -263,12 +283,6 @@ jobs: # Check that libsodium is working macvim_excmd -c 'set cryptmethod=xchacha20' - # Make sure there isn't any dynamic linkage to third-party dependencies in the built binary, as we should only use - # static linkage to avoid dependency hell. Test that all those dylib's are in /usr/lib which is bundled with macOS and not third-party. - if otool -L ${VIM_BIN} | grep '\.dylib\s' | grep -v '^\s*/usr/lib/'; then - echo 'Found external dynamic linkage!'; false - fi - # Make sure we are building universal x86_64 / arm64 builds and didn't accidentally create a thin app. check_arch() { local archs=($(lipo -archs "$1")) diff --git a/src/MacVim/MacVim.xcodeproj/project.pbxproj b/src/MacVim/MacVim.xcodeproj/project.pbxproj index 732c89f3a9..e35f545069 100644 --- a/src/MacVim/MacVim.xcodeproj/project.pbxproj +++ b/src/MacVim/MacVim.xcodeproj/project.pbxproj @@ -1053,7 +1053,7 @@ ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; - shellScript = "./scripts/cleanup-after-build \"$BUILT_PRODUCTS_DIR/$WRAPPER_NAME\"\n"; + shellScript = "./scripts/cleanup-after-build \"$BUILT_PRODUCTS_DIR/$WRAPPER_NAME\" \"$REMOVE_SPARKLE\"\n"; showEnvVarsInLog = 0; }; 90BD4EF224E0E8B700BF29F2 /* Copy locale message translation files */ = { diff --git a/src/MacVim/scripts/cleanup-after-build b/src/MacVim/scripts/cleanup-after-build index 24009263f1..500925d17f 100755 --- a/src/MacVim/scripts/cleanup-after-build +++ b/src/MacVim/scripts/cleanup-after-build @@ -3,22 +3,35 @@ # Utility script to clean up after a MacVim build. if [[ $# == 0 ]]; then - echo "Usage: cleanup-after-build " + echo "Usage: cleanup-after-build " exit -1 fi set -e macvim_path=$1 +remove_sparkle=$2 -sparkle_xpcservices_symlink="$macvim_path/Contents/Frameworks/Sparkle.framework/XPCServices" -sparkle_xpcservices="$macvim_path/Contents/Frameworks/Sparkle.framework/Versions/Current/XPCServices" +if [ "$remove_sparkle" == "1" ]; then + sparkle_path="$macvim_path/Contents/Frameworks/Sparkle.framework" + if [ -d "$sparkle_path" ]; then + # Remove the entire Sparkle folder. Used when --disable-sparkle was set. + # Using a clean up script is easier because there isn't an easy way to tell + # Xcode not to link/copy it unless we make another target, or dynamically + # patch the project file. + set -x + rm -rf "$sparkle_path" + fi +else + sparkle_xpcservices_symlink="$macvim_path/Contents/Frameworks/Sparkle.framework/XPCServices" + sparkle_xpcservices="$macvim_path/Contents/Frameworks/Sparkle.framework/Versions/Current/XPCServices" -if [ -d "$sparkle_xpcservices" ]; then - # This only happens when building using Sparkle 2. It contains XPC Services - # files which are only necessary for sandboxed apps, and not recommended - # otherwise. See https://sparkle-project.org/documentation/sandboxing/. - set -x - rm -rf "$sparkle_xpcservices" - rm "$sparkle_xpcservices_symlink" + if [ -d "$sparkle_xpcservices" ]; then + # This only happens when building using Sparkle 2. It contains XPC Services + # files which are only necessary for sandboxed apps, and not recommended + # otherwise. See https://sparkle-project.org/documentation/sandboxing/. + set -x + rm -rf "$sparkle_xpcservices" + rm "$sparkle_xpcservices_symlink" + fi fi diff --git a/src/auto/configure b/src/auto/configure index 606934b611..bbf7d1c701 100755 --- a/src/auto/configure +++ b/src/auto/configure @@ -5015,7 +5015,7 @@ printf "%s\n" "no" >&6; } else { printf "%s\n" "$as_me:${as_lineno-$LINENO}: result: yes" >&5 printf "%s\n" "yes" >&6; } - XCODEFLAGS="$XCODEFLAGS GCC_PREPROCESSOR_DEFINITIONS='$GCC_PREPROCESSOR_DEFINITIONS DISABLE_SPARKLE=1'" + XCODEFLAGS="$XCODEFLAGS GCC_PREPROCESSOR_DEFINITIONS='$GCC_PREPROCESSOR_DEFINITIONS DISABLE_SPARKLE=1' REMOVE_SPARKLE=1" fi if test "$enable_sparkle" == "yes"; then diff --git a/src/configure.ac b/src/configure.ac index 8eb5d6dc6b..1d7cbe615e 100644 --- a/src/configure.ac +++ b/src/configure.ac @@ -236,7 +236,7 @@ if test "$vim_cv_uname_output" = Darwin; then AC_MSG_RESULT(no) else AC_MSG_RESULT(yes) - XCODEFLAGS="$XCODEFLAGS GCC_PREPROCESSOR_DEFINITIONS='$GCC_PREPROCESSOR_DEFINITIONS DISABLE_SPARKLE=1'" + XCODEFLAGS="$XCODEFLAGS GCC_PREPROCESSOR_DEFINITIONS='$GCC_PREPROCESSOR_DEFINITIONS DISABLE_SPARKLE=1' REMOVE_SPARKLE=1" fi if test "$enable_sparkle" == "yes"; then