Improve error recording for macro cache generation#354
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve developer-visible error recording during server-side macro cache generation by capturing FreeCAD console output (via Python logging) and persisting per-macro error details, alongside additional validation/error reporting for problematic icons.
Changes:
- Record FreeCAD console warnings/errors emitted during macro parsing into
macro_errors.jsonas per-macro lists (via a bounded in-memory logging handler). - Add/adjust macro metadata warnings (e.g., defaulting missing licenses) and normalize warning/message formatting.
- Add PNG inspection to detect duplicate embedded ICC profile chunks and surface warnings/errors for addon/macro icons.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| addonmanager_macro_parser.py | Adds warning/default behavior when a macro omits a license string; tweaks warning/message formatting. |
| addonmanager_icon_utilities.py | Introduces PNG chunk inspection helpers and a new icon creation debug print. |
| MacroCacheCreator.py | Captures console/log output per macro via a deque-backed logging handler; records multiple errors per macro; adds PNG iCCP-related warnings. |
| AddonCatalogCacheCreator.py | Records an icon error when PNG data contains duplicate ICC profile chunks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chennes
force-pushed
the
moreErrorsForMacros
branch
2 times, most recently
from
3631a22 to
92b41ad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif absolute_icon_path.lower().endswith(".png"): | ||
| if icon_utils.png_has_duplicate_iccp(icon_data): | ||
| self.icon_errors[metadata.name] = { | ||
| "valid_icon_path": relative_icon_path, | ||
| "error_message": "PNG data has duplicate ICCP chunk", | ||
| } | ||
|
|
||
| if icon_data_is_good: |
There was a problem hiding this comment.
The new PNG validation path (.png + png_has_duplicate_iccp) isn’t covered by the existing CacheWriter.generate_cache_entry_from_package_xml tests (which currently only exercise SVG icons). Adding a unit test that feeds a PNG with duplicate iCCP and asserts an entry in icon_errors (and expected icon_data behavior) would help prevent regressions.
chennes
force-pushed
the
moreErrorsForMacros
branch
2 times, most recently
from
4a8a9f2 to
00e64c0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.assertIs(a3, iu.cached_default_icons["package"]) | ||
|
|
||
|
|
||
| def build_png_data(chunk_types: list[bytes]): |
There was a problem hiding this comment.
Type annotation chunk_types: list[bytes] is not compatible with Python 3.8 (built-in generics aren’t subscriptable without postponed evaluation), which this repo still supports/tests. Use typing.List[bytes]/typing.Sequence[bytes] (or quote the annotation / enable from __future__ import annotations) to avoid runtime import errors in 3.8.
| macro = Macro(filename[:-8]) # Remove ".FCMacro". | ||
| if macro.name in self.macros: | ||
| print(f"Ignoring second macro named {macro.name} (found on git)\n") | ||
| self.log_error(macro.name, f"Ignoring second macro named {macro.name} (found on git)\n") |
There was a problem hiding this comment.
This error message string includes a trailing newline (\n), which will end up embedded in macro_errors.json and can complicate downstream display/processing. Consider removing the newline and letting the consumer control formatting.
| self.log_error(macro.name, f"Ignoring second macro named {macro.name} (found on git)\n") | |
| self.log_error(macro.name, f"Ignoring second macro named {macro.name} (found on git)") |
chennes
force-pushed
the
moreErrorsForMacros
branch
2 times, most recently
from
514b134 to
ecc8087
Compare
chennes
force-pushed
the
moreErrorsForMacros
branch
from
ecc8087 to
0454d88
Compare
chennes
added
the
release to main
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin main
git worktree add -d .worktree/backport-354-to-main origin/main
cd .worktree/backport-354-to-main
git switch --create backport-354-to-main
git cherry-pick -x 0454d888df934bdc6a2f675bce37d08a39da3dd0 |
Catch the console error output from the macro class while it builds the macros, and record it to a developer-visible log location on addons.freecad.org.