◐ Shell
reader mode source ↗
Skip to content

Remake update_lib#6796

Merged
youknowone merged 5 commits into
RustPython:mainfrom
youknowone:lib_updater
Jan 20, 2026
Merged

Remake update_lib#6796
youknowone merged 5 commits into
RustPython:mainfrom
youknowone:lib_updater

Conversation

@youknowone

@youknowone youknowone commented Jan 19, 2026

Copy link
Copy Markdown
Member

cc @moreal

I am going to rewrite this tools more organized library update toolkit, and remain rooms for further automation.

scripts/update_lib will be main entrance.

lib_updater
-> scripts/update_lib patches (--from --to)
-> scripts/update_lib migrate (--quick-upgrade)

auto_mark_test
-> scripts/update_lib auto_mark

Also scripts/update_lib quick is added to upgrade a whole package at once, copy the library, remove unexected sucesss new add expectedFailure for newly added failing tests but not about regression.

The format also will be aligned to ruff format and PEP8 to fix #6701.

Keeping draft to test for a few more libraries. Please feedback.

Summary by CodeRabbit

  • Documentation

    • Updated library upgrade instructions to reflect new update_lib workflow with improved step-by-step guidance
    • Added troubleshooting section with issue reporting guidelines
  • Refactor

    • Deprecated legacy auto_mark_test.py tool
    • Introduced new update_lib utility offering enhanced test marker preservation and improved failure detection workflow

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Jan 19, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The PR replaces the monolithic auto_mark_test.py script and legacy lib_updater workflow with a new modular update_lib package featuring specialized subcommands for copying libraries, migrating tests, auto-marking failures, and orchestrating updates. Documentation is updated to reflect the new tooling.

Changes

Cohort / File(s) Summary
Documentation Update
\.claude/commands/upgrade-pylib\.md
Updated instructions to reference update_lib instead of legacy lib_updater workflow; added "Report Tool Issues First" section; revised Steps 3–5 to describe new patching, auto-marking, and marker restoration capabilities.
Legacy Script Removal
scripts/auto_mark_test\.py
Deleted entire 411-line automated test-marking script that annotated failures with @unittest.expectedFailure; functionality merged into new scripts/update_lib/auto_mark\.py.
Core Library Modules
scripts/update_lib/__init__\.py, scripts/update_lib/patch_spec\.py
New package initializer re-exporting public API; refactored patch_spec from CLI tool to library with new functions extract_patches, patches_to_json, patches_from_json; renamed internal helpers to private (underscore-prefixed).
CLI Entry Points
scripts/update_lib/__main__\.py, scripts/update_lib/auto_mark\.py, scripts/update_lib/copy_lib\.py, scripts/update_lib/migrate\.py, scripts/update_lib/patches\.py, scripts/update_lib/quick\.py
New CLI interface dispatching to subcommands; auto_mark.py provides AST-driven test result parsing, inheritance-aware method resolution, and patch generation; copy_lib, migrate, and patches handle file operations and patching; quick.py orchestrates end-to-end workflows with optional git integration.
Path Utilities
scripts/update_lib/path\.py
New module providing path transformations (lib-to-test mapping, path parsing) and test file enumeration; supports flexible path handling for both library and test directories.
Comprehensive Test Coverage
scripts/update_lib/tests/test_auto_mark\.py, test_migrate\.py, test_patch_spec\.py, test_path\.py, test_quick\.py
New test suites covering all modules with 1,500+ lines; validates parsing, AST analysis, patch application, path resolution, and integration workflows; includes edge cases and regression-safety filters.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as update_lib<br/>(quick)
    participant Migrate as migrate.py
    participant AutoMark as auto_mark.py
    participant TestRunner as run_test()
    participant FileSystem as File System

    User->>CLI: quick [path] --no-migrate=False
    CLI->>Migrate: patch_file/patch_directory
    Migrate->>FileSystem: Extract patches from Lib/test/<file>.py
    Migrate->>FileSystem: Read cpython/Lib/test/<file>.py
    Migrate->>FileSystem: Apply patches & write to Lib/test/<file>.py
    Migrate-->>CLI: Patching complete

    CLI->>AutoMark: auto_mark_file/auto_mark_directory
    AutoMark->>TestRunner: run_test(test_name)
    TestRunner->>FileSystem: Execute RustPython tests
    TestRunner-->>AutoMark: TestResult{failures, unexpected_successes}
    
    AutoMark->>AutoMark: Parse AST to extract test methods
    AutoMark->>AutoMark: Build inheritance info & resolve method defs
    AutoMark->>AutoMark: build_patches for failures
    AutoMark->>FileSystem: Remove `@expectedFailure` from passing tests
    AutoMark->>FileSystem: Apply patches to mark new failures
    AutoMark-->>CLI: Auto-marking complete (delta counts)

    CLI->>FileSystem: Optional: git commit with version tag
    CLI-->>User: Update workflow finished
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • ShaharNaveh
  • arihant2math

