Merge pull request #15 from savetheclocktower/candidate-2

Candidate for new `master`

.github/workflows/ci.yml

@@ -21,9 +21,13 @@ jobs:
         exclude:
           - os: windows-latest
             node_version: 14
+          - os: macos-latest
+            node_version: 14
         include:
           - os: windows-2019
             node_version: 14
+          - os: macos-13
+            node_version: 14
     name: Node ${{ matrix.node_version }} on ${{ matrix.os }}
 
     steps:
@@ -52,14 +56,37 @@ jobs:
         with:
           python-version: '3.10'
 
-      - name: Install Python setuptools
+      - name: Install Python setuptools (Unix-likes)
         # This is needed for Python 3.12+, since many versions of node-gyp
         # are incompatible with Python 3.12+, which no-longer ships 'distutils'
         # out of the box. 'setuptools' package provides 'distutils'.
-        run: python3 -m pip install setuptools
+        if: ${{ runner.os != 'Windows' }}
+        run: |
+          python3 -m venv CI_venv
+          source CI_venv/bin/activate
+          python3 -m pip install setuptools
 
-      - name: Install dependencies
-        run: npm install
+      - name: Install Python setuptools (Windows)
+        # This is needed for Python 3.12+, since many versions of node-gyp
+        # are incompatible with Python 3.12+, which no-longer ships 'distutils'
+        # out of the box. 'setuptools' package provides 'distutils'.
+        if: ${{ runner.os == 'Windows' }}
+        run: |
+          python3 -m venv CI_venv
+          CI_venv\Scripts\activate.bat
+          python3 -m pip install setuptools
+
+      - name: Install dependencies (Unix-likes)
+        if: ${{ runner.os != 'Windows' }}
+        run: |
+          source CI_venv/bin/activate
+          npm install
+
+      - name: Install dependencies (Windows)
+        if: ${{ runner.os == 'Windows' }}
+        run: |
+          CI_venv\Scripts\activate.bat
+          npm install
 
       - name: Lint
         run: npm run standard

.gitignore

@@ -2,7 +2,10 @@ node_modules
 build
 .DS_Store
 .clang_complete
+ext
 
 /browser.js
 emsdk-portable
 package-lock.json
+
+vendor/libiconv

.npmignore

