GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — update_lib hard dependency resolver by youknowone · Pull Request #6817 · RustPython/RustPython

Conversation

@youknowone

@youknowone youknowone commented Jan 20, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

updaing datetime will triggers to update also _pydatetime

cc @moreal @ShaharNaveh

Summary by CodeRabbit

  • New Features

    • Enhanced dependency resolution for library updates, automatically detecting and processing test-related dependencies and data paths.
  • Bug Fixes

    • Improved test failure reporting with clearer error messages.
    • Fixed handling of package initialization files in test path detection.
    • Enhanced library copying to support complex package structures.
  • Tests

    • Added comprehensive test coverage for dependency resolution and library operations.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh reacted with rocket emoji
All reactions
  • 🚀 1 reaction
youknowone added 2 commits January 20, 2026 21:41
@youknowone
update_lib __init__ handling
7bd50b2
@youknowone
dependency
9b7b53c
@youknowone youknowone added the skip:ci Skip running the ci label Jan 20, 2026
@coderabbitai

coderabbitai Bot commented Jan 20, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR extends the library update toolchain with a new dependency resolution module (deps.py), improves error handling with TestRunError exception, refactors multi-file copying support via a helper function, and adjusts path resolution for __init__.py files. Comprehensive tests are added across all modified modules.

Changes

Cohort / File(s) Summary
Documentation & Configuration
\.claude/commands/upgrade-pylib\.md
Minor indentation adjustment in the STOP section for formatting alignment.
Error Handling & Test Validation
scripts/update_lib/auto_mark\.py
Introduces TestRunError exception class. Replaces capture_output with stdout=PIPE and stderr=None for selective output handling. Adds validation in _is_super_call_only to match method names. Raises TestRunError when test runs produce no results.
Multi-File Copy Support
scripts/update_lib/copy_lib\.py
Introduces private helper _copy_single() for single file/directory copies. Refactors copy_lib() to detect /Lib/ paths and copy multiple files via get_lib_paths(), with fallback to single-file mode.
Dependency Resolution (New Module)
scripts/update_lib/deps\.py
New module implementing dependency resolution with functions: get_lib_paths(), get_test_paths(), get_data_paths(), parse_test_imports(), get_test_dependencies(), and resolve_all_paths(). Includes DEPENDENCIES and TEST_DEPENDENCIES mappings for irregular library layouts and test-specific path overrides.
Import Insertion
scripts/update_lib/patch_spec\.py
Replaces unconditional assertion with guarded fallback logic: returns last import line if present, otherwise returns docstring end line, else returns 0.
Path Resolution
scripts/update_lib/path\.py
Adjusts lib_to_test_path() to use parent directory name when encountering __init__.py instead of the literal filename.
Workflow Enhancement
scripts/update_lib/quick\.py
Adds Step 1.5 to process test dependencies after initial patch via get_test_dependencies(). Introduces DEPENDENCIES-based overrides in _expand_shortcut(). Adds exception handling for TestRunError with clean error output.
Test Suite (New & Updated)
scripts/update_lib/tests/test_auto_mark\.py, test_copy_lib\.py, test_deps\.py, test_patch_spec\.py, test_path\.py, test_quick\.py
Comprehensive unit tests added: validation of _is_super_call_only, _copy_single file/directory operations, dependency resolution across all path types, import insertion fallback logic, __init__.py parent name handling, shortcut overrides, and test failure scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Remake update_lib #6796: Continuation of the same update\_lib toolchain development, modifying identical modules (auto\_mark.py, copy\_lib.py, patch\_spec.py, path.py, quick.py) and introducing the new deps.py and test infrastructure.

Poem

🐰 With paths untangled and tests that stand firm,
Dependencies mapped—our toolchain does turn!
From __init__.py secrets to copies that scale,
This rabbit hops faster, leaves no library pale. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'update_lib hard dependency resolver' accurately captures the main objective of this PR, which is to implement dependency resolution so that updating datetime triggers _pydatetime updates.
Docstring Coverage ✅ Passed Docstring coverage is 96.49% 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
  • X
  • Mastodon
  • Reddit
  • LinkedIn

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions
Auto-format: ruff format
24c1b88
@github-actions

github-actions Bot commented Jan 20, 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh
ShaharNaveh reviewed Jan 20, 2026
View reviewed changes
Comment thread scripts/update_lib/copy_lib.py
from update_lib.path import parse_lib_path

# Extract module name and cpython prefix from path
path_str = str(src_path).replace("\\", "/")

@ShaharNaveh ShaharNaveh Jan 20, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

When I look at this again, wouldn't as_posix() would also do this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@youknowone youknowone Jan 20, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

