bpo-32399: Starting with AIX6.1 there is support in libc.a for uuid (RFC4122) #4974

aixtools · 2017-12-22T10:17:01Z

This patch provides the changes needed for this integration with the OS.

On AIX the base function is uuid_create() rather than uuid_generate_time()
The AIX uuid_t typedef is more aligned to the UUID field based definition
while the Linux typedef that is more aligned with UUID bytes
(or perhaps UUID bytes_le) definitions.

https://bugs.python.org/issue32399

This patch provides the changes needed for this integration with the OS. On AIX the base function is uuid_create() rather than uuid_generate_time() The AIX uuid_t typedef is more aligned to the UUID field based definition while the Linux typedef that is more aligned with UUID bytes (or perhaps UUID bytes_le) definitions.

and appropriate include macro for uuid.h are used.

…ines in _uuidmodules.c are missed. As _uuid module may fail, the tests say PASS - but actually, they failed.

pitrou

Thanks for the PR. A couple comments below.

pitrou · 2017-12-29T21:07:23Z

Modules/_uuidmodule.c

-    return Py_BuildValue("y#i", (const char *) out, sizeof(out), res);
+    res = uuid_generate_time_safe(uuid);
+    return Py_BuildValue("y#i", (const char *) uuid, sizeof(uuid), res);
+#elif HAVE_UUID_CREATE


Could you add a comment that this is AIX-specific?

pitrou · 2017-12-29T21:07:46Z

configure.ac

@@ -2692,6 +2695,17 @@ void *x = uuid_generate_time_safe
  [AC_MSG_RESULT(no)]
 )

+AC_MSG_CHECKING(for uuid_create)


Could you add a comment that this is AIX-specific?

pitrou · 2017-12-29T21:08:05Z

Modules/_uuidmodule.c

+    return Py_BuildValue("y#i", (const char *) uuid, sizeof(uuid), res);
+#elif HAVE_UUID_CREATE
+    unsigned32 status;
+    uuid_create(&uuid, &status);


You should check the return value of uuid_create, no?

bedevere-bot · 2017-12-29T21:09:46Z

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 have made the requested changes; please review again. 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.

aixtools · 2017-12-30T06:42:30Z

Regarding the return value checking. I could check that status != 0, but what should be done then. Could the argument ‘y#i’ and status included as an argument be used? I admit that I didn’t look for documentation on PyBuildValue()

…

Sent from my iPhone

On 29 Dec 2017, at 22:10, Antoine Pitrou ***@***.***> wrote: @pitrou requested changes on this pull request. Thanks for the PR. A couple comments below. In Modules/_uuidmodule.c: > int res; - res = uuid_generate_time_safe(out); - return Py_BuildValue("y#i", (const char *) out, sizeof(out), res); + res = uuid_generate_time_safe(uuid); + return Py_BuildValue("y#i", (const char *) uuid, sizeof(uuid), res); +#elif HAVE_UUID_CREATE Could you add a comment that this is AIX-specific? In configure.ac: > @@ -2692,6 +2695,17 @@ void *x = uuid_generate_time_safe [AC_MSG_RESULT(no)] ) +AC_MSG_CHECKING(for uuid_create) Could you add a comment that this is AIX-specific? In Modules/_uuidmodule.c: > int res; - res = uuid_generate_time_safe(out); - return Py_BuildValue("y#i", (const char *) out, sizeof(out), res); + res = uuid_generate_time_safe(uuid); + return Py_BuildValue("y#i", (const char *) uuid, sizeof(uuid), res); +#elif HAVE_UUID_CREATE + unsigned32 status; + uuid_create(&uuid, &status); You should check the return value of uuid_create, no? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

pitrou · 2017-12-30T11:15:40Z

If there is an error, you should probably return PyErr_SetFromErrno(PyExc_OSError). You can find examples of this in other modules in the Modules directory.

aixtools · 2017-12-30T12:04:51Z

I have made the requested changes; please review again.

bedevere-bot · 2017-12-30T12:04:53Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou

Thank you. Just a few more comments.

pitrou · 2017-12-30T12:19:21Z

configure

@@ -9125,7 +9125,7 @@ then
 			LDCXXSHARED='$(CXX) -shared'
 		else
 			LDSHARED='$(CC) -b'
-			LDCXXSHARED='$(CXX) -shared'
+			LDCXXSHARED='$(CXX) -b'


This doesn't look related.

