Skip to content

Fix mac app sound resource paths#250

Merged
zarvox32 merged 4 commits into
XGDevGroup:mainfrom
vlad-a-c:mac_compatibility
May 26, 2026
Merged

Fix mac app sound resource paths#250
zarvox32 merged 4 commits into
XGDevGroup:mainfrom
vlad-a-c:mac_compatibility

Conversation

@vlad-a-c
Copy link
Copy Markdown
Contributor

made it so the sounds are playable on the mac app bundle aswell.

@zarvox32
Copy link
Copy Markdown
Contributor

Tested locally on the PR branch. This is the clean split I recommended in #246 — scope concern from that thread is resolved. The single blocker is the Windows test failure I called out there on May 16, which was not addressed.

Blocker: same Windows-incompatible test as before.

Reproduced on this branch:

clients/desktop/tests/test_sound_manager.py::test_default_sounds_folder_is_absolute FAILED

E       AssertionError: assert False
E        +  where False = 'C:\Users\me\Documents\PlayPalace11\clients\desktop\sounds'.startswith('/')

tests/test_sound_manager.py:48 still uses manager.sounds_folder.startswith("/"), which is POSIX-only. Windows absolute paths begin with a drive letter. One-line fix:

assert os.path.isabs(manager.sounds_folder)

That's also a more accurate assertion of what the test name claims.

While you're in there — other things worth addressing in this PR rather than as follow-ups:

  1. No test covers the frozen branch. _default_sounds_folder() has two branches and the very one motivating this PR (getattr(sys, "frozen", False)) is uncovered. Easy add with monkeypatch.setattr(sys, "frozen", True) and a patched sys.executable.

  2. pyinstaller_runtime.py mutates global cwd. os.chdir(os.path.dirname(sys.executable)) runs unconditionally for any frozen build. Since _default_sounds_folder() already returns an absolute path, the chdir isn't actually needed for sound lookup — anything else in the client that uses relative paths now silently inherits this. Either drop the runtime hook entirely (the sound_manager change alone is sufficient for the bug you're fixing), or narrow it / document why it's needed.

  3. Non-standard PyInstaller data placement. PlayPalace.spec copies sounds via hand-rolled shutil.copytree to Contents/MacOS/sounds after BUNDLE. The idiomatic approach is datas=[('sounds', 'sounds')] in Analysis(...), which lets PyInstaller put them in _MEIPASS and treat them as resources. The current approach also puts data files in the binaries directory (Contents/MacOS/) rather than the conventional Contents/Resources/, which can cause issues with code signing / notarization down the line.

Non-blocking, just flagging: the project still has no equivalent build spec for Windows/Linux. Someone will need to fill that gap before non-mac users can get bundled builds, but that's not this PR's job.

Production code (sound_manager.py change) itself is correct — _default_sounds_folder() resolves properly in both source and frozen modes, and existing call sites use os.path.join(self.sounds_folder, name) so the switch from "sounds" to an absolute path is safe.

Once the test is platform-agnostic, this is good to merge.

@zarvox32
Copy link
Copy Markdown
Contributor

Tested locally — all 12 tests in tests/test_sound_manager.py pass, and the Mac .app path is correct (PyInstaller's BUNDLE sets sys._MEIPASS to the extraction root, so _MEIPASS/sounds resolves).

One regression risk worth addressing before merge: this likely breaks Windows --onedir builds produced by the existing clients/desktop/build.ps1 (and build.sh). Those scripts run, after PyInstaller:

Move-Item -Force $dist\_internal\sounds $dist\sounds

i.e. they relocate the sounds folder out of _internal/ and up beside the exe. At runtime in PyInstaller 6+ onedir, sys._MEIPASS still points at _internal/, so the new _default_sounds_folder() returns _internal/sounds — but that directory has been moved away. The old relative "sounds" lookup happened to work because the exe's CWD when double-clicked is the dist folder, where the moved copy lives.

The fallback branch (Path(sys.executable).parent / "sounds") only fires when _MEIPASS is missing, not when it exists but the folder under it doesn't — so the regression slips through silently and the Windows build loses all sounds.

Small fix — verify the _MEIPASS candidate exists before committing to it:

def _default_sounds_folder() -> str:
    if getattr(sys, "frozen", False):
        bundle_root = getattr(sys, "_MEIPASS", None)
        if bundle_root:
            candidate = Path(bundle_root) / "sounds"
            if candidate.is_dir():
                return str(candidate)
        return str(Path(sys.executable).resolve().parent / "sounds")
    return str(Path(__file__).resolve().parent / "sounds")

Plus a test for "_MEIPASS set but sounds dir absent → falls back to executable's sibling". Keeps Mac .app working and stops the Windows onedir build from regressing. Alternatively, drop the post-build Move-Item from the build scripts and let PyInstaller keep sounds inside _MEIPASS — same effect, fewer code paths.

Minor nits (non-blocking):

  • PlayPalace.spec sets bundle_identifier=None; recommended to set something like com.xgdevgroup.playpalace so macOS treats it as a distinct app (matters for launch services, codesigning, notarization down the line).
  • Neither build.sh nor build.ps1 invokes the new spec file, so on Mac you still need to run pyinstaller PlayPalace.spec manually to get the .app. Worth a one-line wrapper or a README note.

@zarvox32 zarvox32 merged commit d1b9bd7 into XGDevGroup:main May 26, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants