Skip to content

test(libkernel): run nid_patcher_symbol_versions on Windows - #8717

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
xXPBSXx:test/nid-patcher-symbol-versions-windows
Oct 24, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
xXPBSXx:test/nid-patcher-symbol-versions-windows

Conversation

@xXPBSXx

@xXPBSXx xXPBSXx commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What

nid_patcher_symbol_versions was registered inside the if(Python3_Interpreter_FOUND AND NOT WIN32) block of core/libs/prx/libkernel/CMakeLists.txt that also adds nid_patcher_dynamic_strings, so it never ran on Windows. test_symbol_versions.py only writes a synthetic ELF and runs nid_patcher on it (no host loader involved); only test_dynamic_strings.py reads the ELF fixture module (guest_module_fixture), which is a PE DLL on Windows. The -Wl,--no-as-needed fixture option and the dynamic strings test stay non-Windows; the symbol versions test is now registered on every host.

git merge-tree against upstream/main is clean. pr_overlap was not run.

Tested

Windows 10 (19045), MinGW-w64 GCC 15.2.0, Release:

  • ctest -N -R "^nid_patcher_(symbol_versions|dynamic_strings)$" lists exactly nid_patcher_symbol_versions (the dynamic strings test stays unregistered on Windows).
  • Before (upstream/main): not registered on Windows (NOT WIN32). After: nid_patcher_symbol_versions passes 3 of 3 runs (5.5-6.5 s); run by hand it prints NID patcher symbol version tests passed. This is coverage only, so there is no red state without the change.
  • python tools/check_conventions.py --base upstream/main: passes.
  • Not tested: Linux/macOS (the test was registered there before and still is), a console.

Rebased onto main 1e20650e84 (head 2264b7bbca; main had moved find_package(Python3 ...) up in core/libs/prx/libkernel/CMakeLists.txt, so the Python3_Interpreter_FOUND block was merged again, with the same lines changed as before).

  • Windows 10 19045, MinGW-w64 GCC 15.2.0, Release: CMake configures with no error, nid_patcher_symbol_versions is registered on Windows and ctest -R '^nid_patcher_symbol_versions$' passes 3 of 3 runs. No red run is possible: on main the test is compiled out on Windows (inside NOT WIN32).
  • Linux (WSL2 Ubuntu 24.04, virtualised kernel, not bare-metal), same head and base: CMake configures with no error; nid_patcher_macho, nid_patcher_native_paths, nid_patcher_dynamic_strings (still registered, fixture linked with --no-as-needed) and nid_patcher_symbol_versions pass, 4 of 4.
  • python tools/check_conventions.py --base upstream/main: no finding. Not tested: macOS.

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; no silent stubs added
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • AI-assisted: yes

AI-assisted (Claude Code).

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 20, 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 4563 core/libs/prx/libkernel/CMakeLists.txt
PR 5021 core/libs/prx/libkernel/CMakeLists.txt
PR 6830 core/libs/prx/libkernel/CMakeLists.txt
PR 6936 core/libs/prx/libkernel/CMakeLists.txt
PR 7090 core/libs/prx/libkernel/CMakeLists.txt
PR 7192 core/libs/prx/libkernel/CMakeLists.txt
PR 7201 core/libs/prx/libkernel/CMakeLists.txt
PR 9156 core/libs/prx/libkernel/CMakeLists.txt
PR 9187 core/libs/prx/libkernel/CMakeLists.txt
PR 9263 core/libs/prx/libkernel/CMakeLists.txt
PR 9284 core/libs/prx/libkernel/CMakeLists.txt
PR 9293 core/libs/prx/libkernel/CMakeLists.txt
PR 9297 core/libs/prx/libkernel/CMakeLists.txt
PR 9302 core/libs/prx/libkernel/CMakeLists.txt
PR 9557 core/libs/prx/libkernel/CMakeLists.txt
PR 9561 core/libs/prx/libkernel/CMakeLists.txt
PR 9566 core/libs/prx/libkernel/CMakeLists.txt
PR 9575 core/libs/prx/libkernel/CMakeLists.txt
PR 9669 core/libs/prx/libkernel/CMakeLists.txt
PR 9693 core/libs/prx/libkernel/CMakeLists.txt
PR 9947 core/libs/prx/libkernel/CMakeLists.txt

@MarcZX10

Copy link
Copy Markdown
Contributor

The test registration change looks functionally compatible with the same-file edits in #8658 (macOS dynamic-strings linker flag) and #6586 (Linux runtime-lifecycle test registration). The overlap bot is still flagging core/libs/prx/libkernel/CMakeLists.txt, and this PR notes that pr_overlap was not run. Could you record the exact merge-tree/combined CMake check for these heads, or note the intended rebase/order? In particular, the symbol-version test should remain registered on Windows while dynamic-strings stays non-Windows and the --no-as-needed flag stays off macOS.

@MarcZX10

Copy link
Copy Markdown
Contributor

I checked the current CMake hunks in #6586 and #8658 against this PR. #6586 moves the Python discovery earlier but leaves Python3_Interpreter_FOUND available in this block; #8658 nests the --no-as-needed link option under NOT APPLE; this PR then keeps nid_patcher_dynamic_strings under NOT WIN32 while registering nid_patcher_symbol_versions for every host with Python. The combined conditions preserve the intended Windows/macOS behavior, and the hunks are compatible by inspection. I have not run a combined CMake configure/build; hosted build jobs are skipped on this PR.

@xXPBSXx

xXPBSXx commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking the CMake conditions. I ran git merge-tree of this head against the heads of #8658 and #6586 now: each pair has one textual conflict in core/libs/prx/libkernel/CMakeLists.txt (the same block), while #8658 and #6586 do not conflict with each other. So your reading of the conditions matches, but the hunks do not merge without a manual step. This change is small (6 added, 4 removed lines), so I will rebase it onto whichever of the two lands first, keeping nid_patcher_symbol_versions registered for every host with Python, nid_patcher_dynamic_strings under NOT WIN32 and the --no-as-needed option off macOS. I have not run a combined CMake configure or build of the three heads.

AI-assisted (Claude Code).

@JJenkx

JJenkx commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Independent Windows run, as data only. Machine: Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Release, BUILD_TESTING=ON, Python 3.15. #8654 (c318c70), #8717 (ba9e424) and #9000 (78bfaba) were merged together onto main 830ccd1, without conflicts, and each test was run 3 times. nid_patcher_symbol_versions passes 3 of 3.

AI-assisted (Claude Code).

nid_patcher_symbol_versions was registered inside the block that also adds
nid_patcher_dynamic_strings, so it was skipped on Windows although it only
runs nid_patcher on a synthetic ELF written by the script. Only the
dynamic strings test reads the ELF fixture module, which is a PE DLL on
Windows. Register the symbol versions test on every host.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 24, 2026
@xXPBSXx
xXPBSXx force-pushed the test/nid-patcher-symbol-versions-windows branch from ba9e424 to 2264b7b Compare October 24, 2026 07:03
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 24, 2026
@oneandonlydean
oneandonlydean merged commit 2e0e193 into boykopovar:main Oct 24, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants