Skip to content

fix(tools): detect exports already present on main - #8970

Open
xdCloudy wants to merge 2 commits into
boykopovar:mainfrom
xdCloudy:feat/pr-overlap-main-export-check
Open

xdCloudy wants to merge 2 commits into
boykopovar:mainfrom
xdCloudy:feat/pr-overlap-main-export-check

Conversation

@xdCloudy

@xdCloudy xdCloudy commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What

Extends the PR overlap checker to compare each PR's newly added export NIDs against the target base branch. The comparison recognizes APS5_VABI definitions, APS5_ASM_FUNCTION and APS5_SIGNAL_ASM_FUNCTION exports, and literal NIDs from APS5_EXPORT. It reports matching export names already present on main, including when a plain-name export maps to an implementation with a _nid_postfix suffix. NIDs removed by the same diff are subtracted so in-place replacements are not reported as duplicates.

This is the follow-up suggested by Dean in #8961. It is based on current upstream main and does not depend on #8961.

Tested

  • python -m unittest discover -s tools/tests -p 'test_pr_overlap*.py' — 69 tests passed; 4 platform-specific tests skipped.
  • python -I tools/check_conventions.py --base upstream/main --head HEAD — passed.
  • git diff --check — passed.
  • python tools/pr_overlap.py --repo boykopovar/AnyPS5 --base main --branch HEAD — no overlap with currently open PRs.
  • Scanned current base main: 9,128 distinct export NIDs recognized.
  • Full project build not run; this change only affects the Python overlap-checking tool.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@xdCloudy
xdCloudy force-pushed the feat/pr-overlap-main-export-check branch from 5797c13 to c51995e Compare October 21, 2026 07:55
@xdCloudy xdCloudy changed the title tools: detect exports already present on main fix(tools): detect exports already present on main Oct 21, 2026
@xdCloudy

Copy link
Copy Markdown
Contributor Author

Credit for the idea: @oneandonlydean

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@xdCloudy thanks, this catches the right thing. I ran source_added_exports and base_exports from this head against current main on a few PRs. It flags exactly the duplicates found by hand earlier: the plain-name stubs over _nid_postfix implementations on the earlier heads of #8944 and #8951, all four in #8946, and a plain getcontext stub over libkernel's APS5_ASM_FUNCTION one. Clean stub PRs come back empty.

One problem before it's useful: a PR that replaces an existing stub in place is flagged too, because "added" counts every + definition line and nothing subtracts what the same diff removes. #8805 replaces 18 throwing stubs with implementations and gets all 18 reported as "already on main". The same happens whenever a PR edits an existing export's signature line. Since most libs PRs look like that, the column would mostly be noise.

Could you drop NIDs whose definition the same diff removes (run source_exports over the - lines as well), or count definitions per NID on the merged tree and report only those defined more than once? A test with a stub replaced in place would pin it.

@xdCloudy
xdCloudy force-pushed the feat/pr-overlap-main-export-check branch from c51995e to f23c960 Compare October 21, 2026 08:02
@xdCloudy

Copy link
Copy Markdown
Contributor Author

Fixed in f23c960. The checker now derives export NIDs from both added and removed lines and subtracts NIDs removed by the same diff before comparing with base main, so replacing an existing export in place no longer reports it as a duplicate. Added a regression test for a _nid_postfix stub replaced by its plain-name implementation. The overlap test suite passes: 69 tests, 4 platform-specific skips; the conventions check passes locally.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 988 tools/pr_overlap.py

@xXPBSXx