Note those lines originate from #2519. Perhaps @datalogics-robb can comment on why LDCXXSHARED='$(CXX) -b' in configure.ac becomes LDCXXSHARED='$(CXX) -shared' in configure.

I looked at #2519 and it looks like the changes in configure were editted manually, rather than generated from configure.ac (my two cents).

If the line is suppossed to be with -shared, then configure.ac needs to be fixed. I do not know HPUX, so cannot help.

Sorry I've been AFK for a week, but yes this change looks good. -shared is a gcc/g++ flag and should definitely not be in a non-gcc clause.

pitrou · 2017-12-30T12:20:07Z

configure.ac

@@ -2681,6 +2681,10 @@ AC_CHECK_LIB(sendfile, sendfile)
 AC_CHECK_LIB(dl, dlopen)	# Dynamic linking for SunOS/Solaris and SYSV
 AC_CHECK_LIB(dld, shl_load)	# Dynamic linking for HP-UX

+# checks for uuid.h location
+dnl # AIX is at /usr/include/uuid.h


"dnl" isn't useful here, no?

pitrou · 2017-12-30T12:20:17Z

configure.ac

@@ -2692,6 +2696,18 @@ void *x = uuid_generate_time_safe
  [AC_MSG_RESULT(no)]
 )

+# AIX provides support for RFC4122 (uuid) in libc.a starring with AIX 6.1 (anno 2007)


Typo: "starting".

bedevere-bot · 2017-12-30T12:20:39Z

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 have made the requested changes; please review again. 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.

aixtools · 2017-12-30T20:37:58Z

On 30-Dec-17 13:21, Antoine Pitrou wrote: ***@***.**** requested changes on this pull request. Thank you. Just a few more comments. ------------------------------------------------------------------------ In configure <#4974 (comment)>: > @@ -9125,7 +9125,7 @@ then LDCXXSHARED='$(CXX) -shared' else LDSHARED='$(CC) -b' - LDCXXSHARED='$(CXX) -shared' + LDCXXSHARED='$(CXX) -b'

This is, I think, a side effect of the different aclocal.m4 that my autoreconf generates. I'll have to read the docs, I think they are equivalent. But I'll check.

This doesn't look related. ------------------------------------------------------------------------ In configure.ac <#4974 (comment)>: > @@ -2681,6 +2681,10 @@ AC_CHECK_LIB(sendfile, sendfile) AC_CHECK_LIB(dl, dlopen) # Dynamic linking for SunOS/Solaris and SYSV AC_CHECK_LIB(dld, shl_load) # Dynamic linking for HP-UX +# checks for uuid.h location +dnl # AIX is at /usr/include/uuid.h "dnl" isn't useful here, no?

I was think the comment about AIX was relevant for the source, less so for the generated file. I'll delete it as I reference AIX elsewhere.

------------------------------------------------------------------------ In configure.ac <#4974 (comment)>: > @@ -2692,6 +2696,18 @@ void *x = uuid_generate_time_safe [AC_MSG_RESULT(no)] ) +# AIX provides support for RFC4122 (uuid) in libc.a starring with AIX 6.1 (anno 2007) Typo: "starting".

fixed.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4974 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKWY9dpCNcDY6XqbmliH2X6dpkbiCMhrks5tFiqsgaJpZM4RK36h>.

pitrou · 2017-12-30T20:42:02Z

I was think the comment about AIX was relevant for the source, less so for the generated file.

I was tallking about the "dnl" at the beginning of the comment. I don't know much about m4, but it doesn't seem necessary, given how other comments are formatted.

aixtools · 2017-12-30T21:18:36Z

On 30-Dec-17 13:21, Antoine Pitrou wrote: ***@***.**** requested changes on this pull request. Thank you. Just a few more comments. ------------------------------------------------------------------------ In configure <#4974 (comment)>: > @@ -9125,7 +9125,7 @@ then LDCXXSHARED='$(CXX) -shared' else LDSHARED='$(CC) -b' - LDCXXSHARED='$(CXX) -shared' + LDCXXSHARED='$(CXX) -b' This doesn't look related.

imho - there is a commit error for configure, as configure.ac says: hp*|HP*) if test "$GCC" = "yes" ; then LDSHARED='$(CC) -shared' LDCXXSHARED='$(CXX) -shared' else LDSHARED='$(CC) -b' LDCXXSHARED='$(CXX) -b' fi ;; Darwin/1.3*) LDSHARED='$(CC) -bundle' so my output is, as far as I can determine, correct.

