Drop the unusable runtime .ico load from Ludo#17
Merged
Conversation
Verified while rebuilding the executables: SDL_image cannot decode
ludo_icon.ico ("Unsupported ICO bitmap format"), so the _load_icon()
helper added earlier in this branch always fell through to the
procedural icon. The bundled datas entry shipped a file nothing could
read, and _resource_path existed only to find it.
Remove both and call _make_icon() directly, documenting why the .ico is
not loaded at runtime. The .exe file/taskbar icon is unaffected: it
comes from the spec's separate icon= argument, which PyInstaller embeds
itself. (Monopoly's equivalent uses a .png, which pygame loads fine.)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #16, which is already merged. While rebuilding the four executables locally I verified that the Ludo window icon actually loads at runtime, and it does not.
The problem
pygame.image.load("ludo_icon.ico")raisespygame.error: Unsupported ICO bitmap format— SDL_image cannot decode this file. The_load_icon()helper added in #16 therefore caught the error and fell through to the procedural_make_icon()on every run, anddatas=[('ludo_icon.ico', '.')]bundled a file into the exe that nothing could read.So the change was dead code plus a pointless bundled asset. Nothing was visibly broken — the fallback icon is what shipped either way — which is exactly why the test suite and the headless autotest both stayed green through it. Only loading the file directly exposed the failure.
The fix
_make_icon()directly and document, in its docstring, why the.icois not loaded at runtime._load_icon()and_resource_path(), which existed only to serve it.datas=[]in the spec with a comment explaining that the.icois consumed by PyInstaller rather than at runtime.The .exe file and taskbar icon is unaffected. That comes from the spec's separate
icon=['ludo_icon.ico']argument, which PyInstaller embeds itself; it predates #16 and is untouched here.For contrast, Monopoly's equivalent runtime icon load works because it uses a
.png(verified: loads fine at 256x256). The resource-path pattern is sound — the.icoformat was the problem.Verification
ruff check .clean;mypy "Python Ludo Game"clean (sysandPathare still used elsewhere in the module, so no orphaned imports).Ludo Game.exefrom the fixed spec and ran it withLUDO_AUTOTEST=1; exits 0.Note on the branch
This reuses
claude/games-overhaul, whose previous PR (#16) was merged and the branch deleted. I pushed this commit before noticing the merge, which re-created the branch. GitHub computes the diff against currentmain, so this PR contains only the two files above. The branch can be deleted again on merge.🤖 Generated with Claude Code