GeezerVison

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

GeezerVison is showing github.com — nt is_dir,is_file,listmount,listvolume by youknowone · Pull Request #6373 · RustPython/RustPython

Conversation

youknowone commented Dec 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Summary by CodeRabbit

Release Notes

New Features
- Volume enumeration functionality now available for Windows systems
- Mount path enumeration functionality now available for Windows systems
Bug Fixes
- Environment variable length validation on Windows to prevent overflow
- Improved directory detection with enhanced fallback behavior on Windows systems

_{✏️ 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.

All reactions

coderabbitai Bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Walkthrough

These changes introduce Windows-specific functionality: a new maximum environment variable constant, two volume enumeration functions for the nt module (listvolumes and listmounts), environment variable length validation in the os module, and refactored DirEntry metadata handling with Windows fallback behavior.

Changes

Cohort / File(s)	Summary
Windows Constants `crates/common/src/windows.rs`	Added public constant `_MAX_ENV` with value `32767` representing the Windows CRT maximum environment variable size.
nt Module Volume Functions `crates/vm/src/stdlib/nt.rs`	Introduced `listvolumes()` to enumerate volume names using Windows API (`FindFirstVolumeW`/`FindNextVolumeW`), and `listmounts(volume)` to resolve mount paths for a volume using `GetVolumePathNamesForVolumeNameW` with dynamic buffer sizing. Both return Python lists and handle Windows I/O errors.
os Module Enhancements `crates/vm/src/stdlib/os.rs`	Added Windows-specific environment variable length validation in `putenv()` and `unsetenv()` against `_MAX_ENV`. Refactored `DirEntry.is_dir()` and `is_file()` to directly use metadata queries with Windows-specific `NotFound` fallback using cached `file_type` data.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

crates/vm/src/stdlib/nt.rs: Windows API integration and buffer handling logic in listmounts() requires careful verification of error paths and null-separated string parsing.
crates/vm/src/stdlib/os.rs: Platform-specific fallback behavior in DirEntry methods needs validation that Windows NotFound errors are handled correctly and cached data is reliable; env validation boundary conditions should be verified.
Cross-file consistency: Verify that _MAX_ENV constant usage in os.rs aligns with Windows CRT specifications.

Possibly related PRs

Windows execv, spawnv, wait #6350: Modifies the Windows nt module and environment-related handling, overlapping with volume/environment changes in this PR.
fix scandir/lstat for windows #6357: Refactors DirEntry metadata handling and adds cached lstat data, directly overlapping with the DirEntry.is_dir() and is_file() refactoring in this PR.

Poem

🐰 Hopping through Windows, volumes we find,
Mount paths and metadata, cleverly aligned,
Environment checks, no overflow today,
DirEntry now falters the NotFound way! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title references specific function names (is_dir, is_file, listmount, listvolume) that are implemented in the changeset, but uses unclear abbreviations and lacks context about what was actually changed.	Rewrite the title to be more descriptive, such as 'Add Windows volume/mount listing and improve directory metadata handling' or 'Implement nt.listvolumes/listmounts and fix is_dir/is_file on Windows'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.}

All reactions

Sorry, something went wrong.

Uh oh!

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

youknowone added 3 commits

December 9, 2025 19:24


          is_dir/is_file for windows

d82577a


          listmount/listvolume

2e038cf


          check_env_var_len

2dab1f6

youknowone force-pushed the more-windows branch from ac19e43 to 2dab1f6 Compare

December 9, 2025 10:24

youknowone changed the title ~~nt is_dir,is_file,listmount,listvolume,terminalsize~~ nt is_dir,is_file,listmount,listvolume

Dec 9, 2025

youknowone marked this pull request as ready for review

December 9, 2025 10:24

coderabbitai Bot reviewed

Dec 9, 2025

View reviewed changes

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: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cc534d2 and 2dab1f6.

⛔ Files ignored due to path filters (1)

