ci: guard against MAX_PATH-busting packaged wheel paths - #29587
Conversation
Greptile SummaryThis PR adds a CI guard that builds the litellm wheel and fails if any packaged path exceeds 160 characters, preventing a recurrence of the Windows
Confidence Score: 4/5Safe to merge — adds only a new CI check with no changes to library code or existing workflows. Both files are net-new and purely additive. The check script has a minor resource-management issue (ZipFile not wrapped in a context manager) and the workflow's broad pull_request trigger may fire on more branches than intended, but neither affects correctness or security of the guard itself. .github/scripts/check_wheel_path_length.py for the unclosed ZipFile handle; .github/workflows/check-wheel-path-length.yml to confirm the intended trigger scope.
|
| Filename | Overview |
|---|---|
| .github/scripts/check_wheel_path_length.py | New guard script: opens built wheel as a zip, reports entries exceeding 160-char threshold, exits non-zero on violations. Minor: ZipFile opened without a context manager. |
| .github/workflows/check-wheel-path-length.yml | New CI workflow: builds wheel with uv then runs the path-length check. Triggers on all PRs (no branch filter) and pushes to main/litellm_oss_branch; no Python version pinned but the check script requires only stdlib. |
Reviews (1): Last reviewed commit: "ci: run wheel path-length guard on PRs" | Re-trigger Greptile
| for whl in wheels: | ||
| names = zipfile.ZipFile(whl).namelist() | ||
| longest = max((len(n) for n in names), default=0) | ||
| offenders = sorted( | ||
| (n for n in names if len(n) > MAX_RELATIVE), key=len, reverse=True | ||
| ) | ||
| print(f"{os.path.basename(whl)}: {len(names)} entries, longest path = {longest}") | ||
| if offenders: | ||
| rc = 1 | ||
| print( | ||
| f"::error::{len(offenders)} packaged path(s) exceed {MAX_RELATIVE} chars " | ||
| f"and risk the Windows MAX_PATH limit at install time:" | ||
| ) | ||
| for n in offenders[:15]: | ||
| print(f" {len(n):4} {n}") |
There was a problem hiding this comment.
The
ZipFile is opened without a context manager, so the file handle is never explicitly closed. On Windows, this can hold a file lock for longer than necessary (until GC), which can matter if the script is extended to write or move wheel files later. Use with to guarantee cleanup.
| for whl in wheels: | |
| names = zipfile.ZipFile(whl).namelist() | |
| longest = max((len(n) for n in names), default=0) | |
| offenders = sorted( | |
| (n for n in names if len(n) > MAX_RELATIVE), key=len, reverse=True | |
| ) | |
| print(f"{os.path.basename(whl)}: {len(names)} entries, longest path = {longest}") | |
| if offenders: | |
| rc = 1 | |
| print( | |
| f"::error::{len(offenders)} packaged path(s) exceed {MAX_RELATIVE} chars " | |
| f"and risk the Windows MAX_PATH limit at install time:" | |
| ) | |
| for n in offenders[:15]: | |
| print(f" {len(n):4} {n}") | |
| for whl in wheels: | |
| with zipfile.ZipFile(whl) as zf: | |
| names = zf.namelist() | |
| longest = max((len(n) for n in names), default=0) | |
| offenders = sorted( | |
| (n for n in names if len(n) > MAX_RELATIVE), key=len, reverse=True | |
| ) | |
| print(f"{os.path.basename(whl)}: {len(names)} entries, longest path = {longest}") | |
| if offenders: | |
| rc = 1 | |
| print( | |
| f"::error::{len(offenders)} packaged path(s) exceed {MAX_RELATIVE} chars " | |
| f"and risk the Windows MAX_PATH limit at install time:" | |
| ) | |
| for n in offenders[:15]: | |
| print(f" {len(n):4} {n}") |
| # #22039, #29536, #29553). | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
Workflow triggers on all PRs regardless of target branch
The pull_request: trigger has no branches: filter, so this job fires on PRs targeting any branch (not just main/litellm_oss_branch). That's likely intentional as a broad guard, but worth confirming — if the intent is to match the push: filter, a branches: restriction here would keep the scope consistent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks for this! We already have a ci step inside of Circle CI that deals with Windows, so I think that is a better home for these. I expanded the tests to cover this case. Thanks for bringing this to our attention! |
What
Adds a CI check that builds the wheel and fails if any packaged path is long enough to risk the Windows 260-char
MAX_PATHlimit at install time (default threshold: 160 chars inside the wheel)..github/scripts/check_wheel_path_length.py— opens the built*.whland reports/fails on over-long entries..github/workflows/check-wheel-path-length.yml—uv build --wheelthen runs the check.Why
This is the durable counterpart to #29553. The content-filter benchmark fixtures have now broken
pip install litellmon default Windows three times (#21941 → #22039 → #29536 / #29553), each fixed by renaming. A guard makes the constraint explicit so it can't regress a fourth time.Because the check runs on the built wheel, it honours
[tool.uv.build-backend].source-exclude— i.e. it passes as soon as the offending fixtures are excluded/shortened, and trips if anything over-long gets packaged again.Threshold rationale
The installed path is
<site-packages prefix> + <path inside wheel>. A realistic worst-case Windows prefix (long profile name + roaming AppData venv) is ~95 chars:So we budget
260 − 100 = 160for the in-wheel path. (The real-world break in #29536 was a 176-char in-wheel path → 266 on disk.)Notes
litellm_oss_branchper the contribution policy.litellm/proxy/_experimental/out/**— as noted on fix(packaging): shorten content-filter benchmark fixtures to fit Windows MAX_PATH (#29536) #29553 that's the runtime proxy admin UI, and its longest path (134) is already under the limit.