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.
| if os.getsid(os.getpid()) != os.getpid(): | ||
| os.setsid() | ||
| if os.fork() != 0: | ||
| if hasattr(os, "fork") and os.fork() != 0: |
There was a problem hiding this comment.
📍 src/debugpy/adapter/main.py:38
This PR adds a hasattr(os, "fork") probe but leaves the sibling syscalls os.getsid/os.setsid (a few lines up in the same if os.name == "posix": block) gated only on os.name == "posix". A forkless-POSIX runtime that also lacks setsid would raise AttributeError/OSError here and crash before reaching the new fork guard — defeating the PR's goal. Please confirm GraalPy exposes setsid/getsid, or guard with hasattr(os, "setsid") / wrap in try/except (AttributeError, OSError).
There was a problem hiding this comment.
yes, GraalPy exposes those functions
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
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). |
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/debugpy/launcher/debuggee.py:L36. 📍 src/debugpy/launcher/debuggee.py:33 This file detects the fork-less runtime by identity ( |
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/debugpy/launcher/debuggee.py:L36. 📍 src/debugpy/launcher/debuggee.py:31 The original |
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/debugpy/launcher/debuggee.py:L36. 📍 src/debugpy/launcher/debuggee.py:33
|
|
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:L55. 📍 src/debugpy/server/api.py:49 This |
|
Looks good overall and the approach is reasonable. The main theme of the feedback: the fork-less runtime is detected two different ways ( |
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Fixes
debugpycompatibility with GraalPy, an alternative Python implementation.