Poem

🐰 Hop, hop, the scripts now shine so bright,
Modular magic, organized just right!
Patches and tests in harmony play,
Old scripts retired—update_lib saves the day!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Remake update_lib' clearly and concisely summarizes the main change: a comprehensive rewrite of the update_lib toolkit from a monolithic script to a modular package structure with subcommands.
Linked Issues check ✅ Passed The PR addresses issue #6701 by reformatting the codebase to align with ruff/PEP8 conventions (double quotes), as evidenced by the final commit 'Auto-format: ruff format' and the new modular structure.
Out of Scope Changes check ✅ Passed All changes are in-scope: the update_lib package restructuring, removal of legacy auto_mark_test.py, and documentation updates in the upgrade instructions align with the stated objectives of remaking update_lib and addressing formatting issues.
Docstring Coverage ✅ Passed Docstring coverage is 90.97% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jan 19, 2026

Copy link
Copy Markdown
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • ruff format
    Please pull the latest changes before pushing again:
git pull origin lib_updater

@youknowone youknowone added the skip:ci Skip running the ci label Jan 19, 2026
@moreal

moreal commented Jan 19, 2026

Copy link
Copy Markdown
Contributor

I knew I could push directly to this branch, but I was cautious so I sent a PR: youknowone#10

@youknowone youknowone marked this pull request as ready for review January 20, 2026 02:35
@youknowone

Copy link
Copy Markdown
Member Author

I ran this for dataclasses, inspect, test_types and working all good. Let's fix other bugs one by one later

Hide details View details @youknowone youknowone merged commit f1fd1e9 into RustPython:main Jan 20, 2026
8 of 9 checks passed
@youknowone youknowone deleted the lib_updater branch January 20, 2026 02:38

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/update_lib/patch_spec.py (1)

295-302: Handle files without imports to avoid assertion crashes.
If a test file has no imports, _find_import_insert_line asserts and apply_patches fails when adding import unittest. Add a safe fallback (e.g., after module docstring or at top).

🐛 Proposed fix
-    assert last_import_line is not None
-    return last_import_line
+    if last_import_line is not None:
+        return last_import_line
+    # Fallback: insert after module docstring if present, else at top
+    if (
+        tree.body
+        and isinstance(tree.body[0], ast.Expr)
+        and isinstance(tree.body[0].value, ast.Constant)
+        and isinstance(tree.body[0].value.value, str)
+    ):
+        return tree.body[0].end_lineno or tree.body[0].lineno
+    return 0
🤖 Fix all issues with AI agents
In @.claude/commands/upgrade-pylib.md:
- Around line 15-20: The markdown violates MD007 because the unordered list
items under "Describe:" are indented by two spaces; fix by removing the
two-space indentation so the "-" list markers start at column 0 (e.g., adjust
the lines containing "- Library name" and "- The full command executed..." to
have no leading spaces), and scan the rest of .claude/commands/upgrade-pylib.md
for any other unordered list items with extra leading spaces and normalize them
to 0 indentation to satisfy the markdownlint config.

In `@scripts/update_lib/auto_mark.py`:
- Around line 227-244: The helper _is_super_call_only currently only verifies
that the body returns a super() call but doesn't ensure the delegated attribute
matches the current function, which can prune methods like def foo(self): return
super().bar(); update _is_super_call_only to verify that the ast.Attribute on
the Call has attr equal to func_node.name (i.e., ensure call.func.attr ==
func_node.name) before returning True; keep the existing checks for the AST
shapes and optionally ensure arguments/keywords match if you want stronger
equivalence, but at minimum add the attribute-name equality guard in
_is_super_call_only.

In `@scripts/update_lib/copy_lib.py`:
- Around line 45-54: For directory copies, ensure the destination parent exists
before calling shutil.copytree: before calling shutil.copytree(src_path,
lib_path) create lib_path.parent (using mkdir with parents=True, exist_ok=True)
just like the file branch does; update the block that checks src_path.is_dir()
to call lib_path.parent.mkdir(parents=True, exist_ok=True) (and keep the verbose
print) so copytree won’t raise FileNotFoundError.