@@ -8,6 +8,8 @@
 !src/bindings/*.h
 !src/bindings/*.cc
 
+!script/fetch-libiconv-61.sh
+
 !vendor/libcxx/*
 
 !vendor/pcre/pcre.gyp

README.md

@@ -3,6 +3,10 @@
 
 Native library at the core of Atom's text editor.
 
+## Installation notes:
+
+On macOS 13 and greater, the OS no longer offers GNU `libiconv`. We handle this by downloading it from Apple’s OSS GitHub page and building it as a pre-compilation step.
+
 ## Components:
 
 ### Patch

binding.gyp

@@ -21,12 +21,40 @@
               "src/core",
               "<!(node -e \"require('nan')\")"
             ],
+            "conditions": [
+                ['OS=="mac"', {
+                    "postbuilds": [
+                        {
+                            'postbuild_name': 'Adjust vendored libiconv install name',
+                            'action': [
+                                'install_name_tool',
+                                "-change",
+                                "libiconv.2.dylib",
+                                "@loader_path/../../ext/lib/libiconv.2.dylib",
+                                "<(PRODUCT_DIR)/superstring.node"
+                            ]
+
+                            # NOTE: This version of the post-build action
+                            # should be used if we find it necessary to avoid
+                            # changing the `dylib`’s install name in an earlier
+                            # step.
+                            #
+                            # 'action': [
+                            #     'bash',
+                            #     '<(module_root_dir)/script/adjust-install-name.sh',
+                            #     '<(PRODUCT_DIR)'
+                            # ]
+
+                        }
+                    ]
+                }]
+            ]
         },
         {
             "target_name": "superstring_core",
             "type": "static_library",
             "dependencies": [
-                "./vendor/pcre/pcre.gyp:pcre",
+                "./vendor/pcre/pcre.gyp:pcre"
             ],
             "sources": [
                 "src/core/encoding-conversion.cc",
@@ -46,8 +74,14 @@
             ],
             "conditions": [
                 ['OS=="mac"', {
+                    'dependencies': [
+                        'build_libiconv'
+                    ],
+                    'include_dirs': [
+                        '<(module_root_dir)/ext/include'
+                    ],
                     'link_settings': {
-                        'libraries': ['libiconv.dylib'],
+                        'libraries': ['<(module_root_dir)/ext/lib/libiconv.2.dylib']
                     }
                 }],
                 ['OS=="win"', {
@@ -71,6 +105,43 @@
     },
 
     "conditions": [
+        ['OS=="mac"', {
+            'targets+': [
+                {
+                    "target_name": "build_libiconv",
+                    "target_type": "none",
+                    "actions": [
+                        {
+                            "action_name": "Run script",
+                            "message": "Building GNU libiconv...",
+                            "inputs": [],
+                            "outputs": ["ext"],
+                            "action": [
+                                "bash",
+                                "script/fetch-libiconv-61.sh"
+                            ]
+                        }
+                    ]
+                }
+                # {
+                #     "target_name": "find_libiconv",
+                #     "target_type": "none",
+                #     "actions": [
+                #         {
+                #             "action_name": "Run script",
+                #             "message": "Locating GNU libiconv...",
+                #             "inputs": [],
+                #             "outputs": ["vendor/libiconv/lib/libiconv.2.dylib"],
+                #             "action": [
+                #                 "bash",
+                #                 "script/find-gnu-libiconv.sh"
+                #             ]
+                #         }
+                #     ]
+                # }
+            ]
+        }],
+
         # If --tests is passed to node-gyp configure, we'll build a standalone
         # executable that runs tests on the patch.
         ['tests != 0', {

index.js

@@ -283,6 +283,8 @@ if (process.env.SUPERSTRING_USE_BROWSER_VERSION) {
   }
 
   TextBuffer.prototype.baseTextMatchesFile = function (source, encoding = 'UTF8') {
+    encoding = normalizeEncoding(encoding)
+
     return new Promise((resolve, reject) => {
       const callback = (error, result) => {
         if (error) {

script/fetch-libiconv-61.sh

@@ -0,0 +1,105 @@
+#!/bin/bash
+
+# When compiling `superstring` on macOS, we used to be able to rely on the
+# builtin version of `libiconv`. But newer versions of macOS include FreeBSD
+# `libiconv`, rather than GNU `libiconv`; the two are not API-compatible.
+#
+# For this reason, we download a known good version of `libiconv` from
+# https://github.com/apple-oss-distributions/libiconv/tree/libiconv-61.
+#
+# We might eventually replace this approach with an explicit vendorization of
+# the specific files needed, but that would require a universal build of
+# `libiconv.2.dylib`. For now, letting the user compile their own `libiconv`
+# has the advantage of very likely matching the system's architecture.
+
+echoerr() { echo "$@\n" >&2; }
+
+create-if-missing() {
+  if [ -z "$1" ]; then
+    echoerr "Error: $1 is a file."
+    usage
+    exit 1
+  fi
+  if [ ! -d "$1" ]; then
+    mkdir "$1"
+  fi
+}
+
+usage() {
+  echoerr "superstring requires the GNU libiconv library, which macOS no longer bundles in recent versions. This package attempts to compile it from GitHub. If you're seeing this message, something has gone wrong; check the README for information and consider filing an issue."
+}
+
+# Identify the directory of this script.
+SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
+
+ROOT="$SCRIPT_DIR/.."
+SCRATCH="$ROOT/scratch"
+EXT="$ROOT/ext"
+
+cleanup() {
+  if [ -d "$SCRATCH" ]; then
+    rm -rf "$SCRATCH"
+  fi
+}
+trap cleanup SIGINT EXIT
+
+create-if-missing "$EXT"
+create-if-missing "$SCRATCH"
+
+dylib_path="$EXT/lib/libiconv.2.dylib"
+
+# If this path already exists, we'll assume libiconv has already been fetched
+# and compiled. Otherwise we'll do it now.
+if [ ! -L "$dylib_path" ]; then
+  echo "Path $dylib_path is missing; fetching and installing libiconv."
+  cd $SCRATCH
+  # TODO: Instead of downloading this each time, we can check this into source
+  # control via git subtree. That would allow someone to build this without
+  # needing internet connectivity. But we'd still need to do a `make install` —
+  # at least until we can produce a "universal" version of the `.dylib` and put
+  # _that_ in source control.
+  git clone -b libiconv-61 "https://github.com/apple-oss-distributions/libiconv.git"
+  cd libiconv/libiconv
+  ./configure --prefix="$EXT" --libdir="$EXT/lib"
+  make
+  make install
+
+  if [ ! -L "$dylib_path" ]; then
+    echoerr "Error: expected $dylib_path to be present, but it was not. Installation of libiconv failed. Cannot proceed."
+    usage
+    exit 1
+  fi
+
+  # Remove the directories we don't need.
+  rm -rf "$EXT/bin"
+  rm -rf "$EXT/share"
+
+  # Copy over the license and README from the scratch directory.
+  cp "COPYING.LIB" "$EXT"
+  cp "README" "$EXT"
+else
+  echo "Path $dylib_path is already present; skipping installation of libiconv."
+fi
+
+cd $ROOT
+
+# We expect this path to exist and be a symbolic link that points to a file.
+if [ ! -L "$dylib_path" ]; then
+  echoerr "Error: expected $dylib_path to be present, but it was not. Cannot proceed."
+  usage
+  exit 1
+fi
+
+# Set the install name of this library to something neutral and predictable to
+# make a later step easier.
+#
+# NOTE: macOS complains about this action invalidating the library's code
+# signature. This has not been observed to have any negative effects for
+# Pulsar, possibly because we sign and notarize the entire app at a later stage
+# of the build process. But if it _did_ have negative effects, we could switch
+# to a different approach and skip this step. See the `binding.gyp` file for
+# further details.
+
+install_name_tool -id "libiconv.2.dylib" "${dylib_path}"
+
+cleanup

src/bindings/text-buffer-wrapper.cc

@@ -387,11 +387,7 @@ static Local<Value> encode_ranges(const vector<Range> &ranges) {
   auto length = ranges.size() * 4;
   auto buffer = v8::ArrayBuffer::New(v8::Isolate::GetCurrent(), length * sizeof(uint32_t));
   auto result = v8::Uint32Array::New(buffer, 0, length);
-  #if (V8_MAJOR_VERSION < 8)
-    auto data = buffer->GetContents().Data();
-  #else
-    auto data = buffer->GetBackingStore()->Data();
-  #endif
+  auto data = buffer->GetContents().Data();
   memcpy(data, ranges.data(), length * sizeof(uint32_t));
   return result;
 }
@@ -615,11 +611,7 @@ void TextBufferWrapper::find_words_with_subsequence_in_range(const Nan::Function
       }
 
       auto positions_buffer = v8::ArrayBuffer::New(v8::Isolate::GetCurrent(), positions_buffer_size);
-      #if (V8_MAJOR_VERSION < 8)
-        uint32_t *positions_data = reinterpret_cast<uint32_t *>(positions_buffer->GetContents().Data());
-      #else
-        uint32_t *positions_data = reinterpret_cast<uint32_t *>(positions_buffer->GetBackingStore()->Data());
-      #endif
+      uint32_t *positions_data = reinterpret_cast<uint32_t *>(positions_buffer->GetContents().Data());
 
       uint32_t positions_array_index = 0;
       for (size_t i = 0; i < result.size() && i < max_count; i++) {
@@ -943,11 +935,7 @@ void TextBufferWrapper::load(const Nan::FunctionCallbackInfo<Value> &info) {
   if (!force && text_buffer.is_modified()) {
     Local<Value> argv[] = {Nan::Null(), Nan::Null()};
     auto callback = info[0].As<Function>();
-    #if (V8_MAJOR_VERSION > 9 || (V8_MAJOR_VERSION == 9 && V8_MINOR_VERION > 4))
-      Nan::Call(callback, callback->GetCreationContext().ToLocalChecked()->Global(), 2, argv);
-    #else
-      Nan::Call(callback, callback->CreationContext()->Global(), 2, argv);
-    #endif
+    Nan::Call(callback, callback->CreationContext()->Global(), 2, argv);
     return;
   }
 
@@ -1051,12 +1039,7 @@ void TextBufferWrapper::base_text_matches_file(const Nan::FunctionCallbackInfo<V
     bool result = std::equal(file_contents.begin(), file_contents.end(), text_buffer.base_text().begin());
     Local<Value> argv[] = {Nan::Null(), Nan::New<Boolean>(result)};
     auto callback = info[0].As<Function>();
-
-    #if (V8_MAJOR_VERSION > 9 || (V8_MAJOR_VERSION == 9 && V8_MINOR_VERION > 4))
-      Nan::Call(callback, callback->GetCreationContext().ToLocalChecked()->Global(), 2, argv);
-    #else
-      Nan::Call(callback, callback->CreationContext()->Global(), 2, argv);
-    #endif
+    Nan::Call(callback, callback->CreationContext()->Global(), 2, argv);
   }
 }