Fix GraalPy compatibility#2040
Conversation
|
@steve-s please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/debugpy/server/api.py:L233. 📍 src/debugpy/server/api.py:222 |
| # GraalPy does not support running code between fork and exec | ||
| # GraalPy does not support running code between fork and exec, but supports the | ||
| # process_group argument for Popen | ||
| if sys.platform != "win32" and sys.implementation.name == "graalpy": |
There was a problem hiding this comment.
📍 src/debugpy/launcher/debuggee.py:62
The process_group keyword for subprocess.Popen was only added in Python 3.11. A GraalPy build targeting a pre-3.11 stdlib (e.g. GraalPy 23.x → 3.10) will raise TypeError: got an unexpected keyword argument 'process_group', which is caught and surfaced as "Couldn't spawn debuggee" — making launch fail entirely rather than degrading. Guard it, e.g. if ... graalpy and sys.version_info >= (3, 11): kwargs.update(process_group=0), or wrap in try/except TypeError.
There was a problem hiding this comment.
This would also require adding another branch to kill so that we use os.kill and not os.killpg for older GraalPy versions. Moreover, support for current and older GraalPy releases depends on fabioz/PyDev.Debugger#325, and unless that's merged and propagated to debugpy I think it's better to keep the changes in this file simple - it will work on GraalPy master (and some future release) for which fixes in fabioz/PyDev.Debugger#325 aren't necessary.
|
Solid GraalPy compatibility work overall. The main blocker is the |
|
Thank you for the review! I addressed the blocker in the last commit. I will have to figure out CLA with my company. |
|
Small, well-scoped GraalPy compatibility fix. Note: the api.py daemonize change correctly routes GraalPy into the plain |
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if os.getsid(os.getpid()) != os.getpid(): | ||
| os.setsid() | ||
| if os.fork() != 0: | ||
| if _CAN_DAEMONIZE and os.fork() != 0: |
There was a problem hiding this comment.
The extra variable outside of the if statement seems like overkill. Why not just have this:
if hasattr(os, "fork") and os.fork() != 0:
| env=python_env, | ||
| ) | ||
| if os.name == "posix": | ||
| if _CAN_DAEMONIZE: |
There was a problem hiding this comment.
Same thing here. It would be
if os.name == "posix" and hasattr(os, "fork")
Makes for fewer code changes.
Fixes
debugpycompatibility with GraalPy, an alternative Python implementation.