Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-133490: Fix syntax highlighting for remote PDB#133494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
pablogsal merged 9 commits intopython:mainfromgodlygeek:colorize_remote_pdb
May 6, 2025

Conversation

godlygeek
Copy link
Contributor

@godlygeekgodlygeek commentedMay 6, 2025
edited by bedevere-appbot
Loading

We can skip-news this one, it's a small fix on top of#133491 plus an integration test as@gaogaotiantian requested.

CC@ambv@pablogsal

@godlygeekgodlygeek marked this pull request as draftMay 6, 2025 06:16
@godlygeek
Copy link
ContributorAuthor

Marking as draft until I finish wrestling with getting the tests working on all platforms...

@godlygeekgodlygeek changed the titlegh-133490: Colorize remote pdbgh-133490: Fix syntax highlighting for remote PDBMay 6, 2025
@godlygeekgodlygeek marked this pull request as ready for reviewMay 6, 2025 07:50
@pablogsalpablogsal merged commitfd37f1a intopython:mainMay 6, 2025
55 checks passed
@gaogaotiantian
Copy link
Member

So first of all, I'm not sure if we should merge this without approval from@ambv. We introduced some new interface to the colorize system.

This is also not how I imagined the integration test should be. Why do we have to make the testing process part of the test? I don't think that makes it easier than juggling with 2 processes. Now that we have passed beta freeze, we don't need to rush into anything. I can work on a prototype and see how that works. Maybe I'm wrong - the integration tests is too fragile and we should do less of it.

The currenttest_pdb is using a separate process to run pdb tests and I think they worked fine. Of course it's just one process and there's no socket communication. I agree that when we first develop the test framework, there will be a lot of platform related issues, but most of those should only appear once.

@godlygeek
Copy link
ContributorAuthor

Why do we have to make the testing process part of the test? I don't think that makes it easier than juggling with 2 processes.

Because the Linux buildbots have/proc/sys/kernel/yama/ptrace_scope set to 1, and so if we spawn two child processes, one of them can't attach to the other.

We could spawn 1 child process and have it spawn a grandchild process that it's allowed to connect to. I tried it, but it was drastically more complicated. If you want to invest in it, though, it is possible, just ugly.

@gaogaotiantian
Copy link
Member

Because the Linux buildbots have/proc/sys/kernel/yama/ptrace_scope set to 1, and so if we spawn two child processes, one of them can't attach to the other.

That makes sense, and that also means pdb attach won't work in such scenario right? In real life.

@pablogsal
Copy link
Member

pablogsal commentedMay 6, 2025
edited
Loading

That makes sense, and that also means pdb attach won't work in such scenario right? In real life.

Yeah but that is working as expected. Is the same deal with gdb or any other debugger. Including PEP 768

@godlygeek
Copy link
ContributorAuthor

that also means pdb attach won't work in such scenario right? In real life.

Unless you usesudo, yeah

@pablogsal
Copy link
Member

So first of all, I'm not sure if we should merge this without approval from@ambv. We introduced some new interface to the colorize system.

Not really, we are leveraging all the existing theme infrastructure and mirroring the pyrepl APIs. The APIs are also experimental in any case. We can change them if we need after beta 1. I have also reviewed the original change and I can confirm that this is aligned to how is intended to be used.

@gaogaotiantian
Copy link
Member

Yeah I understand that's expected behavior. I meant we are testing a feature on a platform that does not support the feature.

@pablogsal
Copy link
Member

I meant we are testing a feature on a platform that does not support the feature.

Not really, is just that the permission model is more restricted. The platformdoes support the feature and the feature will work if you do it between process on the same process tree.

@gaogaotiantian
Copy link
Member

The platform does support the feature and the feature will work if you do it between process on the same process tree.

What would be a reasonable use case in this scenario? How would user usepython -m pdb -p for a process that is on the same tree (if I understand correctly, not siblings, they need to be parent/child)?

@godlygeek
Copy link
ContributorAuthor

How would user usepython -m pdb -p for a process that is on the same tree

Yeah, the "same tree" thing won't helppdb -p. It's meant for things likestrace orgdb that want to start a child process and then immediately attach to it and start tracing it. It doesn't help forstrace -p orgdb -p orpython -m pdb -p that want to attach to an arbitrary process. In all of those cases you'll either need to run the attaching process withsudo, or to have an administrator switch the YAMA ptrace scope to 0 instead of 1.

@godlygeek
Copy link
ContributorAuthor

And of course the integration tests that I just added rely on the "same tree" thing, by having the tests spawn a subprocess and then callpdb.main() directly. But that's not something users would ever want to do; that's specific to testing. If users wanted to have PDB launch a process, they'd just use

python -m pdb /path/to/script.py

@gaogaotiantian
Copy link
Member

What I meant is - if our buildbot does not provide an environment to support a real life scenario, then it's impossible for us to do reasonable integration test. I guess what we currently have is a compromise, which is not bad at all. I just wonder if we have some way to deal with the environment.

@godlygeek
Copy link
ContributorAuthor

It's not just about the build bots, either. Python's test suite is run by end users and redistributors, so even if the build bots could be set up with an environment with fewer security restrictions for the sake of realistic remote PDB integration tests, you still wouldn't want to rely on that, because it would still not work by default in most end user's Linux systems.

@gaogaotiantian
Copy link
Member

because it would still not work by default in most end user's Linux systems.

Do you mean that on most of the end users's Linux system,python -m pdb -p won't work by default?

@godlygeek
Copy link
ContributorAuthor

godlygeek commentedMay 6, 2025
edited
Loading

Yes, most Linux distros defaultptrace_scope to 1 these days as a security measure, and an administrator needs to specifically opt in to allowing a process to be debugged by an arbitrary process, either by running the debugger undersudo or granting theCAP_SYS_PTRACE capability or manually updating theptrace_scope to 0 by doing

echo 0| sudo tee /proc/sys/kernel/yama/ptrace_scope

We should probably explicitly call out how to configure different platforms so thatsys.remote_exec is permitted in the docs...

@pablogsal
Copy link
Member

pablogsal commentedMay 6, 2025
edited
Loading

because it would still not work by default in most end user's Linux systems.

Do you mean that on most of the end users's Linux system,python -m pdb -p won't work by default?

Just to insist on this: this applies to the same asany other debugging tool on the system like gdb, strace or anything. It just needs sudo to work as a precaution if the system has yama activated.

godlygeek reacted with heart emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@pablogsalpablogsalpablogsal approved these changes

@gaogaotiantiangaogaotiantianAwaiting requested review from gaogaotiantiangaogaotiantian is a code owner

@lysnikolaoulysnikolaouAwaiting requested review from lysnikolaoulysnikolaou is a code owner

@ambvambvAwaiting requested review from ambvambv is a code owner

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@godlygeek@gaogaotiantian@pablogsal

[8]ページ先頭

©2009-2025 Movatter.jp