Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21762
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit ed0c8cd with merge base e60faa2 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
Co-authored-by: psiddh <2467117+psiddh@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the link-check lint scripts to diff against the merge base (base...head) instead of the base tip (base..head), so URL and xref linting only inspects lines introduced by the PR even when main has advanced since the feature branch diverged.
Changes:
- Switch
scripts/lint_urls.shandscripts/lint_xrefs.shfromgit diff base..headtogit diff base...head. - Add a regression test that constructs diverged
mainandfeaturehistories and asserts only feature-side changes are linted.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/lint_xrefs.sh | Use merge-base diff semantics for PR-only xref linting. |
| scripts/lint_urls.sh | Use merge-base diff semantics for PR-only URL linting. |
| .ci/scripts/tests/test_link_check_diff_selection.py | Regression test covering diverged-history selection for both URL and xref linting. |
Suppressed comments (2)
scripts/lint_urls.sh:78
- With
set -euo pipefail, this per-file pipeline can terminate the whole diff scan when a changed file has no “+” lines (e.g., deletion-only diffs). In that casegrep -Ev '^\+\+\+'returns exit code 1, which aborts the process substitution early and can cause the script to miss URLs in later files.
git diff --unified=0 "$1...$2" -- "$filename" "${excludes[@]}" \
| grep -E '^\+' \
| grep -Ev '^\+\+\+' \
| perl -nle 'print for m#'"$pattern"'#g' \
| sed 's|^|'"$filename"':|'
scripts/lint_xrefs.sh:43
- With
set -euo pipefail, this per-file pipeline can terminate the whole diff scan when a changed file has no “+” lines (e.g., deletion-only diffs). In that casegrep -Ev '^\+\+\+'returns exit code 1, which aborts the process substitution early and can cause the script to miss xrefs in later files.
git diff --unified=0 "$1...$2" -- "$filename" "${excludes[@]}" \
| grep -E '^\+' \
| grep -Ev '^\+\+\+' \
| perl -nle 'print for m#'"$pattern"'#g' \
| sed 's|^|'"$filename"':|'
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: psiddh <2467117+psiddh@users.noreply.github.com>
Co-authored-by: psiddh <2467117+psiddh@users.noreply.github.com>
| # A push creating a tag or branch reports an all zero SHA no remote can serve. | ||
| if [ -n "$BASE_REF" ] && [ "$BASE_REF" != "0000000000000000000000000000000000000000" ]; then | ||
| git fetch --no-tags --depth=1 origin "$BASE_REF" | ||
| git fetch --no-tags origin "$BASE_REF" | ||
| args=("$BASE_REF" "$HEAD_REF") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
.github/workflows/_link_check.yml:33
- This workflow still uses the default shallow checkout (
actions/checkoutwithoutfetch-depth: 0). Iflint_urls.sh/lint_xrefs.shuse merge-base diffing (git diff A...B), a shallow clone can be missing the merge base even aftergit fetch origin "$BASE_REF", causing merge-base-based diffs to fail.
Consider unshallowing the checkout (or setting fetch-depth: 0) so merge-base computations are reliable.
if [ -n "$BASE_REF" ] && [ "$BASE_REF" != "0000000000000000000000000000000000000000" ]; then
git fetch --no-tags origin "$BASE_REF"
args=("$BASE_REF" "$HEAD_REF")
.ci/scripts/tests/test_link_check_diff_selection.py:104
- Similarly, this xref test won't distinguish
BASE..HEADfromBASE...HEADbecausemain.mdis a base-side addition (which becomes a deletion in the diff and is filtered out). Makemainremove an xref that exists in the merge base soBASE..HEADwould incorrectly lint it as an added line.
def test_lint_xrefs_uses_merge_base_for_changed_lines(self) -> None:
base_sha, head_sha = self.create_diverged_history(
"feature.md",
textwrap.dedent(
"""\
[feature](docs/feature-target.md)
"""
),
"main.md",
textwrap.dedent(
"""\
[main](docs/main-target.md)
"""
),
)
.ci/scripts/tests/test_link_check_diff_selection.py:80
- As written, this test won't fail if
lint_urls.shstill usesgit diff BASE..HEAD: themain.mdURL is introduced only on the base side, so it shows up as a deletion (and the script filters to+lines). To make the test distinguish..vs..., havemainremove a URL that exists in the merge base (soBASE..HEADwould treat it as a new+line inHEAD).
def test_lint_urls_uses_merge_base_for_changed_lines(self) -> None:
base_sha, head_sha = self.create_diverged_history(
"feature.md",
"https://example.com/feature\n",
"main.md",
"https://example.com/main\n",
)
scripts/lint_xrefs.sh:40
git diff A...Bdepends on being able to compute the merge base. In the link-check workflow this script is typically run in anactions/checkoutshallow clone (defaultfetch-depth: 1), and_link_check.ymlstill fetchesBASE_REFshallowly for some jobs, which can leave the merge base unreachable and make this loop fail with "no merge base" errors.
Consider computing the merge base once with git merge-base (so failures can be reported with a clear message) and diffing $merge_base..$2 instead of using ... directly.
if [ $# -eq 2 ]; then
for filename in $(git diff --name-only --unified=0 "$1...$2"); do
git diff --unified=0 "$1...$2" -- "$filename" "${excludes[@]}" \
| grep -E '^\+' \
.ci/scripts/tests/test_link_check_diff_selection.py:24
- The test setup doesn't currently create any URL/xref in the merge base commit, so it can't detect the difference between
git diff BASE..HEADandgit diff BASE...HEAD. To exercise the regression, the merge base should contain a URL/xref thatmainlater removes (soBASE..HEADwould see it as an added line inHEAD, even though it predates the branch).
Without that, these tests can pass even if the scripts still use .. diffing.
This issue also appears in the following locations of the same file:
- line 74
- line 90
self.write("README.md", "base\n")
self.write("docs/feature-target.md", "feature target\n")
self.write("docs/main-target.md", "main target\n")
self.run_cmd("git add README.md docs/feature-target.md docs/main-target.md")
|
Closing this PR in favir of #21765 |
…21765) ### Summary The URL, xref, and file-size linters diffed `base..head`, where `base` is the **tip** of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions, and the branch gets blamed for links someone else already repaired. #21729 failed exactly this way. It touches 18 files and adds no URLs at all, but the two-dot diff scoped the lint to 143 files and flagged nine links that #21694 had already fixed or `@lint-ignore`d. #21707 is starker: one Python file with no URLs in it, 199 files linted, the same nine failures. ``` base..head 143 files <- what CI linted base...head 18 files <- what the PR actually changes ``` The stray `jq: parse error` lines in #21729's log are the same symptom from the other direction: the job runs the branch's own pre-#21694 copy of `lint_urls.sh`. ### Fix The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a `pull_request` the reusable workflow resolves from the merge commit, so it carries this fix even though `scripts/` still comes from the branch itself. The scripts also switch to three-dot ranges so `./scripts/lint_urls.sh main HEAD` by hand behaves the same. `lint_xrefs.sh` and `lint_file_size.sh` had the identical bug and get the identical change. **Cost, stated plainly.** A merge base needs real history, so this restores `fetch-depth: 0` on the `pull_request` path — line for line what #17682 removed in February for speed. Measured on this branch, that is ~25s of added wall clock and ~79s of runner time across the three concurrent jobs. #17682's 6min → 10s was mostly the runner and Docker change rather than the fetch depth, though the commit changes both at once and I can't fully separate them. Pushes and the nightly whole-tree scan cannot use a merge base and stay shallow: ```yaml fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} ``` The quotes matter — bare `0` is falsy in GitHub expressions, so `&& 0 || 1` always yields `1` and would silently disable the fix. **No silent fallback.** When there is no merge base there is no usable range, so the range is left unset and the linters scan the whole tree. Substituting the base tip instead produces a range the scripts fail on quietly: verified against unrelated histories, `lint_urls.sh` and `lint_xrefs.sh` both exit **0** having checked nothing, while `lint_file_size.sh` exits 128 and the wrapper reports "some files exceed the 1 MB limit", which is not what happened. Unset args are loud instead — verified rc=1 on a tree containing a dead link. **`--no-color`.** Both `git diff` calls now pass it, matching the `git grep --no-color` two lines below. With `color.ui = always` in a developer's config, added lines arrive wrapped in escape sequences, `grep -E '^\+'` matches nothing, and the check passes having found nothing (measured: 2 matches → 0). ### Test plan `.ci/scripts/tests/test_link_check_diff_selection.py` builds a diverged history where `main` repairs bad links and shrinks an oversized file while the feature branch simply predates all of it. Four fixture files each pin a different part, and `curl` is stubbed so there is no network: | Fixture | Pins | |---|---| | `both_sides.md` — edited on both branches | the per-file diff range | | `big.bin` — oversized at the branch point, shrunk on main | `lint_file_size.sh`'s range | | `colorful.gitconfig` — `color.ui = always` | `--no-color` | | `base_only.md` / `feature_only.md` | that main-only changes stay invisible and the branch's own additions are still checked | ``` pytest .ci/scripts/tests/test_link_check_diff_selection.py # 4 passed ``` Mutating each changed line individually: ``` inner per-file range -> .. 3 failed caught lint_file_size range -> .. 1 failed caught drop --no-color 1 failed caught file-selection range -> .. 4 passed equivalent mutant, see below ``` The file-selection range is not pinned because it cannot be: with the per-file diff at three dots, the extra files it selects produce empty diffs. Replayed against #21707's real 199-file range, both variants emit byte-identical output. It is changed for consistency, not behaviour. **Narrowing the scope must not blunt the check**, so each linter also has a positive control where the branch itself adds the bad thing: ``` lint_urls adds a dead URL -> rc=1, reports example.invalid/dead lint_xrefs adds a broken reference -> rc=1, reports sub/missing.md lint_file_size adds a 1MB+ file -> rc=1, reports feature_big.bin ``` End to end on real content: a synthetic commit on top of `main` that puts #21694's nine repaired links back, as if a PR had added them, gives `rc=1` with 9 FAIL and 5 OK against the live network. Incidentally `musl.cc` answered this time where CI saw `000`, and `pybind/cmake_example` now 404s where CI saw `301` — which is the retry-then-WARN path earning its keep. Also replayed #21729's and #21707's exact CI refs through the fixed scripts (exit 0 each), and ran all three linters plus `lintrunner` against this PR's own diff. ### Known limitations - The checkout is `head.sha`, so references still resolve against the branch tree rather than the merge result — the mirror image of the bug fixed here. Pre-existing and not worsened by this PR. - The whole-tree branch swallows a `git grep` failure and exits 0, so on a git built without PCRE the scan silently checks nothing. Reachable locally, not on the runners, which is why the nightly scan works. Pre-existing; worth a follow-up rather than widening this PR. ### Rollout This does **not** repair a currently red run. A rerun keeps the original `GITHUB_SHA`, and advancing the base alone does not fire `synchronize`, so an already-open PR picks the fix up only on its next newly triggered pull-request run — any push, or close/reopen. That is still cheaper than a content rebase: no history rewrite, no conflicts, nothing to re-review. Supersedes #21762, which fixes the same bug but leaves the checkout shallow and misses `lint_urls.sh`. Its reviewer's shallow-checkout point is exactly right: a `--depth=1` fetch writes a shallow graft even into an otherwise complete clone, after which `git merge-base` fails and `A...B` is fatal, so the two halves of this change are a pair. The regression test here is adapted from that PR. Authored with Claude Code (Claude Opus 5).
Summary
link-check / lint-urlswas diffingbase..head, so PR jobs could lint unrelated URLs introduced onmainafter the branch diverged. This narrows both URL and xref linting to lines introduced by the PR by diffing from the merge base instead.Scope
scripts/lint_urls.shto usegit diff base...headscripts/lint_xrefs.shto use the same merge-base semanticsBehavior change
Regression coverage
mainand feature histories and verifies only the feature-side changes are linted for:Test plan
Added regression coverage in
.ci/scripts/tests/test_link_check_diff_selection.pyfor bothlint_urls.shandlint_xrefs.sh.