In `@scripts/update_lib/path.py`:
- Around line 42-78: The lib_to_test_path function mishandles package
__init__.py names (producing test___init__); update both branches (the "/Lib/"
and "Lib/" branches in lib_to_test_path) to detect when src_path.name ==
"__init__.py" (or lib_path.stem == "__init__") and in that case set lib_name to
the parent directory's name (e.g., lib_path.parent.name or src_path.parent.name)
instead of "__init__"; keep the existing directory-first then file-fallback
logic and continue returning the constructed pathlib.Path values as before.

In `@scripts/update_lib/tests/test_migrate.py`:
- Around line 1-196: The new tests in TestPatchSingleContent, TestPatchFile, and
TestPatchDirectory add general test logic and assertions which violates the
test-file modification policy; to fix, move these checks out of a "*test*.py"
file into a non-test helper/module (e.g., create a new module like
update_lib/tests_helpers.py) or replace the changes so that the only
modifications inside the test files are adding/removing
`@unittest.expectedFailure` with TODO comments (references: patch_single_content,
patch_file, patch_directory, and the test classes
TestPatchSingleContent/TestPatchFile/TestPatchDirectory), then update the
import/usage accordingly or request a policy exception.
🧹 Nitpick comments (6)
scripts/update_lib/patches.py (1)

24-30: Use explicit UTF‑8 encoding when writing output.
Locale defaults can vary; explicit UTF‑8 keeps patch output deterministic across environments.

♻️ Proposed tweak
-    with open(dest, "w") as fd:
+    with open(dest, "w", encoding="utf-8") as fd:
scripts/update_lib/path.py (1)

88-99: Consider adding type hint for consistency.

The function works correctly for the documented use cases. Consider adding a type hint for the parameter to match the style of other functions in this module:

Suggested improvement
-def test_name_from_path(test_path: pathlib.Path) -> str:
+def test_name_from_path(test_path: pathlib.Path | str) -> str:
scripts/update_lib/quick.py (4)

43-64: Consider specifying encoding for file reads.

Lines 59 and 63 use read_text() without explicit encoding. While Python 3 defaults to UTF-8 on most systems, explicitly specifying encoding="utf-8" improves cross-platform consistency and makes the intent clear.

Suggested improvement
     if lib_path.is_file():
-        return extract_test_methods(lib_path.read_text())
+        return extract_test_methods(lib_path.read_text(encoding="utf-8"))
     else:
         result = {}
         for lib_file in get_test_files(lib_path):
-            result[lib_file.resolve()] = extract_test_methods(lib_file.read_text())
+            result[lib_file.resolve()] = extract_test_methods(lib_file.read_text(encoding="utf-8"))
         return result

166-225: Consider using a placeholder email address.

The author string "CPython Developers <>" uses an empty email which may cause issues with certain git configurations or CI systems that validate commit metadata. Consider using a no-reply or placeholder email.

Suggested improvement
     subprocess.run(
         [
             "git",
             "commit",
             "--author",
-            "CPython Developers <>",
+            "CPython Developers <noreply@python.org>",
             "-m",
             message,
         ],
         check=True,
     )

228-260: Minor: Windows path separator not handled in shortcut check.

Line 237 only checks for "/" but not "\\", which could cause issues on Windows where paths might use backslashes. Consider normalizing first:

Suggested improvement
 def _expand_shortcut(path: pathlib.Path) -> pathlib.Path:
     # Only expand if it's a simple name (no path separators) and doesn't exist
-    if "/" in str(path) or path.exists():
+    path_str = str(path).replace("\\", "/")
+    if "/" in path_str or path.exists():
         return path

263-365: Consider handling subprocess.CalledProcessError for git operations.

The main() function catches ValueError and FileNotFoundError but not subprocess.CalledProcessError. If git commands fail (e.g., get_cpython_version or git_commit), the error will result in an uncaught exception with a full traceback instead of a clean error message.

Suggested improvement
+    except subprocess.CalledProcessError as e:
+        print(f"Git command failed: {e}", file=sys.stderr)
+        return 1
     except ValueError as e:
         print(f"Error: {e}", file=sys.stderr)
         return 1

Also add at the top of the file:

 import argparse
 import pathlib
+import subprocess
 import sys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip:ci Skip running the ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lib_updater formatting is not aligned to ruff format

3 participants