Skip to content

bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid #3796

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.

Sign up for GitHub

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

Merged
pitrou merged 8 commits into from
Sep 28, 2017
Merged

bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid #3796

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
32b1b40
bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid
pitrou Sep 28, 2017
05dcee2
Fix _windll_getnode
pitrou Sep 28, 2017
f4d7cf3
Add blurb
pitrou Sep 28, 2017
8f4dfb2
Nits in uuid.py
pitrou Sep 28, 2017
a78dc43
Generalize testing with and without C extension "_uuid"
pitrou Sep 28, 2017
34df739
Fix tests on Windows
pitrou Sep 28, 2017
92b8670
Address review comments
pitrou Sep 28, 2017
88bb338
Fix test regression
pitrou Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address review comments
  • Loading branch information
@pitrou
pitrou committed Sep 28, 2017
commit 92b8670d13e6d2241540aacee1b17664a4646a11
35 changes: 23 additions & 12 deletions Lib/test/test_uuid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import unittest.mock
from test import support
import builtins
import contextlib
import io
import os
import shutil
Expand All @@ -17,7 +18,9 @@ def importable(name):
except:
return False


class BaseTestUUID:
uuid = None

@vstinner vstinner Sep 28, 2017

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.

Please add "uuid = None" here.

def test_UUID(self):
equal = self.assertEqual
Expand Down Expand Up @@ -360,37 +363,44 @@ def test_uuid1_safe(self):
# unknown (unless I suppose the platform is buggy).
self.assertNotEqual(u.is_safe, self.uuid.SafeUUID.unknown)

@contextlib.contextmanager
def mock_generate_time_safe(self, safe_value):
"""
Mock uuid._generate_time_safe() to return a given *safe_value*.
"""
if os.name != 'posix':
self.skipTest('POSIX-only test')
self.uuid._load_system_functions()
f = self.uuid._generate_time_safe
if f is None:
self.skipTest('need uuid._generate_time_safe')
with unittest.mock.patch.object(self.uuid, '_generate_time_safe',
lambda: (f()[0], safe_value)):
yield

@unittest.skipUnless(os.name == 'posix', 'POSIX-only test')
def test_uuid1_unknown(self):
# Even if the platform has uuid_generate_time_safe(), let's mock it to
# be uuid_generate_time() and ensure the safety is unknown.
f = self.uuid._generate_time_safe
with unittest.mock.patch.object(self.uuid, '_generate_time_safe',
lambda: (f()[0], None)):
with self.mock_generate_time_safe(None):
u = self.uuid.uuid1()
self.assertEqual(u.is_safe, self.uuid.SafeUUID.unknown)

@unittest.skipUnless(os.name == 'posix', 'POSIX-only test')
def test_uuid1_is_safe(self):
f = self.uuid._generate_time_safe
with unittest.mock.patch.object(self.uuid, '_generate_time_safe',
lambda: (f()[0], 0)):
with self.mock_generate_time_safe(0):
u = self.uuid.uuid1()
self.assertEqual(u.is_safe, self.uuid.SafeUUID.safe)

@unittest.skipUnless(os.name == 'posix', 'POSIX-only test')
def test_uuid1_is_unsafe(self):
f = self.uuid._generate_time_safe
with unittest.mock.patch.object(self.uuid, '_generate_time_safe',
lambda: (f()[0], -1)):
with self.mock_generate_time_safe(-1):
u = self.uuid.uuid1()
self.assertEqual(u.is_safe, self.uuid.SafeUUID.unsafe)

@unittest.skipUnless(os.name == 'posix', 'POSIX-only test')
def test_uuid1_bogus_return_value(self):
f = self.uuid._generate_time_safe
with unittest.mock.patch.object(self.uuid, '_generate_time_safe',
lambda: (f()[0], 3)):
with self.mock_generate_time_safe(3):
u = self.uuid.uuid1()
self.assertEqual(u.is_safe, self.uuid.SafeUUID.unknown)

Expand Down Expand Up @@ -476,6 +486,7 @@ class TestUUIDWithExtModule(BaseTestUUID, unittest.TestCase):


class BaseTestInternals:

@vstinner vstinner Sep 28, 2017

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.

Please add "uuid = None" here.

uuid = None

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
def test_find_mac(self):
Expand Down
24 changes: 13 additions & 11 deletions Lib/uuid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,13 @@ def _netbios_getnode():
_generate_time_safe = _UuidCreate = None
_has_uuid_generate_time_safe = None

# Import optional C extension at toplevel, to help disabling it when testing
try:
import _uuid
except ImportError:
_uuid = None


def _load_system_functions():
"""
Try to load platform-specific functions for generating uuids.
Expand All @@ -500,15 +507,10 @@ def _load_system_functions():
# Assume that the uuid_generate functions are broken from 10.5 onward,
# the test can be adjusted when a later version is fixed.
pass
else:
try:
import _uuid
except ImportError:
pass
else:
_generate_time_safe = _uuid.generate_time_safe
_has_uuid_generate_time_safe = True
return
elif _uuid is not None:
_generate_time_safe = _uuid.generate_time_safe
_has_uuid_generate_time_safe = True
return

try:
# If we couldn't find an extension module, try ctypes to find
Expand All @@ -529,11 +531,11 @@ def _load_system_functions():
continue
# Try to find the safe variety first.
if hasattr(lib, 'uuid_generate_time_safe'):
_uuid_generate_time = lib.uuid_generate_time_safe
_uuid_generate_time_safe = lib.uuid_generate_time_safe
# int uuid_generate_time_safe(uuid_t out);
def _generate_time_safe():
_buffer = ctypes.create_string_buffer(16)
res = _uuid_generate_time(_buffer)
res = _uuid_generate_time_safe(_buffer)
return bytes(_buffer.raw), res
_has_uuid_generate_time_safe = True
break
Expand Down
10 changes: 5 additions & 5 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1669,16 +1669,16 @@ class db_found(Exception): pass
missing.append('_tkinter')

# Build the _uuid module if possible
build_uuid = False
if find_file("uuid.h", inc_dirs, ["/usr/include/uuid"]):
uuid_incs = find_file("uuid.h", inc_dirs, ["/usr/include/uuid"])
if uuid_incs:
if self.compiler.find_library_file(lib_dirs, 'uuid'):

@vstinner vstinner Sep 28, 2017

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.

You may also store the result of find_library_file() into uuid_libdirs to pass it to Extension library_dirs. I'm not sure about this one.

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let me try...

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, it shouldn't be necessary. Other uses of find_library_file() don't do it.

@vstinner vstinner Sep 28, 2017

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.

Ok, fine.

uuid_libs = ['uuid']
else:
uuid_libs = []
build_uuid = True
if build_uuid:
if uuid_incs:
self.extensions.append(Extension('_uuid', ['_uuidmodule.c'],
libraries=uuid_libs))
libraries=uuid_libs,
include_dirs=uuid_incs))
else:
missing.append('_uuid')

Expand Down