xXPBSXx commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Linux data point for f23c960f38: WSL2 Ubuntu 24.04 (a virtualised kernel, not bare-metal Linux), Python 3.12, PR head alone (not merged onto main), python3 -I -m unittest discover -s tools/tests -p test_pr_overlap_exports.py.

  • With the PR: 10 tests OK.
  • With tools/pr_overlap.py restored to the merge-base (the PR's tests kept): 1 failure and 5 errors (e.g. test_report_includes_exports_already_on_main, test_added_raw_export_matches_base_definition).
    Not tested: macOS, Windows, a merge onto current main.
    AI-assisted (Claude Code).

@harisibrar1 harisibrar1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verified on Windows 11, Python 3.14, merged onto main 80565c5:

  • python -I -m unittest discover -s tools/tests -p "test_pr_overlap*.py": 71 tests OK, 4 skipped (69 OK at the head alone). check_conventions.py against the merge base passes.
  • Negative control with main's tools/pr_overlap.py: the new tests give 1 failure and 5 errors, the same as the Linux run above.
  • The in-place replacement case raised earlier is handled by subtracting source_changed_exports(diff, "-") in f23c960.
  • base_exports takes about 19 s here and runs a single git archive, so no long command line on Windows.

Two problems with the new column:

  1. The finding is stored under key 0 (rows[number][0] = {"added": set(), "main_exports": names}, tools/pr_overlap.py:344), and report() turns every key into a PR link (:285). pr_overlap.report({0: {"added": set(), "main_exports": {"sceKernelRead"}}}, blob) renders | [PR 0](https://www.github.com/boykopovar/AnyPS5/pull/0) | | | sceKernelRead | | under "Other open pull requests touch the same code". test_report_includes_exports_already_on_main uses key 9, so it does not catch this. Could the names already on main get their own section instead of a PR row, with a test that goes through the __main__ row construction?

  2. Matching is by NID alone, across libraries. Running the PR's own source_exports over core/libs/prx on main finds 44 NIDs exported by more than one library, e.g. 0hlfW1O4Aa4 (libc localeconv, libSceAmpr sceAmprLocaleconv), -vXEQdRADLI (strtok_s / sceAmprStrtokS), 2UFh+YKfuzk (ctime_s / sceAmprCtimeS). A PR adding a correct export of such a NID to a second library would be listed as "already on main". Keying both sides by (core/libs/prx/<library>, NID) would keep the same-library duplicates and drop these.

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

@xdCloudy
xdCloudy force-pushed the feat/pr-overlap-main-export-check branch 2 times, most recently from f33836e to d9aa2c2 Compare October 21, 2026 16:58
@xdCloudy

Copy link
Copy Markdown
Contributor Author

Following up on the review:

  • @harisibrar1: both requested changes are in d9aa2c2889. “Already on main” matches now render in a dedicated section with no synthetic PR 0 row, and export keys use (core/libs/prx/<library>, NID) on both the PR diff and base tree. Added tests cover report construction and cross-library NID matches.
  • @oneandonlydean: the same-diff replacement subtraction remains in place; the regression test continues to cover replacing a _nid_postfix stub with its plain-name definition.
  • @xXPBSXx: thanks for the Linux test data. I rebased on current upstream main and reran the overlap suite on Windows: 72 tests passed, 4 platform-specific skips.
  • I manually checked ci: log workflow results and write GitHub job summaries #988’s shared tools/pr_overlap.py hunk. It adds end-of-run workflow summary logging after report generation; it does not change export matching or report construction. The final overlap scan reports only ci: log workflow results and write GitHub job summaries #988 as uncomparable because its branch conflicts with base, so that result is recorded here as a manual review.

check_conventions.py and git diff --check also pass. No merge performed.

@harisibrar1 harisibrar1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked d9aa2c2 merged onto main 950388b: names already on main now render in their own "Exports introduced here that are already present in main" section with no PR 0 row, base_exports keys exports by library via library_for_path, and test_pr_overlap*.py passes (4 platform skips).

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

@xdCloudy

Copy link
Copy Markdown
Contributor Author

Thanks for rechecking the updated head against main and confirming the dedicated main-export section, per-library NID matching, and the overlap test results. I appreciate the approval and the careful follow-up.

@xdCloudy

Copy link
Copy Markdown
Contributor Author

Relevant: GitHub could not compare #988 because its branch conflicts with base. I’ll manually review the shared tools/pr_overlap.py changes and update this PR if coordination is needed. This warning does not itself identify an export overlap.

@xdCloudy

Copy link
Copy Markdown
Contributor Author

The Ubuntu relinker check on this PR was cancelled. I tried to rerun it, but GitHub requires repository admin rights. Locally, the current head passes the merge-tree check against current main, python -I -m unittest discover -s tools/tests -p test_pr_overlap*.py (72 passed, 4 skipped), python -I tools/check_conventions.py --base upstream/main, and git diff --check. Could someone with the required permission rerun the cancelled hosted job?

@xdCloudy

Copy link
Copy Markdown
Contributor Author

Follow-up on the overlap bot warning: I reviewed #988's exact tools/pr_overlap.py hunk. It only adds end-of-run workflow summary logging after write_reports; it does not change export extraction, matching, or report construction. I found no functional overlap with this PR. The warning is relevant as a shared-file/manual-review notice, not as a duplicate-export finding. This is the completed result of my earlier manual check.

@xdCloudy
xdCloudy force-pushed the feat/pr-overlap-main-export-check branch from d9aa2c2 to 9071d69 Compare October 22, 2026 06:15

This branch has not been deployed

No deployments
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.

4 participants