Merge pull request #1626 from ychin/ci-use-homebrew-libiconv

Build MacVim binary release with GNU iconv instead of Apple iconv

Author: ychin

.github/actions/universal-package/action.yml

@@ -7,15 +7,19 @@ description: Create universal Homebrew package which contains x86_64 and arm64
 # that has both x86_64 and arm64 arch, as Homebrew's distributed bottles are thin binaries with only one arch.
 #
 # We still use Homebrew to manage the library because their formulas are up to date and have correct build instructions
-# that will work. This way we don't have to manually configuring and building and updating the package info.
+# that will work. This way we don't have to manually configure, build, and update the package info.
 
 inputs:
   formula:
-    description: Formura name
+    description: Formula name
     required: true
   contents:
     description: Path for contents in package's keg
     required: true
+  gnuiconv:
+    description: Use the Homebrew GNU libiconv instead of system one
+    type: boolean
+    required: false
 runs:
   using: 'composite'
   steps:
@@ -31,14 +35,24 @@ runs:
         # version and stomp what we have here.
         brew update
 
+        brew cat ${formula} >${formula}.rb
+
+        if [[ "${{ inputs.gnuiconv }}" == "true" ]]; then
+          # Modify formula to build using Homebrew libiconv. Usually just adding "depends_on" is enough, but since we
+          # override "CC" to use vanilla system clang, we need to manually inject the compilation/link flags to specify
+          # the locations.
+          sed -i.bak '/^[[:blank:]]*def install$/i\'$'\n  depends_on "libiconv"\n' ${formula}.rb
+          sed -i.bak '/^[[:blank:]]*def install$/a\'$'\n    ENV["CFLAGS"] += " -I'$(brew --prefix)$'/opt/libiconv/include"\n' ${formula}.rb
+          sed -i.bak '/^[[:blank:]]*def install$/a\'$'\n    ENV["LDFLAGS"] += " -L'$(brew --prefix)$'/opt/libiconv/lib"\n' ${formula}.rb
+        fi
+
         # Patch the official Homebrew formula to explicitly build for min deployment target and a universal binary. We
         # also need to explicitly use system Clang because Homebrew's bundled clang script tries to inject -march
         # compiler flags that will cause universal builds to fail as Clang does not like that.
-        brew cat ${formula} | \
-          sed '/^[[:blank:]]*def install$/a\'$'\n    ENV["MACOSX_DEPLOYMENT_TARGET"] = "'${MACOSX_DEPLOYMENT_TARGET}$'"\n' | \
-          sed '/^[[:blank:]]*def install$/a\'$'\n    ENV["CC"] = "/usr/bin/clang"\n' | \
-          sed '/^[[:blank:]]*def install$/a\'$'\n    ENV["CFLAGS"] = "-arch x86_64 -arch arm64"\n' | \
-          sed '/^[[:blank:]]*def install$/a\'$'\n    ENV["LDFLAGS"] = "-arch x86_64 -arch arm64"\n' >${formula}.rb
+        sed -i.bak '/^[[:blank:]]*def install$/a\'$'\n    ENV["MACOSX_DEPLOYMENT_TARGET"] = "'${MACOSX_DEPLOYMENT_TARGET}$'"\n' ${formula}.rb
+        sed -i.bak '/^[[:blank:]]*def install$/a\'$'\n    ENV["CC"] = "/usr/bin/clang"\n' ${formula}.rb
+        sed -i.bak '/^[[:blank:]]*def install$/a\'$'\n    ENV["CFLAGS"] = "-arch x86_64 -arch arm64"\n' ${formula}.rb
+        sed -i.bak '/^[[:blank:]]*def install$/a\'$'\n    ENV["LDFLAGS"] = "-arch x86_64 -arch arm64"\n' ${formula}.rb
 
         # Homebrew requires formula files to be placed in taps and disallows
         # installing from raw paths, so we manually create a taps folder for a
@@ -85,7 +99,9 @@ runs:
         brew list ${formula} &>/dev/null || brew install --quiet --formula -s macvim-dev/deps/${formula}
 
         # If formula was cached, this step is necessary to relink it to brew prefix (e.g. /usr/local)
-        brew unlink ${formula} && brew link ${formula}
+        # The "-f" is there to force link keg-only formulas. Homebrew doesn't provide a command to just link in the
+        # optional /opt/homebrew/opt/... folders, so we have to resort to doing this.
+        brew unlink ${formula} && brew link -f ${formula}
         echo '::endgroup::'
 
         echo '::group::Verify built version'

.github/workflows/macvim-buildtest.yaml

@@ -89,6 +89,15 @@ jobs:
           xcode-select -p
           xcodebuild -version
 
+      # Set up, install, and cache GNU libiconv library to work around Apple iconv issues.
+
+      - name: Set up libiconv
+        if: inputs.publish
+        uses: ./.github/actions/universal-package
+        with:
+          formula: libiconv
+          contents: opt/libiconv/lib/libiconv.a,opt/libiconv/lib/libiconv.dylib
+
       # Set up, install, and cache gettext library for localization.
 
       - name: Set up gettext
@@ -97,6 +106,7 @@ jobs:
         with:
           formula: gettext
           contents: lib/libintl.a,lib/libintl.dylib
+          gnuiconv: true  # gettext needs to match MacVim in using the same version of iconv
 
       # Set up, install, and cache libsodium library for encryption.
 
@@ -222,6 +232,9 @@ jobs:
             sed -i.bak -f ci/config.mk.optimized.sed src/auto/config.mk
           fi
 
+          # Use Homebrew GNU libiconv since Apple iconv has been broken since macOS 14
+          sed -i.bak -f ci/config.mk.brew-libiconv.sed src/auto/config.mk
+
       - name: Modify configure result
         if: inputs.publish
         run: |
@@ -271,6 +284,11 @@ jobs:
             echo 'Found external dynamic linkage!'; false
           fi
 
+          # Make sure we are not using system iconv, which has been buggy since macOS 14.
+          if otool -L ${VIM_BIN} | grep '^\s*/usr/lib/libiconv'; then
+            echo 'Using system iconv! We should be linking against GNU iconv instead.'; 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).

ci/config.mk.brew-libiconv.sed

@@ -0,0 +1,8 @@
+# Use Homebrew GNU libiconv to work around broken Apple iconv. Use static
+# linking as we don't want our binary releases to pull in third-party
+# dependencies.
+#
+# If gettext is configured in the build, it also needs to be built against GNU
+# libiconv. Otherwise we would get a link error from this.
+/^CFLAGS[[:blank:]]*=/s/$/ -I\/opt\/homebrew\/opt\/libiconv\/include/
+/^LIBS[[:blank:]]*=/s/-liconv/\/opt\/homebrew\/opt\/libiconv\/lib\/libiconv.a/

src/testdir/test_macvim.vim

@@ -35,6 +35,7 @@ func Test_macvim_options_commands_exist()
     call assert_true(has('clientserver'), 'Missing feature "clientserver"')
     call assert_true(has('clipboard'), 'Missing feature "clipboard"')
     call assert_true(has('clipboard_working'), 'Missing feature "clipboard_working"')
+    call assert_true(has('iconv'), 'Missing feature "iconv"')
     call assert_true(has('sound'), 'Missing feature "sound"')
     call assert_true(has('terminal'), 'Missing feature "terminal"')
     call assert_true(has('xim'), 'Missing feature "xim"')