Repository navigation
ci: move the large file check into a script shared with rust_lint.sh - #25440
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25440 +/- ##
==========================================
+ Coverage 82.33% 82.36% +0.02%
==========================================
Files 1137 1137
Lines 432049 432937 +888
Branches 432049 432937 +888
==========================================
+ Hits 355723 356577 +854
- Misses 54826 54829 +3
- Partials 21500 21531 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @akashjainn for working on this. Left few comments please take a look.
| echo "[${SCRIPT_NAME}] origin/main is not available; pass --base <ref> to choose the start of the range" >&2 | ||
| exit 1 | ||
| fi | ||
| BASE_REF="$(git merge-base "${HEAD_REF}" origin/main)" |
There was a problem hiding this comment.
For most contributors, origin is their fork, so origin/main is not the upstream branch. Then the check covers commits they did not write, or it fails when origin/main does not exist. Would it work to use the remote that points at apache/datafusion, and use origin only when there is no such remote?
There was a problem hiding this comment.
Thank you for pointing this out! The default base now uses main on the remote whose URL points at apache/datafusion, and falls back to origin only when there is no such remote.
Fixed here: 3389473
| echo "::error file=${path}::File ${path} has size ${size}, exceeding ${MAX_FILE_SIZE_BYTES} limit." | ||
| fi | ||
| fi | ||
| done < <( |
There was a problem hiding this comment.
If git rev-list or git cat-file fails here, the loop sees no lines and the check passes. An object listed as missing is skipped in the same way. I'd make both cases fail the check.
There was a problem hiding this comment.
The object list is now captured into a variable before the loop, so a failing git rev-list or git cat-file fails the check under set -e and pipefail.
Fixed here: 3389473
| ;; | ||
| --max-bytes) | ||
| [[ $# -ge 2 ]] || usage | ||
| MAX_FILE_SIZE_BYTES="$2" |
There was a problem hiding this comment.
a value that is not a number is accepted here. Every size comparison then fails, so the check passes.
There was a problem hiding this comment.
Added a check for a valid limit value. Thank you for flagging that!
Fixed here: 3389473
…n-number values rejected
|
Thank you @akashjainn |
…pache#25440) ## Which issue does this PR close? - Part of apache#21048. The issue stays open for the other checklist items. ## Rationale for this change The "Large files PR check" workflow rejects pull requests that commit a file larger than 1.5 MB, but its logic lives inline in the workflow YAML, so a contributor cannot run it before pushing and only learns about an oversized file from CI. This moves the check into a script that CI and the local lint suite share, so the "Large file check" item in apache#21048 is covered the same way apache#25359 covered the security audit. ## What changes are included in this PR? - `ci/scripts/check_large_files.sh` is a new executable script. It lists the objects committed between a base ref and a head ref and fails if any blob is larger than the limit. `--base` defaults to the merge base of the head and `apache/datafusion`, `--head` defaults to `HEAD`, and `--max-bytes` defaults to the 1.5 MB the workflow used. Both refs are validated before the range is listed. The `::error` annotation is only printed when `GITHUB_ACTIONS` is set, so local runs get the plain message on stderr. - `.github/workflows/large_files.yml` keeps the same job, step name, triggers, permissions, runner and checkout, and its run step now passes the PR or merge-queue SHAs it already computed to the script. The size limit and its comment move from the step's `env:` block into the script. - `dev/rust_lint.sh` runs the script as a read-only step after the ASF status check validation. The `--write` and `--allow-dirty` flags never reach it. - `docs/source/contributor-guide/testing.md` documents the standalone command and the range flags. One difference from the inline version: the script sizes objects with a single `git cat-file --batch-check` instead of one `git cat-file -s` per object, and skips commits and trees by object type rather than by an empty path. On a 2000 commit range with 29,214 objects, the inline loop took 11 min 20 s locally and the script took 0.8 s, with identical output. ## What is the testing strategy for this PR? - The old inline loop and the new script were run on the same four ranges of this repository's history, including the commit that added the 3.7 MB `docs/logos/Datafusion_Branding_Guideline.pdf`. Exit codes and stderr were identical for all four: three passing ranges and the PDF commit failing with the same message. - In a throwaway worktree, a 2 MB file was committed on a branch off `origin/main` and the script was run with no arguments. It picked the merge base as the default and failed on the new file. `--max-bytes 20000` on a small range fails on the expected files, and an unknown `--base` or `--head` exits 1 with a message naming the ref. - With `GITHUB_ACTIONS=true` the `::error file=` line is printed; without it, nothing is printed to stdout. - The script runs under bash 3.2 (macOS `/bin/bash`) and bash 5, and passes `shellcheck`. `typos`, `prettier`, `check_asf_yaml_status_checks.py` and `check_no_cargo_install_in_workflows.sh` pass on the changed files. The workflow file parses and still has the single `check-files` job with the `Check size of new Git objects` step. ## Are there any user-facing changes? No. Contributors get a new local check via `./dev/rust_lint.sh` or `./ci/scripts/check_large_files.sh`.
Which issue does this PR close?
Rationale for this change
The "Large files PR check" workflow rejects pull requests that commit a file larger than 1.5 MB, but its logic lives inline in the workflow YAML, so a contributor cannot run it before pushing and only learns about an oversized file from CI. This moves the check into a script that CI and the local lint suite share, so the "Large file check" item in #21048 is covered the same way #25359 covered the security audit.
What changes are included in this PR?
ci/scripts/check_large_files.shis a new executable script. It lists the objects committed between a base ref and a head ref and fails if any blob is larger than the limit.--basedefaults to the merge base of the head andapache/datafusion,--headdefaults toHEAD, and--max-bytesdefaults to the 1.5 MB the workflow used. Both refs are validated before the range is listed. The::errorannotation is only printed whenGITHUB_ACTIONSis set, so local runs get the plain message on stderr..github/workflows/large_files.ymlkeeps the same job, step name, triggers, permissions, runner and checkout, and its run step now passes the PR or merge-queue SHAs it already computed to the script. The size limit and its comment move from the step'senv:block into the script.dev/rust_lint.shruns the script as a read-only step after the ASF status check validation. The--writeand--allow-dirtyflags never reach it.docs/source/contributor-guide/testing.mddocuments the standalone command and the range flags.One difference from the inline version: the script sizes objects with a single
git cat-file --batch-checkinstead of onegit cat-file -sper object, and skips commits and trees by object type rather than by an empty path. On a 2000 commit range with 29,214 objects, the inline loop took 11 min 20 s locally and the script took 0.8 s, with identical output.What is the testing strategy for this PR?
docs/logos/Datafusion_Branding_Guideline.pdf. Exit codes and stderr were identical for all four: three passing ranges and the PDF commit failing with the same message.origin/mainand the script was run with no arguments. It picked the merge base as the default and failed on the new file.--max-bytes 20000on a small range fails on the expected files, and an unknown--baseor--headexits 1 with a message naming the ref.GITHUB_ACTIONS=truethe::error file=line is printed; without it, nothing is printed to stdout./bin/bash) and bash 5, and passesshellcheck.typos,prettier,check_asf_yaml_status_checks.pyandcheck_no_cargo_install_in_workflows.shpass on the changed files. The workflow file parses and still has the singlecheck-filesjob with theCheck size of new Git objectsstep.Are there any user-facing changes?
No. Contributors get a new local check via
./dev/rust_lint.shor./ci/scripts/check_large_files.sh.