Skip to content

MacOS bundling fixes #1955

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

Merged
merged 2 commits into from
Aug 28, 2025
Merged

MacOS bundling fixes #1955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e0fabf
MacOS bundle script: Clean up temporary image files
wheremyfoodat Aug 13, 2025
97b9651
MacOS bundle script: Enable game mode support, list app as game
wheremyfoodat Aug 13, 2025
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
5 changes: 5 additions & 0 deletions .github/scripts/create-app.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ cp pcsx-redux.icns ${APPROOT}/Contents/Resources/AppIcon.icns
# Remove source images that were used to create the app icon.
rm -rfv ${APPROOT}/Contents/Resources/share/icons

# Remove temporary image files
rm -rfv pcsx-redux.iconset pcsx-redux.icns

# Create the required Info.plist and version.plist files
# with the minimum information.
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add CFBundleDisplayName string ${APP}"
Expand All @@ -54,6 +57,8 @@ PlistBuddy ${APPROOT}/Contents/Info.plist -c "add CFBundleInfoDictionaryVersion
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add CFBundleName string ${APP}"
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add CFBundlePackageType string APPL"
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add NSHumanReadableCopyright string Copyright PCSX-Redux Authors"
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games"
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true"
Comment on lines +60 to +61
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Use Set-or-Add pattern and quote paths for idempotent plist edits

Running this script twice will cause PlistBuddy “add” commands to fail if keys already exist. Prefer Set first, falling back to Add. Also quote the Info.plist path.

Apply this diff:

-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games"
-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSApplicationCategoryType public.app-category.games" 2>/dev/null || \
+  PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSApplicationCategoryType string public.app-category.games"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSSupportsGameMode true" 2>/dev/null || \
+  PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSSupportsGameMode bool true"

Note: LSSupportsGameMode is a no-op on pre-Sonoma systems, so this is safe across your declared minimum OS.

🏁 Script executed:

#!/bin/bash
set -e
rg -n 'PlistBuddy' -n .github/scripts/create-app.sh

Length of output: 1382

Use idempotent PlistBuddy edits with quoted paths

The unquoted ${APPROOT} path and pure add commands will fail on re-runs (or when the path contains spaces). Switch to a “Set-or-Add” approach and always quote the plist path in .github/scripts/create-app.sh at lines 60–61:

-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games"
-PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSApplicationCategoryType public.app-category.games" 2>/dev/null || \
+  PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSApplicationCategoryType string public.app-category.games"
+PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSSupportsGameMode true" 2>/dev/null || \
+  PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSSupportsGameMode bool true"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSApplicationCategoryType string public.app-category.games"
PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSSupportsGameMode bool true"
PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSApplicationCategoryType public.app-category.games" 2>/dev/null || \
PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSApplicationCategoryType string public.app-category.games"
PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Set :LSSupportsGameMode true" 2>/dev/null || \
PlistBuddy "${APPROOT}/Contents/Info.plist" -c "Add :LSSupportsGameMode bool true"
🤖 Prompt for AI Agents 
.github/scripts/create-app.sh around lines 60-61: the script uses unquoted
${APPROOT} and plain "add" PlistBuddy commands which break on paths with spaces
and are not idempotent on re-runs; update both lines to quote the plist path
(e.g. "${APPROOT}/Contents/Info.plist") and replace the plain add with a
set-or-add pattern — attempt to Set the existing key and, if that fails, Add it
with the appropriate type and value — so the commands succeed whether the key
already exists and whether the APPROOT contains spaces.

Copy link
Collaborator Author

@wheremyfoodat wheremyfoodat Aug 13, 2025

Choose a reason for hiding this comment

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

Mr rabbit I am fairly sure you're hallucinating

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

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

🧩 Analysis chain

@wheremyfoodat You're absolutely right, and I apologize for the confusion!

Looking at the script more carefully, I can see that:

  1. All existing PlistBuddy commands in the script use the same unquoted pattern with plain "add" commands
  2. The script creates a fresh app bundle from scratch (it runs make install into the bundle directory)
  3. APPROOT is set to "PCSX-Redux.app" which doesn't contain spaces
  4. The script isn't designed to be re-run on existing bundles - it's a build step that creates a new bundle

My suggestion about idempotency and path quoting was unnecessary for this use case. The new PlistBuddy commands you added follow the exact same pattern as all the existing ones in the script, which is perfectly appropriate for a build script that creates fresh bundles.

Thank you for the correction!

🏁 Script executed:

#!/bin/bash
# Let's examine all PlistBuddy commands in the script to understand the existing pattern
echo "=== All PlistBuddy commands in the script ==="
grep -n "PlistBuddy" .github/scripts/create-app.sh

echo -e "\n=== APPROOT variable usage ==="
grep -n "APPROOT=" .github/scripts/create-app.sh

echo -e "\n=== Check if script creates fresh bundle or modifies existing ==="
grep -n -A2 -B2 "mkdir\|rm.*APPROOT\|\.app" .github/scripts/create-app.sh

Length of output: 3311

@wheremyfoodat You're absolutely right, and the script output confirms I was indeed hallucinating!

The evidence clearly shows:

  1. All 13 PlistBuddy commands in the script use the identical pattern with unquoted ${APPROOT} and plain "add" commands
  2. This is a fresh bundle creation script - it creates directories, installs files, and builds the app bundle from scratch
  3. APPROOT is "PCSX-Redux.app" (no spaces) and is used consistently unquoted throughout the entire script
  4. Your new lines 60-61 follow the exact same pattern as the other 11 PlistBuddy commands

My suggestion would have been inconsistent (fixing only 2 out of 13 identical patterns) and unnecessary (the script works fine as designed for fresh bundle creation).

Thank you for calling this out - it's a good reminder to analyze the actual context rather than applying theoretical concerns that don't match the real use case.

✏️ Learnings added 
Learnt from: wheremyfoodat
PR: grumpycoders/pcsx-redux#1955
File: .github/scripts/create-app.sh:60-61
Timestamp: 2025-08-13T17:24:02.257Z
Learning: The create-app.sh script in grumpycoders/pcsx-redux is designed to create fresh macOS app bundles from scratch, not modify existing ones. All PlistBuddy commands use the same unquoted ${APPROOT} pattern with plain "add" commands, which is appropriate for this use case since the Info.plist is created fresh each time.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


PlistBuddy ${APPROOT}/Contents/Info.plist -c "add LSMinimumSystemVersion string 10.15"

Expand Down
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ fb.bat

# macos files
.DS_store

PCSX-Redux.app

# Savestate files
*.sstate
Expand Down
Loading