-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
bpo-33901: Add _gdbm._GDBM_VERSION #7794
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* Fix also PyInit__gdbm) to catch errors. * test.pythoninfo: add gdbm.version * test_dbm_gnu now logs GDBM_VERSION when run in verbose mode.
Lib/test/pythoninfo.py
Outdated
| info_add('CC.version', text) | ||
|
|
||
|
|
||
| def collect_dbm(info_add): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe more accurate collect_gdbm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if (DbmError == NULL) { | ||
| goto error; | ||
| } | ||
| Py_INCREF(DbmError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? PyErr_NewException returns a new reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but it seems like most modules do the same:
socktemodule.c:
socket_herror = PyErr_NewException("socket.herror",
PyExc_OSError, NULL);
if (socket_herror == NULL)
return NULL;
Py_INCREF(socket_herror);
PyModule_AddObject(m, "herror", socket_herror);
signalmodule.c:
ItimerError = PyErr_NewException("signal.ItimerError",
PyExc_OSError, NULL);
if (ItimerError != NULL)
PyDict_SetItemString(d, "ItimerError", ItimerError);
Note: PyModule_AddObject() is weird, it decrements the reference counter on success...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's documented to steal a reference, though from the implementation, only on success. Module init functions seem usually not written carefully about this kind error. @serhiy-storchaka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module init functions seem usually not written carefully about this kind error.
If you want to fix that, I would suggest to fix it in a different PR. Maybe even open a new bug to track that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an open issue for this. It is not easy. You correctly use PyModule_AddObject() (most code in the stdlib doesn't).
Modules/_gdbmmodule.c
Outdated
| PyObject *obj = Py_BuildValue("iii", GDBM_VERSION_MAJOR, | ||
| GDBM_VERSION_MINOR, GDBM_VERSION_PATCH); | ||
| if (obj == NULL) { | ||
| return NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not goto error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's a bug. Fixed.
| def setUpClass(): | ||
| if support.verbose: | ||
| try: | ||
| import _gdbm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe from _gdbm import _GDBM_VERSION?
| if (DbmError == NULL) { | ||
| goto error; | ||
| } | ||
| Py_INCREF(DbmError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an open issue for this. It is not easy. You correctly use PyModule_AddObject() (most code in the stdlib doesn't).
|
My PR adds a private attribute for tests: pythoninfo and test_dbm_gnu. If
someone wants to get further and add a public PR, go ahead, but I am not
interested to go that far. I just proposed the PR because it wasn't too
hard to write it and Xiang asked for it. I never used dbm.gnu :-)
|
|
This doesn't even build on the TravisCI: /home/travis/build/python/cpython/Modules/_gdbmmodule.c:681:42: error: use of undeclared identifier 'GDBM_VERSION_MAJOR' |
|
see GH-7823 |
Oh :-( I only saw that the build is a success. The compilation error is hidden by default, I had to unfold "make ..." line. |
https://bugs.python.org/issue33901