SG-44731 Adopt Git LFS for vendored dependency ZIPs by julien-lang · Pull Request #1128 · shotgunsoftware/tk-core

julien-lang · 2026-08-10T14:50:55Z

Problem

Adopts Git LFS for large vendored/binary files in tk-core to reduce repo/clone size.

Changes

requirements/**/*.zip (pkgs.zip for all supported Python versions, flow_data_sdk-beta.zip) are now tracked via Git LFS (filter=lfs diff=lfs merge=lfs -text in .gitattributes).
python/tank/descriptor/io_descriptor/git.py: added _validate_lfs_content() on the shared IODescriptorGit base class, called after every clone in _clone_then_execute_git_commands() (covers both the branch and tag descriptors, for persistent downloads and transient temp-clones alike). If the checked out repo declares filter=lfs in .gitattributes, this verifies via git lfs ls-files --json that tracked files were actually resolved to real content, and raises a clear error if git-lfs isn't installed or some content is still a bare pointer, instead of silently proceeding with broken files.
docs/descriptor.rst: documented the new Git LFS requirements for the git/git_branch descriptors (need git-lfs installed, or Toolkit raises an error) and for the github_release descriptor (repo's "Include Git LFS objects in archives" GitHub setting must be enabled, since release archives otherwise contain unresolved LFS pointer files).

- .tkdeploy: replace obsolete CI file exclusions (.travis.yml, appveyor.yml) with the files actually used today (.pre-commit-config.yaml, azure-pipelines.yml, build_resources.yml, codecov.yml, developer/) - azure-pipelines.yml: restrict branch trigger to master (was "*"), and temporarily pin the tk-ci-tools template ref to ticket/SG-44731-use-git-lfs for end-to-end testing before that PR merges - .gitattributes: minor formatting (blank lines, comment header for the LFS rule)

codecov · 2026-08-10T14:57:41Z

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.11%. Comparing base (889c1cd) to head (4567992).

Files with missing lines	Patch %	Lines
python/tank/descriptor/io_descriptor/git.py	89.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1128      +/-   ##
==========================================
+ Coverage   80.09%   80.11%   +0.01%     
==========================================
  Files         203      203              
  Lines       19537    19556      +19     
==========================================
+ Hits        15649    15668      +19     
  Misses       3888     3888

Flag	Coverage Δ
Linux	`79.56% <89.47%> (+0.01%)`	⬆️
Python-3.10	`79.93% <89.47%> (+0.01%)`	⬆️
Python-3.11	`79.83% <89.47%> (+0.01%)`	⬆️
Python-3.13	`79.83% <89.47%> (+0.01%)`	⬆️
Python-3.9	`79.89% <89.47%> (+0.01%)`	⬆️
Windows	`79.60% <89.47%> (+0.03%)`	⬆️
macOS	`79.53% <89.47%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Adds _validate_lfs_content(), called at the end of _clone_then_execute_git_commands(), the shared clone path used by both the git branch and git tag descriptors (persistent downloads and transient temp-clones alike). If the checked out repo's root .gitattributes declares filter=lfs, verifies via 'git lfs ls-files --json' that tracked files were actually smudged to their real content. Raises TankGitError if: - the git-lfs binary isn't installed/on PATH, or - any tracked file is still a bare LFS pointer (not checked out). Repos that don't declare LFS tracking skip the check entirely, so there's no new dependency on git-lfs for the vast majority of existing git descriptors.

…-lfs

Copilot

🟡 Changes recommended

The new LFS validation parses git lfs ls-files --json using an incorrect JSON shape/field set, which will raise at runtime and block descriptor operations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces Git LFS tracking for large vendored ZIP dependencies and adds runtime validation in the git descriptor implementation to detect unresolved LFS pointer files early, with accompanying documentation updates.

Changes:

Track requirements/**/*.zip via Git LFS by updating .gitattributes.
Add an LFS validation step after git clones in IODescriptorGit to error if LFS content isn’t resolved.
Document Git LFS requirements/constraints for git- and GitHub-release-based descriptors.

File summaries

File	Description
`python/tank/descriptor/io_descriptor/git.py`	Adds `_validate_lfs_content()` and invokes it after clone/checkout to detect missing/unresolved Git LFS content.
`docs/descriptor.rst`	Documents Git LFS requirements for git descriptors and release-archive caveats.
`.gitattributes`	Marks `requirements/*/.zip` as LFS-tracked.

Review details

Files reviewed: 3/8 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

- Add a new bare fixture repo (lfs-test-repo.git) with one file tracked via Git LFS, for use in real (non-mocked) descriptor tests. - Add skip_if_git_lfs_missing, mirroring skip_if_git_missing. - Add test_git_lfs.py covering the two behaviors _validate_lfs_content() is meant to guard: normal resolved LFS content checks out fine, and content left as unresolved pointer text (simulated via GIT_LFS_SKIP_SMUDGE=1) raises TankDescriptorError.

Copilot

🟡 Changes recommended

Unresolved LFS validation, error-handling, compatibility, and test coverage issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

docs/descriptor.rst:326

This note is inside the Tracking against commits in a git branch section, so users of the git tag descriptor introduced at the earlier section will not see the new Git LFS requirement. Move the note to a shared Git-descriptor section or repeat it under the tag section so the documentation matches the base-class behavior for both descriptor types.

.. note:: If a repository uses `Git LFS <https://git-lfs.com>`_ to store some of its files
          (declared via ``filter=lfs`` entries in its ``.gitattributes``), the machine
          downloading the descriptor also needs ``git-lfs`` installed and initialized
          (``git lfs install``). If it isn't, Toolkit will raise an error rather than
          silently checking out files that still contain unresolved LFS pointer text.

python/tank/descriptor/io_descriptor/git.py:206

The new integration tests exercise only git_branch, while this shared hook also changes git tag downloads through the ref clone path. Add a tag-based LFS case so regressions in tag checkout and validation are covered as promised by the PR.

        self._validate_lfs_content(target_path)

python/tank/descriptor/io_descriptor/git.py:245

The new error path for an unavailable git-lfs executable is not covered: the test class is skipped whenever git-lfs is missing, and the unresolved test only exercises a successful git lfs ls-files call with checkout: false. Add a mocked command-failure case so regressions in this user-facing error handling are detected.

        except Exception as err:
            raise TankGitError(
                f"{self} uses Git LFS to store some of its files, but git-lfs "
                "does not appear to be installed on this machine. Install "
                "it from https://git-lfs.com, run `git lfs install`, and "
                "try again."
            ) from err

python/tank/descriptor/io_descriptor/git.py:206

Because _clone_then_execute_git_commands is also used by has_remote_access(), this new validation error is caught by that method's broad except and converted to False. For a version-less descriptor with resolve_latest=True, the factory then reports only Could not get latest version (or falls back to a stale cache), so the promised clear Git LFS error is lost. Propagate a dedicated LFS-validation error through the connectivity probe or handle it separately.

        self._validate_lfs_content(target_path)

tests/descriptor_tests/test_git_lfs.py:32

Because this entire class is skipped when git-lfs is unavailable, the new error path in _validate_lfs_content() for a missing executable is never exercised in the environment that triggers it. Keep the integration tests conditional, but add a unit-level case that mocks the LFS command failure and asserts the clear TankGitError message.

@skip_if_git_lfs_missing

tests/python/tank_test/tank_test_base.py:125

The new helper docstring has two grammatical errors; please use “Tests if” and “True if” so the test utility documentation is clear.

    Tests is git-lfs is available in PATH
    :returns: True is git-lfs is available, False otherwise.

tests/python/tank_test/tank_test_base.py:125

The return-value description is inverted: this helper returns True when the probe fails (git-lfs is missing) and False after a successful probe. It also has grammatical errors, so the docstring should describe the actual missing-state result.

    Tests is git-lfs is available in PATH
    :returns: True is git-lfs is available, False otherwise.

Files reviewed: 5/10 changed files
Comments generated: 3
Review effort level: Lite

carlos-villavicencio-adsk

LGTM. Just a couple of observations that are not blocking to move forward.

carlos-villavicencio-adsk · 2026-09-14T16:33:46Z

                )
            log.debug("Execution successful. stderr/stdout: '%s'" % output)

+        self._validate_lfs_content(target_path)


Just thinking out loud, not a real request from my side.

What if we validate the lfs content earlier to prevent doing a lot of steps that will end up meaningless when LFS is not installed? That way we can warn the user in advance.

I'm not sure if that will be possible to be honest.

It's not possible unfortunately. We first need to clone the repo before indentifying if it's using LFS.

…date_lfs_content

julien-lang added 2 commits August 10, 2026 11:04

test

9c5063f

This was referenced Aug 10, 2026

SG-44731 Adopt Git LFS for tk-framework-desktopserver vendored dependency ZIPs shotgunsoftware/tk-framework-desktopserver#365

Open

SG-44731 Adopt Git LFS for vendored dependency ZIPs shotgunsoftware/tk-framework-desktopstartup#160

Draft

julien-lang added 3 commits August 13, 2026 15:59

cleanup

d17ca35

LFS Py res files

2c3863d

LFS fonts

48e3f31

hound Bot reviewed Aug 13, 2026

View reviewed changes

Comment thread python/tank/platform/qt/resources_rc.py Outdated

Comment thread python/tank/platform/qt/resources_rc.py Outdated

julien-lang added 2 commits September 4, 2026 12:41

Merge remote-tracking branch 'origin/master' into ticket/SG-42549-git…

fe8bbd3

…-lfs

julien-lang changed the title ~~SG-44731 Adopt Git LFS for tk-core vendored dependency ZIPs~~ SG-44731 Adopt Git LFS for tk-core vendored dependency ZIPs and Qt resources Sep 4, 2026

julien-lang added 2 commits September 4, 2026 13:50

Document Git LFS requirements for git and github_release descriptors

7367e16

Cleanup

a1e1694

julien-lang marked this pull request as ready for review September 8, 2026 22:24

julien-lang added 2 commits September 8, 2026 15:27

Cleanup

60055cf

better

6e6185e

julien-lang requested a lite review from Copilot September 9, 2026 17:31

Copilot started reviewing on behalf of julien-lang September 9, 2026 17:32 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread python/tank/descriptor/io_descriptor/git.py

Comment thread .gitattributes

Comment thread python/tank/descriptor/io_descriptor/git.py

julien-lang changed the title ~~SG-44731 Adopt Git LFS for tk-core vendored dependency ZIPs and Qt resources~~ SG-44731 Adopt Git LFS for tk-core vendored dependency ZIPs Sep 9, 2026

julien-lang changed the title ~~SG-44731 Adopt Git LFS for tk-core vendored dependency ZIPs~~ SG-44731 Adopt Git LFS for vendored dependency ZIPs Sep 9, 2026

stevelittlefish reviewed Sep 10, 2026

View reviewed changes

Comment thread python/tank/descriptor/io_descriptor/git.py

julien-lang added 2 commits September 10, 2026 13:45

Build the Git LFS test fixture dynamically instead of checking it in

81d268d

stevelittlefish approved these changes Sep 11, 2026

View reviewed changes

julien-lang requested a review from carlos-villavicencio-adsk September 11, 2026 18:06

cabote requested a lite review from Copilot September 11, 2026 19:07

Copilot started reviewing on behalf of cabote September 11, 2026 19:08 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread tests/descriptor_tests/test_git_lfs.py

Comment thread python/tank/descriptor/io_descriptor/git.py

Comment thread tests/descriptor_tests/test_git_lfs.py

carlos-villavicencio-adsk approved these changes Sep 14, 2026

View reviewed changes

Catch SubprocessCalledProcessError instead of bare Exception in _vali…

4567992

…date_lfs_content

This comment was marked as off-topic.

Sign in to view

Navigation Menu

Uh oh!

SG-44731 Adopt Git LFS for vendored dependency ZIPs - #1128

SG-44731 Adopt Git LFS for vendored dependency ZIPs#1128
julien-lang wants to merge 14 commits into
masterfrom
ticket/SG-42549-git-lfs

julien-lang commented Aug 10, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-villavicencio-adsk left a comment

Uh oh!

Uh oh!

carlos-villavicencio-adsk Sep 14, 2026

Uh oh!

julien-lang Sep 15, 2026

Uh oh!

This comment was marked as off-topic.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

julien-lang commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Uh oh!

codecov Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (7)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-villavicencio-adsk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-villavicencio-adsk Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

julien-lang Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

julien-lang commented Aug 10, 2026 •

edited

Loading

codecov Bot commented Aug 10, 2026 •

edited

Loading