Lib/test/test_os.py is excluded by !Lib/**

📒 Files selected for processing (3)

crates/common/src/windows.rs (1 hunks)
crates/vm/src/stdlib/nt.rs (1 hunks)
crates/vm/src/stdlib/os.rs (5 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/common/src/windows.rs
crates/vm/src/stdlib/nt.rs
crates/vm/src/stdlib/os.rs

🧠 Learnings (1)

📚 Learning: 2025-06-28T16:31:03.991Z

Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.

Applied to files:

crates/vm/src/stdlib/os.rs

🧬 Code graph analysis (1)

crates/vm/src/stdlib/nt.rs (1)

crates/vm/src/stdlib/os.rs (1)

errno_err (39-41)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Ensure compilation on various targets
GitHub Check: Check Rust code with clippy

🔇 Additional comments (6)

crates/vm/src/stdlib/nt.rs (2)

685-717: LGTM! Volume enumeration implementation looks correct.

The handle lifecycle is properly managed: FindVolumeClose is called on any FindNextVolumeW failure, covering both normal termination (ERROR_NO_MORE_FILES) and error conditions. The buffer size of MAX_PATH + 1 is appropriate for volume GUIDs.

Minor observation: from_utf16_lossy silently replaces invalid sequences. This is acceptable for volume names which should always be valid UTF-16.

719-764: LGTM! Mount path enumeration handles dynamic buffer sizing correctly.

The retry loop for ERROR_MORE_DATA with buffer resizing follows the standard Windows pattern. The null-separated string parsing with double-null termination detection (lines 757-758) is correct.

crates/common/src/windows.rs (1)

6-8: LGTM!

The constant value 32767 correctly matches the Windows CRT _MAX_ENV definition, and the doc comment clearly identifies the source.

crates/vm/src/stdlib/os.rs (3)

431-443: LGTM! Windows environment variable length validation.

The helper function correctly validates against _MAX_ENV and provides a clear error message.

529-546: LGTM! Windows fallback for is_dir handles race conditions.

The fallback to cached file_type when the file is removed between directory enumeration and the is_dir call is a sensible approach for Windows. This matches CPython's behavior of using directory entry metadata when the file no longer exists.

548-565: LGTM! Consistent pattern with is_dir.

The is_file refactoring mirrors the is_dir implementation with the same Windows-specific fallback to cached file_type on NotFound.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

crates/vm/src/stdlib/os.rs

Comment on lines +483 to +485

		// For unsetenv, size is key + '=' (no value, just clearing)
		#[cfg(windows)]
		check_env_var_len(key.len() + 1, vm)?;

coderabbitai Bot Dec 9, 2025

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 | 🟡 Minor

Potential off-by-one: missing null terminator in size calculation.

The comment states "size is key + '='" but Windows _MAX_ENV includes the null terminator. For unsetenv, Windows internally sets "KEY=\0", so the size should be key.len() + 2 (key + '=' + '\0'), not key.len() + 1.

         // For unsetenv, size is key + '=' (no value, just clearing)
         #[cfg(windows)]
-        check_env_var_len(key.len() + 1, vm)?;
+        check_env_var_len(key.len() + 2, vm)?;  // key + '=' + '\0'

📝 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

	// For unsetenv, size is key + '=' (no value, just clearing)
	#[cfg(windows)]
	check_env_var_len(key.len() + 1, vm)?;
	// For unsetenv, size is key + '=' (no value, just clearing)
	#[cfg(windows)]
	check_env_var_len(key.len() + 2, vm)?; // key + '=' + '\0'

🤖 Prompt for AI Agents

In crates/vm/src/stdlib/os.rs around lines 483 to 485, the size computed for
unsetenv on Windows omits the null terminator; change the Windows branch to pass
key.len() + 2 (key + '=' + '\0') into check_env_var_len instead of key.len() + 1
so the length accounts for the terminating NUL when validating against _MAX_ENV.

Sorry, something went wrong.

Uh oh!

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

All reactions

youknowone merged commit a99164f into RustPython:main

Dec 9, 2025

13 checks passed

Uh oh!

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

youknowone deleted the more-windows branch

December 9, 2025 11:08

This was referenced Dec 9, 2025

nt.terminalsize #6374

Merged

new_last_{os,errno}_error #6381

Merged

test_os, test_io on windows #6379

Merged

youknowone mentioned this pull request

Dec 24, 2025

Implement nt.listvolumes #5730

Closed

coderabbitai Bot mentioned this pull request

Feb 3, 2026

Update os from v3.14.2 and fix nt,posix,_os #6958

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

1 participant

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 01402503 — 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.

GeezerVison

Conversation

youknowone commented Dec 9, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

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

coderabbitai Bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

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