…

------------------------------------------------------------------------ In configure.ac <#4974 (comment)>: > @@ -2681,6 +2681,10 @@ AC_CHECK_LIB(sendfile, sendfile) AC_CHECK_LIB(dl, dlopen) # Dynamic linking for SunOS/Solaris and SYSV AC_CHECK_LIB(dld, shl_load) # Dynamic linking for HP-UX +# checks for uuid.h location +dnl # AIX is at /usr/include/uuid.h "dnl" isn't useful here, no? ------------------------------------------------------------------------ In configure.ac <#4974 (comment)>: > @@ -2692,6 +2696,18 @@ void *x = uuid_generate_time_safe [AC_MSG_RESULT(no)] ) +# AIX provides support for RFC4122 (uuid) in libc.a starring with AIX 6.1 (anno 2007) Typo: "starting". — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4974 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKWY9dpCNcDY6XqbmliH2X6dpkbiCMhrks5tFiqsgaJpZM4RK36h>.

aixtools · 2017-12-30T21:22:40Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-30T21:22:42Z

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou

Ok, this looks good to me. I'm gonna merge once the CI is happy.

pitrou · 2017-12-30T21:39:32Z

Thank you @aixtools !

the-knights-who-say-ni added the CLA signed label Dec 22, 2017

bedevere-bot added the awaiting review label Dec 22, 2017

aixtools added 5 commits December 22, 2017 12:38

add blurb

e49eef0

Modify _uuidmodule.c and configure.ac so that the uuid() routines

a471c96

and appropriate include macro for uuid.h are used.

Seems autoreconf is not run for integration testing, so the macro def…

93101c8

…ines in _uuidmodules.c are missed. As _uuid module may fail, the tests say PASS - but actually, they failed.

textual change

62d142d

revert to previous aclocal.m4

938ce02

pitrou requested changes Dec 29, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 29, 2017

modifications per reviewer request

7740fc6

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 30, 2017

pitrou requested changes Dec 30, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Dec 30, 2017

modifications per reviewer request

c933229

bedevere-bot removed the awaiting changes label Dec 30, 2017

bedevere-bot added the awaiting change review label Dec 30, 2017

pitrou approved these changes Dec 30, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 30, 2017

pitrou merged commit 0d3ccb4 into python:master Dec 30, 2017

bedevere-bot removed the awaiting merge label Dec 30, 2017

aixtools deleted the bpo-32399 branch January 8, 2018 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32399: Starting with AIX6.1 there is support in libc.a for uuid (RFC4122) #4974

bpo-32399: Starting with AIX6.1 there is support in libc.a for uuid (RFC4122) #4974

aixtools commented Dec 22, 2017 •

edited

Loading

pitrou left a comment

pitrou Dec 29, 2017

pitrou Dec 29, 2017

pitrou Dec 29, 2017

bedevere-bot commented Dec 29, 2017

aixtools commented Dec 30, 2017 via email

pitrou commented Dec 30, 2017

aixtools commented Dec 30, 2017

bedevere-bot commented Dec 30, 2017

pitrou left a comment

pitrou Dec 30, 2017

pitrou Dec 30, 2017

aixtools Dec 30, 2017

datalogics-robb Jan 2, 2018

pitrou Dec 30, 2017

pitrou Dec 30, 2017

bedevere-bot commented Dec 30, 2017

aixtools commented Dec 30, 2017 via email

pitrou commented Dec 30, 2017

aixtools commented Dec 30, 2017 via email

aixtools commented Dec 30, 2017

bedevere-bot commented Dec 30, 2017

pitrou left a comment

pitrou commented Dec 30, 2017

bpo-32399: Starting with AIX6.1 there is support in libc.a for uuid (RFC4122) #4974

bpo-32399: Starting with AIX6.1 there is support in libc.a for uuid (RFC4122) #4974

Conversation

aixtools commented Dec 22, 2017 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 29, 2017

aixtools commented Dec 30, 2017 via email

pitrou commented Dec 30, 2017

aixtools commented Dec 30, 2017

bedevere-bot commented Dec 30, 2017

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 30, 2017

aixtools commented Dec 30, 2017 via email

pitrou commented Dec 30, 2017

aixtools commented Dec 30, 2017 via email

aixtools commented Dec 30, 2017

bedevere-bot commented Dec 30, 2017

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Dec 30, 2017

aixtools commented Dec 22, 2017 •

edited

Loading