Skip to content

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Jun 19, 2018

  • Fix also PyInit__gdbm) to catch errors.
  • test.pythoninfo: add gdbm.version
  • test_dbm_gnu now logs GDBM_VERSION when run in verbose mode.

https://bugs.python.org/issue33901

* Fix also PyInit__gdbm) to catch errors.
* test.pythoninfo: add gdbm.version
* test_dbm_gnu now logs GDBM_VERSION when run in verbose mode.
info_add('CC.version', text)


def collect_dbm(info_add):
Copy link
Member

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?

Copy link
Member Author

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);
Copy link
Member

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.

Copy link
Member Author

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...

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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).

PyObject *obj = Py_BuildValue("iii", GDBM_VERSION_MAJOR,
GDBM_VERSION_MINOR, GDBM_VERSION_PATCH);
if (obj == NULL) {
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

why not goto error?

Copy link
Member Author

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
Copy link
Member

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);
Copy link
Member

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).

@serhiy-storchaka
Copy link
Member

Wouldn't be better to add a named tuple like the one in bpo-31680? I have not merged #4217 still because I want to add similar named tuples for all external libraries.

@vstinner
Copy link
Member Author

vstinner commented Jun 19, 2018 via email

@vstinner vstinner merged commit 00f9edb into python:master Jun 19, 2018
@vstinner vstinner deleted the gdbm_version branch June 19, 2018 21:29
@ZackerySpytz
Copy link
Contributor

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'
PyObject *obj = Py_BuildValue("iii", GDBM_VERSION_MAJOR,
^
/home/travis/build/python/cpython/Modules/_gdbmmodule.c:682:35: error: use of undeclared identifier 'GDBM_VERSION_MINOR'
GDBM_VERSION_MINOR, GDBM_VERSION_PATCH);
^
/home/travis/build/python/cpython/Modules/_gdbmmodule.c:682:55: error: use of undeclared identifier 'GDBM_VERSION_PATCH'
GDBM_VERSION_MINOR, GDBM_VERSION_PATCH);

@zhangyangyu
Copy link
Member

see GH-7823

@vstinner
Copy link
Member Author

This doesn't even build on the TravisCI: (...)

Oh :-( I only saw that the build is a success. The compilation error is hidden by default, I had to unfold "make ..." line.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants