GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid by pitrou · Pull Request #3796 · python/cpython

Conversation

@pitrou

@pitrou pitrou commented Sep 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue11063

https://bugs.python.org/issue20519

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pitrou
bpo-11063, bpo-20519: avoid ctypes and improve import time for uuid
32b1b40
@bedevere-bot bedevere-bot added the awaiting merge label Sep 28, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 28, 2017
@vstinner

vstinner commented Sep 28, 2017

Copy link
Copy Markdown
Member

Please add a NEWS entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pitrou

pitrou commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

The potential issue here is that the uuid headers are not always installed (for example they aren't on the Travis-CI machine, even though it has many development headers installed).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pitrou added 2 commits September 28, 2017 13:16
@pitrou
Fix _windll_getnode
05dcee2
@pitrou
Add blurb
f4d7cf3
@vstinner vstinner mentioned this pull request Sep 28, 2017
bpo-11063: Create _uuid1 submodule #3795
Closed
vstinner
vstinner requested changes Sep 28, 2017
View reviewed changes
Comment thread Modules/_uuidmodule.c
PyMODINIT_FUNC
PyInit__uuid(void)
{
assert(sizeof(uuid_t) == 16);

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

You might use Py_BUILD_ASSERT() to run the check at the compilation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Is sizeof() allowed in macros? Usually we have SIZEOF_* for that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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 don't know. In case of doubt, keep your assert(), it's fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_uuidmodule.c Outdated


static PyObject *
_uuid_generate_time_safe(void)

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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 suggest to add a py prefix: py_uuid_generate_time_safe().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Agreed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_uuidmodule.c Outdated
NULL,
-1,
uuid_methods,
NULL,

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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 remove trailing "NULL,". The compiler set them to 0 / NULL anyway.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Yup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_uuidmodule.c Outdated
"_uuid",
NULL,
-1,
uuid_methods,

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

Can you use C99 style here to avoid surprising/boring "NULL," members?

.m_name = "_uuid",
.m_size = -1,
.m_methods = uuid_methods,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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, great, I'll do that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/uuid.py Outdated
else:
def _generate_time_safe():
return _uuid.generate_time_safe()
_has_uuid_generate_time_safe = True

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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 just _generate_time_safe = _uuid.generate_time_safe?

I would prefer an explicit "return" here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Hmm, ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_uuid.py

@unittest.skipUnless(c_uuid, 'requires the C _uuid module')
class TestInternalsWithExtModule(BaseTestInternals, unittest.TestCase):
uuid = c_uuid

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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 don't see the point of testing all uuid.*getnode() functions with and without _uuid. Basically, it's the same test run twice, no?

Only getnode() and uuid1() should be tested with and without _uuid, no?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Only getnode() and uuid1() should be tested with and without _uuid, no?

Probably. I didn't want to modify the tests too much but I'll take a look.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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've changed my mind: I think all tests should be run both with and without the C _uuid extension module. It doesn't cost much to do so, and it's more future-proof.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Sep 28, 2017

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Sep 28, 2017
@vstinner

vstinner commented Sep 28, 2017

Copy link
Copy Markdown
Member

@pitrou: "The potential issue here is that the uuid headers are not always installed (for example they aren't on the Travis-CI machine, even though it has many development headers installed)."

That's fine. We have buildbots for further tests.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Sep 28, 2017
bpo-5885: fix slow uuid initialization #3684
Closed
pitrou added 3 commits September 28, 2017 18:17
@pitrou
Nits in uuid.py
8f4dfb2
@pitrou
Generalize testing with and without C extension "_uuid"
a78dc43
@pitrou
Fix tests on Windows
34df739
@pitrou

pitrou commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

@Haypo, I think I've addressed your comments.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Sep 28, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

Ah, the new PR now looks better! I hope that it's my last round of minor comments :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_uuidmodule.c
PyModuleDef_HEAD_INIT,
.m_name = "_uuid",
.m_size = -1,
.m_methods = uuid_methods,

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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, I really like this syntax. We should use it more often!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread setup.py Outdated

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

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

Since you add /usr/include/uuid to inc_dirs, I would maybe store find_file() result into uuid_incdirs and then pass it to Extension(): inc_dirs=uuid_incdirs? Similar code is used for ssl_incs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread setup.py
# Build the _uuid module if possible
build_uuid = False
if find_file("uuid.h", inc_dirs, ["/usr/include/uuid"]):
if self.compiler.find_library_file(lib_dirs, 'uuid'):

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

Let me try...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

Ok, fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_uuid.py

class TestUUID(unittest.TestCase):
class BaseTestUUID:

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

Please add "uuid = None" here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_uuid.py
uuid = c_uuid


class BaseTestInternals:

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

Please add "uuid = None" here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/uuid.py Outdated
continue
# Try to find the safe variety first.
if hasattr(lib, 'uuid_generate_time_safe'):
_uuid_generate_time = lib.uuid_generate_time_safe

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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.

Would you mind to rename "_uuid_generate_time" to "_uuid_generate_time_safe"? I was confused while reading the code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/uuid.py
if _has_uuid_generate_time_safe is not None:
return

_has_uuid_generate_time_safe = False

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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 of the value of _has_uuid_generate_time_safe. The exact value (True or False) is not use. Maybe rename the flag to _system_functions_initialized or something like that, and only use False/True values?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Sep 28, 2017

Copy link
Copy Markdown
Member Author

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 used in the tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

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, I didn't notice. In this case, it's fine. I was just surprised to see 3 states where only 2 states are used in uuid.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pitrou
Address review comments
92b8670
@vstinner

vstinner commented Sep 28, 2017

Copy link
Copy Markdown
Member

I hacked the code to disable _uuid, uuid_generate_time_safe and uuid_generate_time in uuid.py. With these changes, tests fail:

======================================================================
ERROR: test_unix_getnode (test.test_uuid.TestInternalsWithExtModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/haypo/prog/python/master/Lib/test/test_uuid.py", line 574, in test_unix_getnode
    self.check_node(self.uuid._unix_getnode(), 'unix')
  File "/home/haypo/prog/python/master/Lib/uuid.py", line 578, in _unix_getnode
    uuid_time, _ = _generate_time_safe()
TypeError: 'NoneType' object is not callable

======================================================================
ERROR: test_unix_getnode (test.test_uuid.TestInternalsWithoutExtModule)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/haypo/prog/python/master/Lib/test/test_uuid.py", line 574, in test_unix_getnode
    self.check_node(self.uuid._unix_getnode(), 'unix')
  File "/home/haypo/prog/python/master/Lib/uuid.py", line 578, in _unix_getnode
    uuid_time, _ = _generate_time_safe()
TypeError: 'NoneType' object is not callable

Python 3.6 test contains:

        try: # Issues 1481, 3581: _uuid_generate_time() might be None.
            node = uuid._unixdll_getnode()
        except TypeError:
            self.skipTest('requires uuid_generate_time')
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pitrou

pitrou commented Sep 28, 2017

Copy link
Copy Markdown
Member Author

You're right, I'm gonna reinstate the try / except clause.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Sep 28, 2017
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

LGTM as soon as you fix the test failure I reported in my latest comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Sep 28, 2017
@pitrou
Fix test regression
88bb338
@vstinner vstinner mentioned this pull request Sep 28, 2017
bpo-29729: uuid.UUID now accepts bytes-like object #3801
Closed
@pitrou
pitrou merged commit a106aec into python:master Sep 28, 2017
@pitrou
pitrou deleted the uuid_ext_module branch September 28, 2017 21:03
@Mariatta Mariatta removed the awaiting merge label Oct 8, 2017
@igalic

igalic commented Oct 21, 2017

Copy link
Copy Markdown
Contributor

Is it possible to get this back-ported into (i guess, at least) 3.6?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Oct 21, 2017 via email

Copy link
Copy Markdown
Member
No, sorry, it's not possible. The change is too big to be qualified as a bugfix. For me, it's a new feature and so cannot be backported. The risk of regression is too high. The best you can do is to write a 3rd party extension on PyPI to backport the module to 3.6 and maybe older (2.7??).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adamantike adamantike mentioned this pull request Aug 4, 2019
import redis slow on FreeBSD / Python3.6 / related to uuid redis/redis-py#1193
Closed
@sobolevn sobolevn mentioned this pull request Dec 20, 2023
uuid has a bunch of deprecated functions: let's decide what to do with them #113308
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@pitrou @vstinner @bedevere-bot @igalic @Mariatta @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.