Rework uuid module: lazy initialization and add a new C extension

[dfcc25ac-e242-4495-8f49-176193f06a74]

BPO	11063
Nosy	@ncoghlan, @orsenthil, @pitrou, @vstinner, @tiran, @ned-deily, @merwok, @bitdancer, @methane, @skrah, @ambv, @berkerpeksag, @hynek, @vadmium, @serhiy-storchaka, @aixtools
PRs	python/cpython#3795 python/cpython#3796 python/cpython#3684 python/cpython#3855 python/cpython#4287 python/cpython#4343 python/cpython#4565
Files	issue11063.patch: Fix issue python/cpython#55272 issue11063_2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = ['easy', '3.7', 'library', 'performance'] title = 'Rework uuid module: lazy initialization and add a new C extension' updated_at = user = 'https://bugs.python.org/KeithDart' ``` bugs.python.org fields: ```python activity = actor = 'ncoghlan' assignee = 'none' closed = True closed_date = closer = 'berker.peksag' components = ['Library (Lib)'] creation = creator = 'Keith.Dart' dependencies = [] files = ['20685', '29220'] hgrepos = [] issue_num = 11063 keywords = ['patch', 'easy'] message_count = 67.0 messages = ['127442', '127989', '127998', '128001', '128008', '128010', '128012', '128013', '128015', '128016', '128017', '128018', '128019', '128020', '128021', '178293', '178297', '182778', '182873', '182875', '264151', '264998', '265109', '265122', '265126', '265449', '303202', '303205', '303208', '303209', '303210', '303211', '303212', '303213', '303214', '303215', '303218', '303220', '303228', '303230', '303231', '303234', '303238', '303281', '303284', '303288', '303365', '303406', '303525', '303531', '303532', '303533', '303534', '303539', '303540', '303552', '303555', '305587', '305589', '305635', '305897', '305900', '305904', '305909', '305911', '306984', '306985'] nosy_count = 23.0 nosy_names = ['ncoghlan', 'orsenthil', 'kdart', 'pitrou', 'vstinner', 'christian.heimes', 'ned.deily', 'eric.araujo', 'Arfrever', 'r.david.murray', 'methane', 'skrah', 'nvetoshkin', 'lukasz.langa', 'knny-myer', 'nailor', 'Keith.Dart', 'berker.peksag', 'hynek', 'martin.panter', 'serhiy.storchaka', 'Michael.Felt', 'aixtools@gmail.com'] pr_nums = ['3795', '3796', '3684', '3855', '4287', '4343', '4565'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'resource usage' url = 'https://bugs.python.org/issue11063' versions = ['Python 3.7'] ```

[pitrou]

Though I don't know how to reuse the find_file() logic in configure...

[vstinner]

Though I don't know how to reuse the find_file() logic in configure...

Maybe we could use pkg-config instead?

haypo@selma$ pkg-config uuid --cflags -I/usr/include/uuid haypo@selma$ pkg-config uuid --libs -luuid

[pitrou]

pkg-config is a Linux-ism. But Linux already works fine...

$ uname 
Darwin
$ pkg-config
-bash: pkg-config: command not found

[vstinner]

I proposed PR 3855 to add macOS support to _uuid (and fix the compilation error).

[vstinner]

New changeset 4337a0d9955f0855ba38ef30feec3858d304abf0 by Victor Stinner in branch 'master': bpo-11063: Fix _uuid module on macOS (bpo-3855) https://github.com/python/cpython/commit/4337a0d9955f0855ba38ef30feec3858d304abf0

[ned-deily]

I agree with Barry's comment on PR 3855: "I'd rather see a configure check for the existence of uuid_generate_time_safe() rather than hard coding it to platforms !APPLE for two reasons. 1) If macOS ever adds this API in some future release, this ifndef will be incorrect, and 2) if some other platform comes along that doesn't have this API, it will still use the incorrect function." It's exactly for situations like this that autoconf tests exist; we should not be hardwiring assumptions about the lack of particular platform APIs.

[5531d0d8-2a9c-46ba-8b8b-ef76132a492c]

I think the configure check should be this (sets HAVE_LIBUUID in pyconfig.h):

diff --git a/configure.ac b/configure.ac
index 41bd9effbf..90d53c1b7d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2657,6 +2657,7 @@ AC_MSG_RESULT($SHLIBS)
 AC_CHECK_LIB(sendfile, sendfile)
 AC_CHECK_LIB(dl, dlopen)       # Dynamic linking for SunOS/Solaris and SYSV
 AC_CHECK_LIB(dld, shl_load)    # Dynamic linking for HP-UX
