◐ Shell
reader mode source ↗
Skip to content

bpo-28009: for AIX correct uuid.getnode() and add ctypes() based uuid._generate_time_safe#5183

Closed
aixtools wants to merge 8 commits into
python:masterfrom
aixtools:bpo-28009
Closed

bpo-28009: for AIX correct uuid.getnode() and add ctypes() based uuid._generate_time_safe#5183
aixtools wants to merge 8 commits into
python:masterfrom
aixtools:bpo-28009

Conversation

@aixtools

@aixtools aixtools commented Jan 14, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor
  • Add uuid._generate_time_safe using ctypes() from libc.uuid_create (for when _uuid is not available)
  • use the character '.' rather than ':' to separate the MAC ADDR values
    returned by 'netstat -i' command (AIX specific)
  • The commands arp and ifconfig do not return a local MAC address

https://bugs.python.org/issue28009

@aixtools
* in Lib/uuid.py define ctypes() based definition of …
7837745 
  util._generate_time_safe based on lib.uuid_create (for when _uuid is not available)
* use the character '.' rather than ':' to seperate the MAC ADDR values
  returned by 'netstat -i' command (AIX specific)
* The commands arp and ifconfig do not return a local MAC address
@aixtools
correct typo in News
f6d95f3
@aixtools

aixtools commented Feb 5, 2018

Copy link
Copy Markdown
Contributor Author

I wanted this in:
michael@x071:[/data/prj/python/git/python3-3.7]diff -u Lib/uuid.py ../uuid.py-3.8
--- Lib/uuid.py 2018-02-05 16:07:49 +0000
+++ ../uuid.py-3.8 2018-02-05 15:21:09 +0000
@@ -708,6 +708,8 @@
_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]

+_NODE_GETTERS_AIX = [_unix_getnode, _netstat_getnode]
+
def getnode(*, getters=None):
"""Get the hardware address as a 48-bit positive integer.

@@ -722,6 +724,8 @@

 if sys.platform == 'win32':
     getters = _NODE_GETTERS_WIN32
  • elif sys.platform.startswith("aix"):
  •    getters = _NODE_GETTERS_AIX
    else:
    getters = _NODE_GETTERS_UNIX

but continuously creates a merge conflict. C'est la vie.

native-api
native-api suggested changes May 17, 2018
View reviewed changes

@native-api native-api left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

As a general note, AIX is a niche system and should have the absolute possible minimum of code specific to it -- if anything, because the devteam can't test and thus maintain the correctness of that code. For the same reason, it should disrupt control flow the least amount possible -- otherwise, those untestable chunks will put limitations on further changes. Whenever possible, UNIX flavors should be distinguished by functional differences rather than names.

vstinner
vstinner reviewed Jun 4, 2018
View reviewed changes

@vstinner vstinner left a comment

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.

Hide comment

"define uuid._generate_time_safe in uuid.py based on ctypes() lib.uuid_create (for when _uuid is not available)"

When is _uuid not available? Why not fixing _uuid instead, if it has issues?

@vstinner vstinner requested a review from pitrou June 4, 2018 09:00
@aixtools

aixtools commented Jun 4, 2018 via email

Copy link
Copy Markdown
Contributor Author

@vstinner

vstinner commented Jun 4, 2018

Copy link
Copy Markdown
Member

_uuid is fixed in AIX 6.1 and later, but not in AIX 5.3 as AIX 5.3 does not have uuid_create() in libc.

What is available on AIX 5.3?

@aixtools

aixtools commented Jun 4, 2018 via email

Copy link
Copy Markdown
Contributor Author

@vstinner

vstinner commented Jun 4, 2018

Copy link
Copy Markdown
Member

I don't understand why ctypes is needed. You wrote that AIX 6.1 can use _uuid, but AIX 5.3 has no usable system uuid library. If AIX doesn't benefit of ctypes, would you mind to simplify your PR to only fix the AIX specific issue (the colon character issue)?

@aixtools

aixtools commented Jun 5, 2018 via email

Copy link
Copy Markdown
Contributor Author

@vstinner

vstinner commented Jun 5, 2018

Copy link
Copy Markdown
Member

"Yes, I can do that. However, I still hope you will consider backporting this - as it has been broken everywhere for a long time."

I don't understand well the AIX platform. This PR is very long and so hard to review. I suggested you to write a shorter PR so it should be simpler to get a review and fix the master branch (and 3.7 which also has _uuid).

Once master (and 3.7) will be fixed, we can start discussing how to fix other branches.

I don't like the idea of adding code to the master branch which is written for older branches. I prefer to not "add legacy code" to the master branch. What do you think of my rationale?

@aixtools

aixtools commented Jun 5, 2018 via email

Copy link
Copy Markdown
Contributor Author

@vstinner

vstinner commented Jun 5, 2018

Copy link
Copy Markdown
Member

You can reuse bpo-28009 to fix _uuid in master/3.7, but please create a different PR.

@aixtools

aixtools commented Jun 6, 2018 via email

Copy link
Copy Markdown
Contributor Author

@vstinner

vstinner commented Jun 6, 2018

Copy link
Copy Markdown
Member

The development starts in master, once it's stabilized, it's merged into 3.7, and then to 3.6 and maybe 2.7. I'm talking about the general case. I don't know until which version we should backport these enhancements/fixes.

@aixtools

aixtools commented Jun 6, 2018 via email

Copy link
Copy Markdown
Contributor Author

@aixtools

aixtools commented Jun 10, 2018

Copy link
Copy Markdown
Contributor Author

Will start a new PR now that _uuid is working in master

@aixtools aixtools closed this Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

@pitrou pitrou Awaiting requested review from pitrou

+1 more reviewer

@native-api native-api native-api requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aixtools @vstinner @native-api @the-knights-who-say-ni @bedevere-bot