Skip to content

Conversation

@chennes
Copy link
Member

@chennes chennes commented Jan 24, 2026

There are three addons that currently cannot be installed via the Addon Manager, but have to be installed manually, due to their extremely large size. On the server side a change will soon be made to do a sparse metadata-only checkout, then allowing direct-download of the addons using the Addon Manager, though those downloads will be coming from whatever host the Addon uses, not the FreeCAD-maintained cache.

Copilot AI review requested due to automatic review settings January 24, 2026 20:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for large addons through sparse checkout of cache. The cache system will perform sparse metadata-only checkouts for extremely large addons, with direct downloads coming from the addon's host rather than the FreeCAD cache.

Changes:

  • Added curated boolean field to track whether an addon is curated (schema and code)
  • Added sparse_cache boolean field to mark addons that use sparse cache approach (code only)
  • Modified URL selection logic to force zip_url usage for sparse cache entries

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
AddonCatalog.schema.json Replaced last_update_time with curated field in schema
AddonCatalog.py Added curated and sparse_cache fields; updated URL selection logic to handle sparse cache entries
Addon.py Added curated field initialization to Addon class

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +144 to +145
# TODO: Try to generate the expected URL form based on the repo location
raise RuntimeError(f"Sparse cache entry {addon_id} has no zip_url")
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The TODO comment suggests generating an expected URL from the repo location when zip_url is missing. However, the code raises a RuntimeError instead. If this fallback behavior is intended, it should be implemented now rather than raising an error. If sparse cache entries are guaranteed to have a zip_url by the server-side cache generation (as suggested by the PR description), then this TODO and the error handling may be unnecessary. Consider either: (1) implementing the URL generation logic now, (2) removing the TODO if the server guarantees zip_url will always be present, or (3) clarifying in the error message that this is a server-side configuration issue.

Suggested change
# TODO: Try to generate the expected URL form based on the repo location
raise RuntimeError(f"Sparse cache entry {addon_id} has no zip_url")
raise RuntimeError(
f"Sparse cache entry '{addon_id}' is missing a 'zip_url'. "
"This indicates a server-side sparse cache configuration or generation error."
)

Copilot uses AI. Check for mistakes.
"last_update_time": {
"type": "string"
"curated": {
"type": "boolean"
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The schema replaces last_update_time with curated, but in AddonCatalog.py (line 83), last_update_time is still present and actively used (line 166). This creates a discrepancy. Both fields are marked as "Generated by the cache system" in the Python code. If the schema is validated against cached catalog data, and curated is being added to the schema, then last_update_time should remain in the schema as well, or it should be properly removed from both the schema and the code. Additionally, sparse_cache (line 85 in AddonCatalog.py) is also cache-generated but missing from the schema. With additionalProperties: false set, schema validation will fail if these fields are present in the data but not defined in the schema.

Suggested change
"type": "boolean"
"type": "boolean"
},
"last_update_time": {
"description": "Generated by the cache system: timestamp of the last update for this catalog entry."
},
"sparse_cache": {
"description": "Generated by the cache system: auxiliary cache data associated with this catalog entry."

Copilot uses AI. Check for mistakes.
Comment on lines +140 to +145
if self.sparse_cache:
if self.zip_url:
url = self.zip_url
else:
# TODO: Try to generate the expected URL form based on the repo location
raise RuntimeError(f"Sparse cache entry {addon_id} has no zip_url")
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

The new sparse_cache functionality lacks test coverage. The instantiate_addon method now has a new code path (lines 140-145) that handles sparse cache entries differently, requiring a zip_url and raising a RuntimeError if it's missing. This new behavior should be tested to ensure it works correctly and that the error message is appropriate. Consider adding tests that cover: (1) sparse_cache=True with valid zip_url, (2) sparse_cache=True without zip_url (should raise RuntimeError), and (3) that the resulting Addon object is correctly configured when using sparse_cache.

Copilot uses AI. Check for mistakes.
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.

1 participant