+AC_CHECK_LIB(uuid, uuid_generate_time_safe)

 # only check for sem_init if thread support is requested
 if test "$with_threads" = "yes" -o -z "$with_threads"; then

[berkerpeksag]

I've followed Stefan's suggestion and opened PR 4287 (tested on 10.10.5)

[5531d0d8-2a9c-46ba-8b8b-ef76132a492c]

Berker's latest patch looks good to me.

Unrelated to the patch (same before and after), this looks odd to me:

>>> import uuid
>>> uuid._has_uuid_generate_time_safe is None
True
>>> 
>>> import _uuid
>>> _uuid.has_uuid_generate_time_safe
1

[Also, I thought we weren't supposed to use ctypes in the stdlib.]

[vstinner]

"""

Unrelated to the patch (same before and after), this looks odd to me:

>>> import uuid
>>> uuid._has_uuid_generate_time_safe is None
True
>>> 
>>> import _uuid
>>> _uuid.has_uuid_generate_time_safe
1
"""

None means "not initialized yet". It's initialized on demand, at the first call of uuid1() or get_node():

$ python3
Python 3.7.0a2+ (heads/master:a5293b4ff2, Nov  6 2017, 12:22:04) 
>>> import uuid
>>> uuid._has_uuid_generate_time_safe  # == None
>>> uuid.uuid1()
UUID('3e5a7628-c2e5-11e7-adc1-3ca9f4650c0c')
>>> uuid._has_uuid_generate_time_safe
1

[Also, I thought we weren't supposed to use ctypes in the stdlib.]

Antoine's commit a106aec2ed6ba171838ca7e6ba43c4e722bbecd1 avoids ctypes when libuuid is available.

For the other systems without libuuid, well, it was probably simpler to use ctypes. ctypes was more popular a few years ago. The code "just works" and I guess that nobody wants to touch it :-)

[berkerpeksag]

New changeset 9a10ff4deb2494e22bc0dbea3e3a6f9e8354d995 by Berker Peksag in branch 'master': bpo-11063: Add a configure check for uuid_generate_time_safe (GH-4287) https://github.com/python/cpython/commit/9a10ff4deb2494e22bc0dbea3e3a6f9e8354d995

[serhiy-storchaka]

Does this check work? I tried similar checks with other functions and they were falsely passed because calling an undeclared function is not an error in C.

The reliable check of the existence of the uuid_generate_time_safe function is:

void *x = uuid_generate_time_safe

[5531d0d8-2a9c-46ba-8b8b-ef76132a492c]

On Wed, Nov 08, 2017 at 08:28:10PM +0000, Serhiy Storchaka wrote:

Does this check work? I tried similar checks with other functions and they were falsely passed because calling an undeclared function is not an error in C.

Not here. If I replace uuid_generate_time_safe with a non-existing function name, it is still found:

checking for uuid_generate_time_unsafe... yes

The originally suggested AC_CHECK_LIB macro however works here in both cases. :)

[berkerpeksag]

It worked for me on OS X (returns no) and Linux (returns yes after I installed uuid-dev) but I didn't test it on both systems at the same. Travis CI also returned 'no': https://travis-ci.org/python/cpython/jobs/299285001

In any case, Serhiy's suggestion is better than mine so I've opened PR 4343.

And yes, I'm beginning to regret my decision on not using AC_CHECK_LIB :)

[berkerpeksag]

New changeset 0e163d2ced28ade8ff526e8c663faf03c2c0b168 by Berker Peksag in branch 'master': bpo-11063: Use more reliable way to check if uuid function exists (GH-4343) https://github.com/python/cpython/commit/0e163d2ced28ade8ff526e8c663faf03c2c0b168

[ncoghlan]

The header file check in setup.py incorrectly reported "not found" if uuid.h was in one of the standard include directories, so I've submitted a tweak to fix that: https://github.com/python/cpython/pull/4565

[ncoghlan]

New changeset 53efbf3977a44e382397e7994a2524b4f8c9d053 by Nick Coghlan in branch 'master': bpo-11063: Handle uuid.h being in default include path (GH-4565) https://github.com/python/cpython/commit/53efbf3977a44e382397e7994a2524b4f8c9d053