Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
pitrou merged 8 commits intopython:masterfrompitrou:uuid_ext_module
Sep 28, 2017

Conversation

@pitrou
Copy link
Member

@pitroupitrou commentedSep 28, 2017•
edited
Loading

@vstinner
Copy link
Member

Please add a NEWS entry.

@pitrou
Copy link
MemberAuthor

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

PyMODINIT_FUNC
PyInit__uuid(void)
{
assert(sizeof(uuid_t)==16);
Copy link
Member

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Issizeof() allowed in macros? Usually we haveSIZEOF_* for that.

Copy link
Member

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.



staticPyObject*
_uuid_generate_time_safe(void)
Copy link
Member

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Agreed.

NULL,
-1,
uuid_methods,
NULL,
Copy link
Member

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yup.

"_uuid",
NULL,
-1,
uuid_methods,
Copy link
Member

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,

Copy link
MemberAuthor

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.

Lib/uuid.py Outdated
else:
def_generate_time_safe():
return_uuid.generate_time_safe()
_has_uuid_generate_time_safe=True
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 just _generate_time_safe = _uuid.generate_time_safe?

I would prefer an explicit "return" here.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, ok.


@unittest.skipUnless(c_uuid,'requires the C _uuid module')
classTestInternalsWithExtModule(BaseTestInternals,unittest.TestCase):
uuid=c_uuid
Copy link
Member

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?

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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.

@bedevere-bot
Copy link

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

@vstinner
Copy link
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.

@pitrou
Copy link
MemberAuthor

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

Copy link
Member

@vstinnervstinner left a 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 :-)

PyModuleDef_HEAD_INIT,
.m_name="_uuid",
.m_size=-1,
.m_methods=uuid_methods,
Copy link
Member

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!

setup.py Outdated

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ok.

# Build the _uuid module if possible
build_uuid=False
iffind_file("uuid.h",inc_dirs, ["/usr/include/uuid"]):
ifself.compiler.find_library_file(lib_dirs,'uuid'):
Copy link
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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Let me try...

Copy link
MemberAuthor

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 offind_library_file() don't do it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fine.


classTestUUID(unittest.TestCase):
classBaseTestUUID:

Copy link
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=c_uuid


classBaseTestInternals:
Copy link
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.

Lib/uuid.py Outdated
continue
# Try to find the safe variety first.
ifhasattr(lib,'uuid_generate_time_safe'):
_uuid_generate_time=lib.uuid_generate_time_safe
Copy link
Member

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh, sure.

if_has_uuid_generate_time_safeisnotNone:
return

_has_uuid_generate_time_safe=False
Copy link
Member

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?

Copy link
MemberAuthor

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.

Copy link
Member

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.

@vstinner
Copy link
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')

@pitrou
Copy link
MemberAuthor

You're right, I'm gonna reinstate thetry / except clause.

Copy link
Member

@vstinnervstinner left a 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.

@pitroupitrou merged commita106aec intopython:masterSep 28, 2017
@pitroupitrou deleted the uuid_ext_module branchSeptember 28, 2017 21:03
@igalic
Copy link
Contributor

Is it possible to get thisback-ported into (i guess, at least) 3.6?

@vstinner
Copy link
Member

vstinner commentedOct 21, 2017 via email

No, sorry, it's not possible. The change is too big to be qualified as abugfix. For me, it's a new feature and so cannot be backported. The risk ofregression is too high.The best you can do is to write a 3rd party extension on PyPI to backportthe module to 3.6 and maybe older (2.7??).

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

Reviewers

@vstinnervstinnervstinner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pitrou@vstinner@bedevere-bot@igalic@Mariatta@the-knights-who-say-ni
[8]ページ先頭
©2009-2025 Movatter.jp