Skip to content

bpo-11063: Fix _uuid module on macOS#3855

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:uuid_macos
Oct 2, 2017
Merged

bpo-11063: Fix _uuid module on macOS#3855
vstinner merged 1 commit intopython:masterfrom
vstinner:uuid_macos

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Oct 2, 2017 •
edited by bedevere-bot
Loading

On macOS, use uuid_generate_time() instead of
uuid_generate_time_safe() of libuuid, since uuid_generate_time_safe()
is not available.

https://bugs.python.org/issue11063

@vstinner vstinner requested a review from pitrou October 2, 2017 14:04
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Oct 2, 2017

I tested this PR manually on macOS Sierra: test_uuid pass, and is_safe attribute is unknown as expected.

macbook:master haypo$ ./python.exe -m test test_uuid 
Run tests sequentially
0:00:00 load avg: 0.75 [1/1] test_uuid
1 test OK.

Total duration: 337 ms
Tests result: SUCCESS

macbook:master haypo$ ./python.exe -c 'import uuid; print(uuid.uuid1().is_safe)'
SafeUUID.unknown

Note: I hacked _netstat_getnode() in Lib/uuid.py to add "-n" option to make tests faster, to prevent slow DNS resolution.

pitrou
pitrou reviewed Oct 2, 2017
View reviewed changes
Modules/_uuidmodule.c Outdated
Copy link
Copy Markdown
Member

@pitrou pitrou Oct 2, 2017

Choose a reason for hiding this comment

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

I'd prefer if you'd return the (out, None) tuple here.

Copy link
Copy Markdown
Member Author

@vstinner vstinner Oct 2, 2017

Choose a reason for hiding this comment

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

I prefer to have very thin wrapper for C functions, and handle differences at the Python level.

Copy link
Copy Markdown
Member

@pitrou pitrou Oct 2, 2017

Choose a reason for hiding this comment

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

IMO it only makes both the Python code and the C code more complicated.

Copy link
Copy Markdown
Member

@pitrou pitrou Oct 2, 2017

Choose a reason for hiding this comment

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

One of the annoyances with modifying uuid.py is that there are so different code paths to take care of.

pitrou
pitrou reviewed Oct 2, 2017
View reviewed changes
Modules/_uuidmodule.c Outdated
Copy link
Copy Markdown
Member

@pitrou pitrou Oct 2, 2017

Choose a reason for hiding this comment

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

There's no need to expose two different function names, IMO.

Copy link
Copy Markdown
Member Author

@vstinner vstinner Oct 2, 2017

Choose a reason for hiding this comment

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

There's no need to expose two different function names, IMO.

It's not the same API:

  • _uuid.generate_time_safe() returns (uuid, is_safe).
  • _uuid.generate_time() only returns the uuid

Copy link
Copy Markdown
Member

@pitrou pitrou Oct 2, 2017

Choose a reason for hiding this comment

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

I'm proposing that it's the same API (see above) :-)

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Oct 2, 2017

I simplified my PR to not add a new function with a different name in _uuid. Instead, I modified uuid.py to decide if we have or not uuid_generate_time_safe() using sys.platform.

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Oct 2, 2017

Instead of checking sys.platform, maybe we could have a constant in _uuid?

@pitrou
Copy link
Copy Markdown
Member

pitrou commented Oct 2, 2017

Yes, I think it's better to put this in _uuid.

@vstinner
bpo-11063: Fix _uuid module on macOS
642f52d 
On macOS, use uuid_generate_time() instead of
uuid_generate_time_safe() of libuuid, since uuid_generate_time_safe()
is not available.
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Oct 2, 2017

Yes, I think it's better to put this in _uuid.

Ok, done. I added _uuid.has_uuid_generate_time_safe (int: 1 or 0).

@pitrou
Copy link
Copy Markdown
Member

pitrou commented Oct 2, 2017

Thank you! :-)

@warsaw
Copy link
Copy Markdown
Member

warsaw commented Oct 2, 2017

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.

@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Oct 2, 2017

@warsaw: "I'd rather see a configure check for ..."

Please, go ahead and write it :-) I began to look at AC_CHECK_HEADER() but I don't see how to pass /usr/include/uuid/ to ask to test also this directory. Then I wanted to use pkg-config, but @pitrou showed me that it doesn't exist on macOS.

In short, modifying the autotools are out of my skills. I proposed a pragmatic solution to fix the _uuid module on macOS.

@vstinner vstinner merged commit 4337a0d into python:master Oct 2, 2017
@vstinner vstinner deleted the uuid_macos branch October 2, 2017 14:58
@ned-deily
Copy link
Copy Markdown
Member

ned-deily commented Oct 2, 2017

I agree with @warsaw. This should be a configure check.

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

Reviewers

@pitrou pitrou pitrou approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vstinner @pitrou @warsaw @ned-deily @Mariatta @the-knights-who-say-ni @bedevere-bot