Skip to content

MacOS compatibility for gotta-patch-em-all!.sh - #1976

Merged
Finii merged 4 commits into
ryanoasis:masterfrom
shaunchokshi:add-cachyos-icons
Jan 15, 2026
Merged

Finii merged 4 commits into
ryanoasis:masterfrom
shaunchokshi:add-cachyos-icons

Conversation

@shaunchokshi

Copy link
Copy Markdown
Contributor

Description

This PR improves the gotta-patch-em-all-font-patcher!.sh script with enhanced font filtering logic and cross-platform compatibility fixes for macOS/BSD systems.

Key changes:
  1. Refactored filename filtering logic - Replaced -iregex pattern matching with -iname conditions grouped with parentheses, supporting both directory-based filtering (path starts with "/") and filename-based filtering with proper wildcard matching for .ttf, .otf, and .sfd files
  2. Added macOS/BSD compatibility for date commands - The script now detects whether GNU or BSD date is available and uses the appropriate syntax:
    GNU: date -R "--date=@${SOURCE_DATE_EPOCH}"
    BSD: date -r "${SOURCE_DATE_EPOCH}" "+%a, %d %b %Y %H:%M:%S %z"
  3. Improved code clarity - Added extensive comments explaining parentheses placement and glob expansion handling
  4. Minor Python syntax fix - Updated generate-original-source.py to use idiomatic i not in data instead of not i in data

