◐ Shell
reader mode source ↗
Skip to content

bpo-24160: Fix breakpoints persistence across multiple pdb sessions#21989

Merged
gvanrossum merged 9 commits into
python:masterfrom
iritkatriel:pdb-bps
Apr 2, 2021
Merged

bpo-24160: Fix breakpoints persistence across multiple pdb sessions#21989
gvanrossum merged 9 commits into
python:masterfrom
iritkatriel:pdb-bps

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Aug 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

The information about breakpoints is split between Breakpoint.bplist/Breakpoint.bpbynumber (at class scope) and the breaks dictionary that lives on the Bdb() instance. The problems described in issue24160 occurs when those two parts get out of sync.

In interactive mode, each pdb.run() call creates a new instance of Bdb(). In this case the user would like the breakpoints to be remembered between calls. This PR adds a function _load_breaks to Bdb, which is called in its constructor and populates the instance's breaks dict with the data from the Breakpoint class.

Tests were added in both test_bdb (to test it directly) and in test_pdb (to test it with the interactive usecase).

https://bugs.python.org/issue24160

@iritkatriel iritkatriel changed the title bpo-24160: load breakpoints from global list when creating a new debu… Aug 28, 2020
@pppery

pppery commented Aug 28, 2020

Copy link
Copy Markdown

I would have gone the other way with this, and made the breakpoints for different Bdb instances completely independent, rather than completely dependent, but this works too.

@iritkatriel

iritkatriel commented Aug 28, 2020

Copy link
Copy Markdown
Member Author

That was my first thought as well, but then I realised it would be annoying to have to set the same breakpoints again and again while debugging something. (It’s easy to do ‘clear’ if that’s what you want).

@iritkatriel iritkatriel reopened this Aug 28, 2020
@iritkatriel iritkatriel force-pushed the pdb-bps branch 2 times, most recently from fb6cac6 to 74c6560 Compare August 28, 2020 23:00
@iritkatriel iritkatriel changed the title bpo-24160: Fixed breakpoints persistence across multiple pdb sessions Aug 29, 2020
@taleinat taleinat added needs backport to 3.8 type-bug An unexpected behavior, bug, or error labels Nov 5, 2020
taleinat
taleinat reviewed Nov 5, 2020
View reviewed changes

@taleinat taleinat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

If possible, I think it would be god for the new tests to be slightly expanded to also include deleting breakpoints.

taleinat
taleinat reviewed Nov 5, 2020
View reviewed changes

@taleinat taleinat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Perhaps now Bdb.clear_all_breaks() should use the new clearBreakpoints internally?

@iritkatriel

iritkatriel commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

If possible, I think it would be god for the new tests to be slightly expanded to also include deleting breakpoints.

I don't see any tests for deleting breakpoint, and indeed coverage seems pretty grim:
https://codecov.io/gh/python/cpython/src/master/Lib/bdb.py

@iritkatriel

iritkatriel commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

Perhaps now Bdb.clear_all_breaks() should use the new clearBreakpoints internally?

I think this could break something if some Bdb subclass changes the way this data is represented.

@iritkatriel

iritkatriel commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

It's messy because this class level state is accessed through self.

@taleinat

taleinat commented Nov 5, 2020

Copy link
Copy Markdown
Contributor

Perhaps now Bdb.clear_all_breaks() should use the new clearBreakpoints internally?

I think this could break something if some Bdb subclass changes the way this data is represented.

True, let's leave it as is for now.

@taleinat

taleinat commented Nov 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Random question: Maybe someone knows how "lineno" came to be shorthand for "line number"??

@taleinat

taleinat commented Nov 5, 2020

Copy link
Copy Markdown
Contributor

@iritkatriel, this is looking good. I'll make a final review later and likely approve.

@iritkatriel

iritkatriel commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

Random question: Maybe someone knows how "lineno" came to be shorthand for "line number"??

I asked google and it said something about Lenin.

@gvanrossum

gvanrossum commented Dec 22, 2020

Copy link
Copy Markdown
Member

This looks like a feature, let's not backport it.

@iritkatriel iritkatriel removed needs backport to 3.8 type-bug An unexpected behavior, bug, or error labels Dec 22, 2020
@iritkatriel

iritkatriel commented Apr 1, 2021

Copy link
Copy Markdown
Member Author

Shall we try to get this into 3.10?

@gvanrossum

gvanrossum commented Apr 1, 2021

Copy link
Copy Markdown
Member

SGTM. Is it ready? I haven't reviewed it yet.

@iritkatriel

iritkatriel commented Apr 2, 2021

Copy link
Copy Markdown
Member Author

I've just rebased it. I think it is ready to merge. @taleinat had reviewed it a while back and we made some improvements.

gvanrossum
gvanrossum approved these changes Apr 2, 2021
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Okay, from a cursory inspection looks good. I trust @taleinat.

@gvanrossum gvanrossum merged commit ad442a6 into python:master Apr 2, 2021
@iritkatriel iritkatriel deleted the pdb-bps branch April 29, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taleinat taleinat taleinat left review comments

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@iritkatriel @pppery @taleinat @gvanrossum @the-knights-who-say-ni @bedevere-bot