Page MenuHomeFreeBSD

lang/python*: Install GDB debugging scripts
ClosedPublic

Authored by cem on Apr 14 2017, 4:28 PM.

Details

Summary

Commit log message:

lang/python*: Install GDB debugging scripts

Users with a GDB that supports Python extensions [1] will automatically
load the extra debugging extensions when debugging programs that are linked
with libpythonX.Y.so.foo.

This enables extensions like 'py-bt' and 'py-frame' as described in
the Fedora Wiki Article: Easier Python Debugging [2], which can be
very useful for debugging Python program state from crashes in
C extensions, for example.

[1] PYTHON option enabled in devel/gdb
[2] https://fedoraproject.org/wiki/Features/EasierPythonDebugging 

PR: 203021
Reviewed_by: mat, koobs (Python)
Approved by: koobs (Python)
Differential_Revision: D10398
Test Plan
  • testport: Pending (poudriere)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I've seen other -gdb.py files installed to share/gdb/auto-load (such as /usr/local/share/gdb/auto-load/usr/local/lib/libglib-2.0.so.0.5000.2-gdb.py from the devel/glib20 port). Should the python -gdb.py file be installed there, too?

lang/python27/Makefile
128 ↗(On Diff #27439)

Use ${INSTALL_DATA} instead of 'install -m444' (same for python3* versions)

I've seen other -gdb.py files installed to share/gdb/auto-load (such as /usr/local/share/gdb/auto-load/usr/local/lib/libglib-2.0.so.0.5000.2-gdb.py from the devel/glib20 port). Should the python -gdb.py file be installed there, too?

Does that mean the port would need a runtime dependency on gdb for the /usr/local/share/gdb directory? If not, seems like a good idea to me.

mat requested changes to this revision.Apr 14 2017, 5:31 PM
mat added inline comments.
lang/python27/Makefile
128 ↗(On Diff #27439)

No, INSTALL_DATA means 644. (But, yeah, it must use INSTALL_DATA, not install direclty.

This revision now requires changes to proceed.Apr 14 2017, 5:31 PM
In D10398#215440, @cem wrote:

Does that mean the port would need a runtime dependency on gdb for the /usr/local/share/gdb directory? If not, seems like a good idea to me.

No a dependency on gdb should not be required.

I don't know if share/gdb/auto-load/ is the desired standard place. FWIW, linux (e.g., Fedora) puts some -gdb.py files there:

$ locate gdb.py
/usr/bin/mono-gdb.py
/usr/bin/mono-sgen-gdb.py
/usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.21-gdb.py
/usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.21-gdb.pyc
/usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.21-gdb.pyo
/usr/share/gdb/auto-load/usr/lib64/libisl.so.13.1.0-gdb.py
/usr/share/gdb/auto-load/usr/lib64/libstdc++.so.6.0.21-gdb.py
/usr/share/gdb/auto-load/usr/lib64/libstdc++.so.6.0.21-gdb.pyc
/usr/share/gdb/auto-load/usr/lib64/libstdc++.so.6.0.21-gdb.pyo

lang/python27/Makefile
128 ↗(On Diff #27439)

Indeed. Although if for some reason one really wanted 0444, you should add to pkg-plist anyway. In this case 444 is probably right (not really a file to be edited), but not worth the trouble to override the default 644 IMO.

In D10398#215440, @cem wrote:

Does that mean the port would need a runtime dependency on gdb for the /usr/local/share/gdb directory? If not, seems like a good idea to me.

No a dependency on gdb should not be required.

Ok.

I don't know if share/gdb/auto-load/ is the desired standard place. FWIW, linux (e.g., Fedora) puts some -gdb.py files there:

$ locate gdb.py
/usr/bin/mono-gdb.py
/usr/bin/mono-sgen-gdb.py

Right — they can either be co-located with the relevant binaries, or in a couple automatically loaded places.

/usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.21-gdb.py
/usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.21-gdb.pyc
/usr/share/gdb/auto-load/usr/lib/libstdc++.so.6.0.21-gdb.pyo
/usr/share/gdb/auto-load/usr/lib64/libisl.so.13.1.0-gdb.py
/usr/share/gdb/auto-load/usr/lib64/libstdc++.so.6.0.21-gdb.py
/usr/share/gdb/auto-load/usr/lib64/libstdc++.so.6.0.21-gdb.pyc
/usr/share/gdb/auto-load/usr/lib64/libstdc++.so.6.0.21-gdb.pyo

Fedora puts the Python -gdb.py stuff here:

/usr/lib/debug/usr/lib64/libpython2.7.so.1.0.debug-gdb.py
/usr/lib/debug/usr/lib64/libpython2.7_d.so.1.0.debug-gdb.py

But /usr/lib/debug is still somewhat immature in FreeBSD, I'd prefer either co-located or in the /usr/share/gdb directory. (By the way, is it ok for a port to install into such a path outside of LOCALBASE?)

The reason Fedora gives for not co-locating the script with the shared library is that it "generates noise in ldconfig:" https://bugzilla.redhat.com/show_bug.cgi?id=562980 . We could instead teach FreeBSD ldconfig to ignore such scripts, if it whines about them.

cem edited edge metadata.
cem marked 3 inline comments as done.

Use ${INSTALL_DATA} instead of manual install.
Leave installed location alone while it is under discussion.

Installing under ${PREFIX} (typically LOCALBASE) is generally preferred. The most common things installed outside of PREFIX/LOCALBASE are things under /var. Many kernel module ports install to /boot/modules.

FreeBSD's ldconfig doesn't whine for non-shlib files. It has to work a wee bit more to ignore them, but that's pretty minimal.

Sounds like there's no clear standard location. I think co-located with the python lib is probably fine for now.

Installing under ${PREFIX} (typically LOCALBASE) is generally preferred. The most common things installed outside of PREFIX/LOCALBASE are things under /var. Many kernel module ports install to /boot/modules.

Yeah, that makes sense.

FreeBSD's ldconfig doesn't whine for non-shlib files. It has to work a wee bit more to ignore them, but that's pretty minimal.

Ok.

Sounds like there's no clear standard location. I think co-located with the python lib is probably fine for now.

If you think it's okay as-is, can you mark the review as "Accepted?" Thanks!

Ping? Can I commit this?

In D10398#218054, @cem wrote:

Ping? Can I commit this?

As soon as someone from python says yes. I added the blocking reviewer for a reason, so that you know when it is ok to commit this. Removing it does not change that fact.

cem abandoned this revision.EditedApr 28 2017, 12:21 PM

Nevertheless, it's been two weeks and no one has actually reviewed it... so it's a de facto veto. The ports team desperately needs to grow and this is not an effective way to grow it.

@cem The Python port changes look fine, though I can't comment on the correct location for debug/gdb bits in the short or long-term for FreeBSD in general.

If you'd like to commit this change, please:

  • Update the SUMMARY field to reflect what the exact proposed commit log message would be and include all the relevant Metadata: fields such as Reviewed_by:, Approved by: Differential Revision:, et al.
  • Confirm these changes pass QA (poudriere in particular). Add these details to the TEST PLAN section of the review

If you don't have an adequate QA environment setup now, just let me know and I'll take care of the commit, but a complete commit log message including rationale/explanation as well as describing the use-case/benefit for users would be great so I can just copy pasta.

@cem The Python port changes look fine, though I can't comment on the correct location for debug/gdb bits in the short or long-term for FreeBSD in general.

I think it's an ok location for now. If we change our minds later, we can always fix it then.

If you'd like to commit this change, please:

I don't care who commits it, I just want FreeBSD to be a little bit better and I'm trying to share knowledge of the awesome Python GDB integration work that's been done.

  • Update the SUMMARY field to reflect what the exact proposed commit log message

It's already that. (The first line is in the title, though.)

would be and include all the relevant Metadata: fields such as Reviewed_by:, Approved by: Differential Revision:, et al.

Well, so far, all of those are empty. I fill them in when I commit based on the results of the review.

  • Confirm these changes pass QA (poudriere in particular). Add these details to the TEST PLAN section of the review

If you don't have an adequate QA environment setup now, just let me know

I don't have a pourdriere environment set up. My FreeBSD machine has 4.5 GB of free disk space and I can't just install a bunch of jails. Are there any cluster resources for poudriere testing?

Obviously, this change should have no functional impact on dependent ports. It's just dropping an extra file that shouldn't conflict with anything.

and I'll take care of the commit,

I would appreciate it.

but a complete commit log message including rationale/explanation as well as describing the use-case/benefit for users would be great so I can just copy pasta.

That's already present in the review title/description. Python developers can more easily debug their programs from GDB. This is most useful when you have a crash in a C extensions, or are debugging Python internals. But you could also gcore a running Python and inspect it with GDB with these extensions. Really, check out the URL from the description, it's quite neat.

koobs retitled this revision from Python: Install gdb debugging scripts to lang/python*: Install GDB debugging scripts.Apr 29 2017, 5:55 AM
koobs edited the summary of this revision. (Show Details)
koobs edited the test plan for this revision. (Show Details)
In D10398#218437, @cem wrote:

@cem The Python port changes look fine, though I can't comment on the correct location for debug/gdb bits in the short or long-term for FreeBSD in general.

I think it's an ok location for now. If we change our minds later, we can always fix it then.

If you'd like to commit this change, please:

I don't care who commits it, I just want FreeBSD to be a little bit better and I'm trying to share knowledge of the awesome Python GDB integration work that's been done.

  • Update the SUMMARY field to reflect what the exact proposed commit log message

It's already that. (The first line is in the title, though.)

would be and include all the relevant Metadata: fields such as Reviewed_by:, Approved by: Differential Revision:, et al.

Well, so far, all of those are empty. I fill them in when I commit based on the results of the review.

  • Confirm these changes pass QA (poudriere in particular). Add these details to the TEST PLAN section of the review

If you don't have an adequate QA environment setup now, just let me know

I don't have a pourdriere environment set up. My FreeBSD machine has 4.5 GB of free disk space and I can't just install a bunch of jails. Are there any cluster resources for poudriere testing?

Obviously, this change should have no functional impact on dependent ports. It's just dropping an extra file that shouldn't conflict with anything.

and I'll take care of the commit,

I would appreciate it.

but a complete commit log message including rationale/explanation as well as describing the use-case/benefit for users would be great so I can just copy pasta.

That's already present in the review title/description. Python developers can more easily debug their programs from GDB. This is most useful when you have a crash in a C extensions, or are debugging Python internals. But you could also gcore a running Python and inspect it with GDB with these extensions. Really, check out the URL from the description, it's quite neat.

Reviews are for commit metadata as well so they ought to be as verbatim as possible, particularly for non-port committers requiring extra approval or where the committer is not the maintainer. I've updated the SUMMARY as an example for future reviews, can you please re-read it and confirm that it still has correct/accurate intent and semantics.

I'll take care of the QA and commit from there once you confirm the messaging is OK.

Reviewed_by: mat, koobs (Python)

Mat didn't actually review it, did he?

Approved by: koobs (Python)
Differential_Revision: D10398

What's with the extra underscores in the metadata?

@cem also take the differential again (undo the abandoning) so the changeset/review remains attributed to you

@cem The status of @mat's review appears to request/require further changes. I'm not sure if this is still the case, and he'll need to update the status

Reviews are for commit metadata as well so they ought to be as verbatim as possible, particularly for non-port committers requiring extra approval or where the committer is not the maintainer. I've updated the SUMMARY as an example for future reviews, can you please re-read it and confirm that it still has correct/accurate intent and semantics.

Looks good to me (modulo earlier nits).

I'll take care of the QA and commit from there once you confirm the messaging is OK.

Thanks!

In D10398#218442, @cem wrote:

Reviewed_by: mat, koobs (Python)

Mat didn't actually review it, did he?

I'm going by the review status says requires changes (x) by mat to mean it was reviewed (changes requested implies it was)

Approved by: koobs (Python)
Differential_Revision: D10398

What's with the extra underscores in the metadata?

Reviewed by appears to be a reserved word/phrase in Phabricator when they at the beginning of a line, even within code blocks and Phabricator wont allow you to save updates to revision without changing those property names. Differential Revision is also reserved.

@cem also take the differential again (undo the abandoning) so the changeset/review remains attributed to you

Meh. I've had a horrible experience trying to improve ports without a commit bit. In the future, I'll just maintain a ports overlay and stop submitting my improvements back upstream. It just isn't worth it when portmgr is so hostile to outsiders.

@cem The status of @mat's review appears to request/require further changes. I'm not sure if this is still the case, and he'll need to update the status

So far all he's done is sandbag the review. If Python team approves and he has no concrete objections, I'm not sure why the review needs to be blocked on him.

In D10398#218447, @cem wrote:

@cem also take the differential again (undo the abandoning) so the changeset/review remains attributed to you

Meh. I've had a horrible experience trying to improve ports without a commit bit. In the future, I'll just maintain a ports overlay and stop submitting my improvements back upstream. It just isn't worth it when portmgr is so hostile to outsiders.

Please don't do that, the vast majority of ports committers (including myself) want to make it as easy as possible for people to submit changes, ideally ending up with more src people getting ports bits after mentorship (unofficial or otherwise). For all diffs I review, I treat them as if they are from a mentee (full review), to encourage continual improvement, even from existing committers. This should not be seen as hoops to jump through or barriers to entry.

@cem The status of @mat's review appears to request/require further changes. I'm not sure if this is still the case, and he'll need to update the status

So far all he's done is sandbag the review. If Python team approves and he has no concrete objections, I'm not sure why the review needs to be blocked on him.

Mat has subscribed himself to all diffs that fall under the ports tree (as anyone can do), and giving him the benefit of the doubt, its so that if a new diff has been created with no 'reviewers' by the submitter, that the changeset can receive some (not zero) attention and not fall through the cracks or demotivate contributors. Whether the execution matches the goal is a separate issue.

I see that the INSTALL_DATA change requested (by @mat) has been made in the diff, and only the state of the review needs to change, but the commit is not (administratively) blocked on that.

In D10398#218447, @cem wrote:

@cem The status of @mat's review appears to request/require further changes. I'm not sure if this is still the case, and he'll need to update the status

So far all he's done is sandbag the review. If Python team approves and he has no concrete objections, I'm not sure why the review needs to be blocked on him.

What I did was:

  1. review the change, and comment on it
  2. add the appropriate group to the review so that it can be reviewed by the people who have a clue.

This tool, phabricator, here, is a code review tool, which means it is there so that changes can be reviewed so that the submitter improves his ports skills (or src, or doc skills).

If you did not mean to ask for review of your code and wanted to open a bug report, so that the appropriate maintainer could work on it and commit it after it met with the ports standards, you should have done so on our bug tracking software, which is at https://bugs.freebsd.org/submit/.

In D10398#219056, @mat wrote:
  1. add the appropriate group to the review so that it can be reviewed by the people who have a clue.

It's a stupidly simple change that any ports committer could review for sanity. Hell, a dozen would review it immediately if I just committed it. It's either fine, or not fine. No one reviewed it for weeks.

This tool, phabricator, here, is a code review tool, which means it is there so that changes can be reviewed so that the submitter improves his ports skills (or src, or doc skills).

Don't be an ass, mat. I've used phabricator dozens of times for my own reviews and reviewed hundreds of others' changes. I'm familiar with its use.

If you did not mean to ask for review of your code and wanted to open a bug report, so that the appropriate maintainer could work on it and commit it after it met with the ports standards, you should have done so on our bug tracking software, which is at https://bugs.freebsd.org/submit/.

Don't be an ass, mat. I requested a review and never got one. I'm familiar with bugzilla and you know that. This is just another example of your hostility to non-committers.

Found some issue in QA, will add comments here where appropriate. No changes necessary, I've fixed them in my working copy. Will land them once QA is complete.

lang/python33/Makefile
121 ↗(On Diff #27444)

Doesn't match pkg-plist entry, missing ${ABIFLAGS} suffix of filename (libpython3.3m.so.1-gdb.py)

lang/python34/Makefile
124 ↗(On Diff #27444)

Doesn't match pkg-plist entry, missing ${ABIFLAGS} suffix of filename (libpython3.4m.so.1-gdb.py)

lang/python35/Makefile
132 ↗(On Diff #27444)

Using package plist (pkg-plist) %%substitution%% syntax, should be the following instead:

${STAGEDIR}${PREFIX}/lib/libpython${PYTHON_VER}${ABIFLAGS}.so.1.0-gdb.py

lang/python36/Makefile
128 ↗(On Diff #27444)

Using package plist (pkg-plist) %%substitution%% syntax, should be the following instead:

${STAGEDIR}${PREFIX}/lib/libpython${PYTHON_VER}${ABIFLAGS}.so.1.0-gdb.py

Package changes (new file), bump PORTREVISION accordingly