once tried, but not simply replacable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ShaharNaveh
ShaharNaveh approved these changes Jan 20, 2026
View reviewed changes

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I like this change a lot!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@youknowone
youknowone marked this pull request as ready for review January 20, 2026 14:15
coderabbitai[bot]
coderabbitai Bot reviewed Jan 20, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@scripts/update_lib/copy_lib.py`:
- Around line 65-77: The code that parses src_path via path_str and splits on
"/Lib/" mis-handles nested module paths and makes the "else" fallback
unreachable because parse_lib_path always raises; update the logic in
copy_lib.py so that when "/Lib/" is present you preserve the full after_lib (not
just its first path component) and derive the destination accordingly (e.g., use
the full nested path portion from after_lib to compute lib_path), and when
"/Lib/" is not present call parse_lib_path inside a try/except and on exception
perform an explicit direct copy using _copy_single (or consult DEPENDENCIES
mapping) instead of assuming parse_lib_path will succeed; refer to
variables/path pieces path_str, cpython_prefix, after_lib, name, and functions
parse_lib_path and _copy_single to locate the changes.

In `@scripts/update_lib/deps.py`:
- Around line 358-365: In resolve_all_paths, the loop over deps returned from
get_test_dependencies is appending dictionary keys ("hard_deps"/"data") into
result["test_deps"]; change the logic so you iterate over deps["hard_deps"]
(and, if you need test data, also extend/merge deps["data"]) instead of
iterating the deps dict itself; reference get_test_dependencies, the local
variable deps, and result["test_deps"], and add existence checks for "hard_deps"
and "data" before iterating to avoid KeyError.
🧹 Nitpick comments (2)
scripts/update_lib/tests/test_patch_spec.py (1)

325-358: Good test coverage for the three main branches.

The tests correctly validate each branch of _find_import_insert_line. Consider adding a test for a multi-line docstring to verify end_lineno handling, though this is optional since the current coverage is sufficient.

🧪 Optional: Additional test for multi-line docstring
def test_no_imports_with_multiline_docstring(self):
    """Test fallback to after multi-line docstring when no imports."""
    code = '''"""Module docstring.

This spans multiple lines.
"""

class Foo:
    pass
'''
    tree = ast.parse(code)
    line = _find_import_insert_line(tree)
    self.assertEqual(line, 4)
scripts/update_lib/quick.py (1)

108-133: Dependency migration is solid; please confirm data deps are directories.
shutil.copytree will fail if any entry in test_deps["data"] is a file. If files are possible, consider handling both cases.

Optional hardening for file vs directory data deps
-            if data_lib.exists():
-                shutil.rmtree(data_lib)
-            shutil.copytree(data_src, data_lib)
+            if data_lib.exists():
+                if data_lib.is_dir():
+                    shutil.rmtree(data_lib)
+                else:
+                    data_lib.unlink()
+            if data_src.is_dir():
+                shutil.copytree(data_src, data_lib)
+            else:
+                data_lib.parent.mkdir(parents=True, exist_ok=True)
+                shutil.copy2(data_src, data_lib)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread scripts/update_lib/copy_lib.py
Comment on lines +65 to +77
# Extract module name and cpython prefix from path
path_str = str(src_path).replace("\\", "/")
if "/Lib/" in path_str:
cpython_prefix, after_lib = path_str.split("/Lib/", 1)
# Get module name (first component, without .py)
name = after_lib.split("/")[0]
if name.endswith(".py"):
name = name[:-3]
else:
# Fallback: just copy the single file
lib_path = parse_lib_path(src_path)
_copy_single(src_path, lib_path, verbose)
return

@coderabbitai coderabbitai Bot Jan 20, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fallback path handling is currently unreachable; nested Lib paths mis-resolve module names.

When /Lib/ isn’t present, parse_lib_path will always raise, so the “fallback” can’t work. Also, deriving the name from the first component after /Lib/ mis-resolves nested paths like cpython/Lib/test/libregrtest (name becomes test), which can copy the wrong module. Consider copying nested paths directly (or reverse-mapping via DEPENDENCIES) and making the non-/Lib/ fallback explicit.

🛠️ Proposed fix (direct-copy for nested paths + explicit fallback)
     if "/Lib/" in path_str:
         cpython_prefix, after_lib = path_str.split("/Lib/", 1)
+        # If a deeper path is provided (e.g., Lib/test/libregrtest),
+        # copy it directly instead of resolving by top-level module name.
+        if "/" in after_lib:
+            lib_path = parse_lib_path(src_path)
+            _copy_single(src_path, lib_path, verbose)
+            return
         # Get module name (first component, without .py)
         name = after_lib.split("/")[0]
         if name.endswith(".py"):
             name = name[:-3]
     else:
-        # Fallback: just copy the single file
-        lib_path = parse_lib_path(src_path)
-        _copy_single(src_path, lib_path, verbose)
-        return
+        # Fallback: allow direct Lib/... paths
+        if path_str.startswith("Lib/"):
+            lib_path = pathlib.Path(path_str)
+            _copy_single(src_path, lib_path, verbose)
+            return
+        raise ValueError(f"Path must contain '/Lib/' or start with 'Lib/' (got: {src_path})")
🤖 Prompt for AI Agents
In `@scripts/update_lib/copy_lib.py` around lines 65 - 77, The code that parses
src_path via path_str and splits on "/Lib/" mis-handles nested module paths and
makes the "else" fallback unreachable because parse_lib_path always raises;
update the logic in copy_lib.py so that when "/Lib/" is present you preserve the
full after_lib (not just its first path component) and derive the destination
accordingly (e.g., use the full nested path portion from after_lib to compute
lib_path), and when "/Lib/" is not present call parse_lib_path inside a
try/except and on exception perform an explicit direct copy using _copy_single
(or consult DEPENDENCIES mapping) instead of assuming parse_lib_path will
succeed; refer to variables/path pieces path_str, cpython_prefix, after_lib,
name, and functions parse_lib_path and _copy_single to locate the changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread scripts/update_lib/deps.py
Comment on lines +358 to +365
if include_deps:
# Auto-detect test dependencies
for test_path in result["test"]:
deps = get_test_dependencies(test_path)
for dep in deps:
if dep not in result["test_deps"]:
result["test_deps"].append(dep)

@coderabbitai coderabbitai Bot Jan 20, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

resolve_all_paths appends dict keys instead of dependency paths.

Iterating over deps yields "hard_deps" / "data" strings, so test_deps becomes invalid and downstream logic will break. Iterate over deps["hard_deps"] (and merge deps["data"] if you intend to collect test data).

🛠️ Proposed fix
     if include_deps:
         # Auto-detect test dependencies
         for test_path in result["test"]:
             deps = get_test_dependencies(test_path)
-            for dep in deps:
-                if dep not in result["test_deps"]:
-                    result["test_deps"].append(dep)
+            for dep in deps["hard_deps"]:
+                if dep not in result["test_deps"]:
+                    result["test_deps"].append(dep)
+            for data_path in deps["data"]:
+                if data_path not in result["data"]:
+                    result["data"].append(data_path)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if include_deps:
# Auto-detect test dependencies
for test_path in result["test"]:
deps = get_test_dependencies(test_path)
for dep in deps:
if dep not in result["test_deps"]:
result["test_deps"].append(dep)
if include_deps:
# Auto-detect test dependencies
for test_path in result["test"]:
deps = get_test_dependencies(test_path)
for dep in deps["hard_deps"]:
if dep not in result["test_deps"]:
result["test_deps"].append(dep)
for data_path in deps["data"]:
if data_path not in result["data"]:
result["data"].append(data_path)
🤖 Prompt for AI Agents
In `@scripts/update_lib/deps.py` around lines 358 - 365, In resolve_all_paths, the
loop over deps returned from get_test_dependencies is appending dictionary keys
("hard_deps"/"data") into result["test_deps"]; change the logic so you iterate
over deps["hard_deps"] (and, if you need test data, also extend/merge
deps["data"]) instead of iterating the deps dict itself; reference
get_test_dependencies, the local variable deps, and result["test_deps"], and add
existence checks for "hard_deps" and "data" before iterating to avoid KeyError.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@youknowone
youknowone merged commit 5ef8d48 into RustPython:main Jan 20, 2026
9 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@youknowone
youknowone deleted the lib_updater branch January 20, 2026 14:52
This was referenced Jan 20, 2026
[update_lib] show deps #6821
Merged
[update_lib] todo subcommand #6823
Merged
[update_lib] show dependent tests in deps subcommand #6828
Merged
improve deps CI formatting and name resolution #6847
Merged
Update urllib and windows libraries from v3.14.2 #6858
Merged
Fix update_lib tests #6866
Merged
[update_lib] Apply test grouping to deps #6867
Merged
This was referenced Jan 30, 2026
[update_lib] Fix async func auto-mark #6903
Merged
Make auto-mark output deterministic and fix blank line leak #6957
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Feb 9, 2026
[update_lib] fix hard_deps resolution and fix commit not to miss test data #7058
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip:ci Skip running the ci

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@youknowone @ShaharNaveh
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.