Requirements / Checklist

  • [ x ] Read the Contributing Guidelines
  • [ x ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.
    Issue number where discussion took place: #xxx
  • [ n/a ] If this contains a font/glyph add its origin as background info below (e.g. URL)
  • [ n/a ] Verified the license of any newly added font, glyph, or glyph set. License is: xxx

What does this Pull Request (PR) do?

Fixes cross-platform compatibility issues in the font patching script, particularly for macOS users. The filtering logic now works correctly on BSD-based systems (macOS) where find -iregex and date command syntax differ from GNU coreutils.

How should this be manually tested?

  • On a macOS system, run the gotta-patch-em-all-font-patcher!.sh script with a filter argument (e.g., ./gotta-patch-em-all-font-patcher!.sh Hack)
  • Verify the date commands execute without errors (previously would fail on BSD date)
  • Confirm font filtering correctly matches fonts by directory path or filename
  • Test on Linux (GNU) to ensure no regressions

Any background context you can provide?

Intended to add icons for CachyOS and zsh, and while doing that using a Mac (M4max MacBook Pro, macOS 26.2, stock Mac bash 3.2.57 and Homebrew bash 5.3.9 used for testing) discovered that the command syntax for some of the basic utils was not cleanly compatible with Mac's BSD-origin utils. Found issues with filtering logic that presented similar incompatibility. Separating the code contribution from new icons request.

What are the relevant tickets (if any)?

(no tickets)

Screenshots (if appropriate or helpful)

(not particularly helpful here I think)

@Finii Finii left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR and disentangling the icon addition from the source code.

Some small remarks.

Comment thread bin/scripts/lib/i_seti.sh Outdated
Comment thread bin/scripts/gotta-patch-em-all-font-patcher!.sh
Comment thread bin/scripts/gotta-patch-em-all-font-patcher!.sh Outdated
@shaunchokshi
shaunchokshi requested a review from Finii January 12, 2026 14:36
@Finii
Finii force-pushed the add-cachyos-icons branch from b023fc1 to afdb5bd Compare January 12, 2026 17:23

@Finii Finii left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I removed some more remaims (see below) into your commit, and added a new commit with a code simplification.

What do you think?

git rebase -i HEAD~5
<marking first of your commits as 'edit'>
git rm src/svgs/cachyos.svg
git rm src/svgs/zsh.svg
git checkout HEAD^ -- src/svgs/icons.tsv
git checkout HEAD^ -- src/glyphs/original-source.otf
git checkout HEAD^ -- bin/scripts/lib/i_seti.sh
git commit --amend
git rebase --continue

Edit: With 'into your commit' I mean I edited your commit and force pushed the new version back to your repo.

@Finii Finii added this to the v3.5.0 milestone Jan 12, 2026
@Finii
Finii force-pushed the add-cachyos-icons branch from afdb5bd to e42951e Compare January 12, 2026 17:37
@Finii

Finii commented Jan 12, 2026

Copy link
Copy Markdown
Collaborator

Hmm, we both forgot to increase the script version numbers, put that into my commit and force push.

@shaunchokshi

Copy link
Copy Markdown
Contributor Author

Thank you, I appreciate your guidance and corrections, and especially patience with my first attempt at contributing to an OSS project!

@Finii

Finii commented Jan 15, 2026

Copy link
Copy Markdown
Collaborator

Your last commit was merging master into your branch. Projects differ what they want, but having master merged into a pull request, and that merged back to master creates a needlessly complex (non straightforward) history that can be hard to follow (and even harder to bisect, in repos where bisect is likely to be used).

This branch now looks like this:

image

And after merging the master branch would look like this:

image

If you instead of merge master into your branch would rebase master we get a much more clean and readable commit history:

image

I will just undo your last commit and instead rebase (and force push) - and then pull (merge) this PR.

Overall as projects handle this differently it is best to look how they handle it and/or just ignore how the main branch progresses and let the maintainers do it as they prefer or wait until they ask for some specific action.
Whatever, that is vastly different. I personally like 'clean' histories and fast-forward (i.e. rebase in the PR).

👍

Thank you a lot for the contribution!

shaunchokshi and others added 4 commits January 15, 2026 18:37
Updated gotta-patch-em-all-font-patcher!.sh to enhance the filename filtering logic by ensuring that the filter matches fonts that start with the specified filter argument. This change improves the accuracy of font searches and maintains the script's functionality.

Changes include:
- Modified the filename filter to match fonts starting with the filter argument.
- Updated comments for clarity regarding the filtering patterns.
…atibility

- Improved the find command to support both directory and filename filters for font files.
- Retained compatibility with existing patterns while updating the filtering logic.
- Enhanced date command handling for cross-platform compatibility between GNU and BSD systems.
- Updated comments for clarity and maintainability.

Updated gotta-patch-em-all-font-patcher!.sh to include additional comments clarifying the patterns used for directory and filename filters. This change improves code readability and maintains the script's functionality by ensuring users understand the filtering mechanisms in place.

Changes include:
- Added comments for directory and filename filter patterns.
- Ensured existing functionality remains intact while enhancing clarity.
bin/scripts/lib/i_seti.sh: removed two glyph lines missed during previous revert

Summary of changes to bin/scripts/gotta-patch-em-all-font-patcher!.sh:

- Add error checks to actual date conversion commands (not just version detection)
- Ensure script exits immediately on invalid SOURCE_DATE_EPOCH
- Add consistent error handling in patch_font() function
- Addresses maintainer feedback on lines 213 and 256"

---
Details:

> This changes the behavior, before - if the user gives an invalid `SOURCE_DATE_EPOCH` the script complained and exited. Now it just runs on - and the error message is output when no compatible `date` is found.
>
> I would insist that on new line 213 you should `if` on the actual command and not on `date --version`.

- Fixed misplaced conditional to check for success of actual conversion and not just success of date util-based "OS" detection.

---
> This can never happen, becaus we would exit farther up, right? Suggest exiting here also.

- Fixed the error handling behavior to be consistent (exit with code 2) if timestamp conversion fails (safe failure) rather than continuing with invalid date value (silent but an error and failure that was not recognized)
We can unify the error message, and thereby also not relying on the mare
existance of `--version` but on actual parsability of the timestamp.

[note]
Also increasing script versions after most recent changes.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Finii force-pushed the add-cachyos-icons branch from a39b0ce to 8461ead Compare January 15, 2026 17:38
@Finii
Finii merged commit 50e1eff into ryanoasis:master Jan 15, 2026
@shaunchokshi
shaunchokshi deleted the add-cachyos-icons branch January 16, 2026 16:00
@Finii

Finii commented Jan 22, 2026 •

Copy link
Copy Markdown
Collaborator

Strangely I didn't see this the last time 🤔

./bin/scripts/gotta-patch-em-all-font-patcher!.sh:177:3: warning: find_path_pattern appears unused. Verify use (or export if used externally). [SC2034]
./bin/scripts/gotta-patch-em-all-font-patcher!.sh:179:3: warning: like_pattern appears unused. Verify use (or export if used externally). [SC2034]

find_path_filter_with_pattern is filled directly instead of built with find_path_pattern.

And like_pattern has been superseeded by find_cmd_args?

There are also some SC2155 warnings (by me) that I would remove then in one go.

  export SOURCE_DATE_EPOCH=$(date +%s)
         ^---------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.

@Finii

Finii commented Jan 22, 2026 •

Copy link
Copy Markdown
Collaborator

I would do this:
What do you think?

From 7c654f115ae6c3e93c754ae59e6948054bfcd756 Mon Sep 17 00:00:00 2001
From: Fini Jastrow <ulf.fini.jastrow@desy.de>
Date: Thu, 22 Jan 2026 16:21:23 +0100
Subject: [PATCH] gotta-patch-em: Remove shellcheck warnings

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
---
 .../gotta-patch-em-all-font-patcher!.sh        | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/bin/scripts/gotta-patch-em-all-font-patcher!.sh b/bin/scripts/gotta-patch-em-all-font-patcher!.sh
index e85787d15..7927d85da 100755
--- a/bin/scripts/gotta-patch-em-all-font-patcher!.sh
+++ b/bin/scripts/gotta-patch-em-all-font-patcher!.sh
@@ -30,7 +30,6 @@ sd="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 || exit ; pwd -P )"
 
 repo_root_dir=$(dirname "$(dirname "${sd}")") # two levels up (i.e. ../../)
 # Set source and target directories
-like_pattern='.*\.\(otf\|ttf\|sfd\)'
 last_font_root=""
 unpatched_parent_dir="src/unpatched-fonts"
 patched_parent_dir="patched-fonts"
@@ -155,28 +154,20 @@ then
     # For directory filter, -ipath must be outside the parentheses grouping
     find_cmd_args=(-iname "*.ttf" -o -iname "*.otf" -o -iname "*.sfd")
     find_path_filter="-ipath"
-    find_path_pattern="*${filter_dir}/*"
     find_path_filter_with_pattern=(-ipath "*${filter_dir}/*")
     echo "$LINE_PREFIX Filter given, limiting search and patch to pathname pattern '$filter_arg'"
-    # Pattern for directory filter: match paths containing the directory
-    like_pattern=".*/${filter_dir}/.*\.\(otf\|ttf\|sfd\)"
   else
     # Filename filter: match fonts with filter in filename
     find_cmd_args=(-iname "*${filter_arg}*.ttf" -o -iname "*${filter_arg}*.otf" -o -iname "*${filter_arg}*.sfd")
     find_path_filter=""
-    find_path_pattern=""
     find_path_filter_with_pattern=()
     echo "$LINE_PREFIX Filter given, limiting search and patch to filename pattern '$filter_arg'"
-    # Pattern for filename filter: match filter in filename
-    like_pattern=".*${filter_arg}.*\.\(otf\|ttf\|sfd\)"
   fi
 else
   # No filter
   find_cmd_args=(-iname "*.ttf" -o -iname "*.otf" -o -iname "*.sfd")
   find_path_filter=""
-  find_path_pattern=""
   find_path_filter_with_pattern=()
-  like_pattern='.*\.\(otf\|ttf\|sfd\)'
 fi
 
 # correct way to output find results into an array (when files have space chars, etc)
@@ -207,7 +198,8 @@ echo "$LINE_PREFIX Total source fonts found: ${#source_fonts[*]}"
 # Use one date-time for ALL fonts and for creation and modification date in the font file
 if [ -z "${SOURCE_DATE_EPOCH}" ]
 then
-  export SOURCE_DATE_EPOCH=$(date +%s)
+  SOURCE_DATE_EPOCH=$(date +%s)
+  export SOURCE_DATE_EPOCH
 fi
 # Detect GNU vs BSD date implementations reliably
 if date -R "--date=@${SOURCE_DATE_EPOCH}" >/dev/null 2>&1; then
@@ -233,7 +225,8 @@ function patch_font {
     # take everything before the last slash (/) to start building the full path
     local ts_font_dir="${f%/*}/"
     local ts_font_dir="${ts_font_dir/$unpatched_parent_dir/$timestamp_parent_dir}"
-    local one_font=$(find "${ts_font_dir}" -name '*.[ot]tf' | head -n 1)
+    local one_font
+    one_font=$(find "${ts_font_dir}" -name '*.[ot]tf' | head -n 1)
     if [ -n "${one_font}" ]
     then
       orig_font_date=$(ttfdump -t head "${one_font}" | \
@@ -353,7 +346,8 @@ function generate_info {
 
   [[ -d "$patched_font_dir" ]] || mkdir -p "$patched_font_dir"
 
-  local font_root=$(echo "$patched_font_dir" | sed "s|.*$patched_parent_dir/||;s|/.*||")
+  local font_root
+  font_root=$(echo "$patched_font_dir" | sed "s|.*$patched_parent_dir/||;s|/.*||")
   # if first time with this font then re-build parent dir readme, else skip:
   if [ "$last_font_root" != "$font_root" ]
   then
-- 
2.51.0

@shaunchokshi

Copy link
Copy Markdown
Contributor Author

Sorry for delay responding! Thank you for your diligence; I missed cleaning up the deprecated like_pattern after refactoring to separate the filename and path filtering (and consequently missed separating the declarative from the execution leading to the shell check warning).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants