fix(tools): guard Unix-only imports in terminal package for Windows#2096
fix(tools): guard Unix-only imports in terminal package for Windows#2096neubig merged 3 commits intoOpenHands:mainfrom
Conversation
WolffM
force-pushed
the
fix/windows-fcntl-import-crash
branch
2 times, most recently
from
5846231 to
67d176c
Compare
…#12781) The terminal package eagerly imported SubprocessTerminal and TmuxTerminal in __init__.py, which triggered `import fcntl` on Windows and crashed before factory.py could raise NotImplementedError. Guard the imports behind platform.system() checks so the package loads cleanly on Windows. Co-Authored-By: Rb <rubenwolff@gmail.com>
WolffM
force-pushed
the
fix/windows-fcntl-import-crash
branch
from
67d176c to
6f2b723
Compare
|
tested it against my fork of openhands |
There was a problem hiding this comment.
Thank you for the PR!
Please let me note that we have another here:
and in that line there's a bit of an alternative way to do this, separating Windows a bit better within the codebase.
The fix in your PR is normal but unfortunately, there is more to Windows support than not crashing on imports. That actually poses a problem for us, on how to support Windows, and I admit I heavily prefer a more modular approach if we can.
Thanks for the quick reply! I was thinking this PR could serve as a short-term fix until the Windows terminal backend (#1012) is ready. I don't think it conflicts with that work. Right now openhands serve crashes immediately on Windows with an opaque fcntl import error, this fix lets the package actually load so the existing error handling in factory.py can kick in properly. Not sure how many people are hitting this right now, but it would unblock them. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @WolffM, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
On second thought, let's get this in, thanks for the contribution!
|
This also fixes Linear ticket ALL-5247. Fixes ALL-5247 |
…penHands#2096) Cherry-pick from upstream caf1b0b
4 participants
Summary
Fixes OpenHands/OpenHands#12781
SubprocessTerminalandTmuxTerminalimports interminal/terminal/__init__.pybehindplatform.system() != "Windows"so the package loads cleanly on Windowsraise ImportErrorguard insubprocess_terminal.pybefore the Unix-onlyfcntl/pty/selectimports, replacing the opaqueModuleNotFoundErrorFixes the crash where
import openhands.tools.terminalimmediately fails on Windows withModuleNotFoundError: No module named 'fcntl'beforefactory.pycan raise itsNotImplementedError("Windows is not supported yet").Context
The import chain was:
factory.pyalready had platform awareness (raisesNotImplementedErroron Windows), but it never got a chance to run because__init__.pyeagerly imported the Unix-only backends at module level.