Skip to content

Conversation

@niechen
Copy link
Contributor

@niechen niechen commented Jun 4, 2025

…lity

The fcntl module is not available on Windows, and its direct import was causing ImportError when mcpm share was invoked or its module (src/mcpm/commands/share.py) was loaded on Windows systems.

This commit makes the import and usage of fcntl within the make_non_blocking function conditional on os.name == 'posix'. On non-POSIX systems (like Windows), the function will now do nothing.

The existing code for reading from subprocess pipes in mcpm share utilizes select.select() with timeouts and handles IOError/OSError exceptions. This setup is expected to provide sufficient non-blocking behavior for reading subprocess output on Windows, even without an explicit fcntl call to set O_NONBLOCK.

Note: Full dynamic testing of mcpm share on a Windows-like environment was not possible due to a Python version mismatch (environment: 3.10.17, project requires: >=3.11) in the available testing sandbox. This change addresses the direct ImportError.

…lity

The `fcntl` module is not available on Windows, and its direct import
was causing `ImportError` when `mcpm share` was invoked or its module
(src/mcpm/commands/share.py) was loaded on Windows systems.

This commit makes the import and usage of `fcntl` within the
`make_non_blocking` function conditional on `os.name == 'posix'`.
On non-POSIX systems (like Windows), the function will now do nothing.

The existing code for reading from subprocess pipes in `mcpm share`
utilizes `select.select()` with timeouts and handles `IOError`/`OSError`
exceptions. This setup is expected to provide sufficient non-blocking
behavior for reading subprocess output on Windows, even without an
explicit `fcntl` call to set `O_NONBLOCK`.

Note: Full dynamic testing of `mcpm share` on a Windows-like environment
was not possible due to a Python version mismatch (environment: 3.10.17,
project requires: >=3.11) in the available testing sandbox. This change
addresses the direct `ImportError`.
Copilot AI review requested due to automatic review settings June 4, 2025 20:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adjusts the make_non_blocking function to avoid importing fcntl on non-POSIX systems, preventing ImportError on Windows while preserving existing subprocess-read behavior.

  • Wraps fcntl import and usage in an os.name == 'posix' check.
  • Adds comments explaining fallback behavior on Windows.
Comments suppressed due to low confidence (2)

src/mcpm/commands/share.py:38

  • Add a test case simulating a non-POSIX (Windows) environment to verify that make_non_blocking does not raise ImportError and that subprocess reading still behaves as expected.
# On other platforms (e.g., Windows), we rely on the behavior of select()

src/mcpm/commands/share.py:32

  • Ensure the os module is imported in this file to prevent a NameError when referencing os.name and os.O_NONBLOCK.
if os.name == 'posix':

@niechen niechen merged commit 6c9d470 into main Jun 5, 2025
6 checks passed
@niechen niechen deleted the fix/fcntl-windows-share branch June 5, 2025 03:47
@mcpm-semantic-release
Copy link

🎉 This PR is included in version 1.13.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants