bpo-26439 Fix ctypes.util.find_library failure on AIX #4507

aixtools · 2017-11-22T21:28:21Z

This patch implements the function find_library() for AIX.
By default it searches in AIX style archives first, and if None is found, repeats the search looking for a .so file instead. For example:
find_library("c") returns libc.a(shr.o)
while
find_library("rpm") returns librpm.so

If python is a 64-bit build, the routine knows to find the 64-bit members, e.g.,
find_library("c") returns libc.a(shr_64.o)

https://bugs.python.org/issue26439

the-knights-who-say-ni · 2017-11-22T21:28:23Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

aixtools · 2017-11-23T13:48:29Z

where do I add news and/or say "skip news".

Mariatta · 2017-11-23T14:41:58Z

This will need a news entry, and you can add it using blurb

aixtools · 2017-11-24T07:19:36Z

thx.

Too bad the file cannot just be added by using an editor. While packaging I do not have any python installed - so definitely in a chicken and egg circle here...

You can install blurb from PyPI using pip. Alternatively, simply add blurb to a directory on your path. blurb’s only dependency is Python 3.5+.

aixtools · 2017-11-24T07:20:25Z

p.s. how many days before CLA is reviewed again?

Mariatta · 2017-11-24T07:39:21Z

I rechecked CLA :)

If you don't have blurb, the News entry can be added manually:

Create a new file at Misc/NEWS.d/next/ .... containing the news entry. You'll need to read blurb readme about the file name requirement.
commit it

aixtools · 2017-11-24T08:08:19Z

thx for the CLA update.

I had already found the directory and the readme. will work on that today.

Update: installed python3-3.5.2 on AIX, pip installed blurb - seemed to be working - but blurb insists on calling git - so that failed (my git depends on python2 and I have not tested having python2 and python3 installed in parallel. Been working on that, but not tested it yet.

Looked at: PRETTY_NAME="Debian GNU/Linux stretch/sid" (on power)

python3.5.1 - but no pip, not even via apt install pip
centos1611 - no python3 - and the latest python2 is 2.7.5 (really sad!)

So, when I am back home I'll start testing parallel python installs - as I do not see how I can fake a random number, or is it paired to a git commit number, etc..

Here is the text I would like to add - in case someone can tell me what the file name should be.

fyi: I also have an update to Modules/_ctypes/_ctypes.c (so that import RTLD_MEMBER from _ctypes would work) - and configure.ac so that a "HAVE_RTLD_MEMBER" gets defined (not perfect define name, but close enough I hope for discussion).

Shall I push those updates now - or wait until the blurb is also ready?

aixtools · 2017-11-26T18:31:30Z

Little bit amazing to me that removing two words from the NEWS does not complete after two hours - while with the error (finding it took <3 minutes) the 3 other tests took about 30 minutes.

aixtools · 2017-11-28T17:33:05Z

I just remembered - something I had originally worked in, but had to remove, was looking at LD_LIBRARY_PATH and LIBPATH. Need to add that back in. Update to come asap.

aixtools · 2017-11-28T22:06:12Z

travis ended with this error: oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded - so I hope someone will cause a repeat for the pass (expected).

I have tested localally in both 32-bit and 64-bit mode. The 64-bit mode returns. as expected:
michael@x071:[/data/prj/python/git/python-3.7.0.1]./python Lib/ctypes/util.py
None
libc.a(shr_64.o)
libbz2.a(libbz2.so.1)
<CDLL 'libc.a(shr_64.o)', handle d at 0x7000000001e7a20>
<CDLL 'libc.a(shr_64.o)', handle e at 0x7000000001e7a20>

aixtools · 2017-11-29T16:25:33Z

Could someone please get the tests to re-run. The failure was established in one second - due to lack of disk space - nothing related to code.

Mariatta · 2017-11-29T17:01:42Z

I'll close and re-open the PR, it'll trigger the CI.

aixtools · 2017-11-29T17:59:19Z

thx. so, learning to wait for queues :)

(update: and now that the tests are passed - wait for someone to accept assignment and review!)

vstinner

Enjoy a first round of review ;-)

The overall change LGTM, but I have sent a long list of requests ;-)

vstinner · 2017-11-30T14:45:51Z

Lib/ctypes/_aix.py

+    """
+    return sizeof(c_void_p) * 8
+
+def get_dumpH(file):


instead of "H", would you mind to replace it with the meaning of the option?

vstinner · 2017-11-30T14:46:04Z

Lib/ctypes/_aix.py

+from ctypes._util import _last_version
+from subprocess import Popen, PIPE
+
+def aixABI():


PEP 8 suggests the name: aix_abi()

vstinner · 2017-11-30T14:47:17Z

Lib/ctypes/_aix.py

+
+def aixABI():
+    """Internal support function:
+    return executable size - 32-bit, or 64-bit


"return executable size - 32-bit, or 64-bit"

I propose: "Get the executable bits: return 32 or 64"

vstinner · 2017-11-30T14:47:42Z

Lib/ctypes/_aix.py

+def aixABI():
+    """Internal support function:
+    return executable size - 32-bit, or 64-bit
+    This is vital to the search for suitable member in an archive


"This is vital to the search for suitable member in an archive"

I don't think that this sentence is useful. At soon as a function is used, it's useful ;-)

vstinner · 2017-11-30T14:48:25Z

Lib/ctypes/_aix.py

+        objects.append((object, get_info(p)))
+
+    p.stdout.close()
+    p.wait()


Please use "with p: ..." to automatically close stdout on error.

You may raise an exception if p.returncode is non-zero.

Again, this was based on recommendation from someone else. I'll reread the documentation. I hope
with Popen(....) as p:

is what you mean

If p.returncode is non-zero then there is 'nothing to find' and a return value of None is still ok.

vstinner · 2017-11-30T15:05:05Z

Lib/ctypes/util.py

+        # issue-26439 - fix broken test call for AIX
+        elif sys.platform.startswith("aix"):
+            from ctypes import CDLL
+            RTLD_MEMBER =  0x00040000


Please use os.RTDL_MEMBER instead of an hardcoded constant.

vstinner · 2017-11-30T15:06:04Z

Misc/NEWS.d/next/Library/2017-11-24-08-35-43.bpo-26439.IC45_f.rst

@@ -0,0 +1,10 @@
+Fix ctypes.util.find_library() for AIX


I'm not sure of this summary. In short, I understand that you implemented find_library() on AIX, not just "fixed".

Maybe it just needs to be implemented - I take that to be the "reviews" position.

However, ctypes.util.find_library() does exist for AIX - asis. And, this is broken.
The documentation makes clear that the behavior of find_library is platform-dependent.

So if you object to saying that this patch 'fixes' the published (aka existing feature of ctypes.util) because this patch 'implements' (as ctypes._aix.find_library(), just as "darwin" implements it as ctypes.macholib.dyld.dyld_find() (this was actually what I tried to emulate, just without a new sub-directory, and per much earlier reviews renamed it ctypes._aix to make clear it is not something to be imported automagically. Again - it is a platform dependency (and your suggestion to just import it into the namespace is excellent. Should have thought of that long ago.)

In closing - I see ctypes.util.find_library() as the API definition, and the current implementation is broken. This patch replaces that implementation with a new one - and this 'fixes' the API for AIX.
If the reviewers feel that the word 'fixed' is misplaced - I'll delete the line. Or, could/should I say I fixed the AIX implementation of find_library().

vstinner · 2017-11-30T15:06:23Z

Misc/NEWS.d/next/Library/2017-11-24-08-35-43.bpo-26439.IC45_f.rst

+while legacy names e.g., find_library("c") returns libc.a(shr.o)
+or libc.a(shr_64.o) - depending on 32 or 64-bit operations.
+Include RTLD_MEMBER to mode to support AIX legacy library(member) names
+(Modules/_ctype/_ctype.c), ctypes/__init__.py and configure.ac)


Please credit yourself: add "Patch by ..." (your full name).

vstinner · 2017-11-30T15:07:46Z

Modules/_ctypes/_ctypes.c

+#endif
+#if HAVE_DECL_RTLD_NOW
+    PyModule_AddObject(m, "RTLD_NOW", PyLong_FromLong(RTLD_NOW));
+#endif


Please move this to Modules/posixmodule.c. We already have many os.RTLD_* constants there... like os.RTLD_NOW ;-)

ok. as above. In my previous convos about this - they were being put into _ctypes - so I just used that convention.

vstinner · 2017-11-30T15:08:37Z

Lib/ctypes/__init__.py

@@ -338,6 +338,16 @@ def __init__(self, name, mode=DEFAULT_MODE, handle=None,
            flags |= _FUNCFLAG_USE_ERRNO
        if use_last_error:
            flags |= _FUNCFLAG_USE_LASTERROR
+        if _sys.platform.startswith("aix"):
+            """When the name contains ".a(" and ends with ")",
+               asin "libFOO.a(libFOO.so)" this is taken to be an


asin: typo for as in?

Yes, I frequently type "asin" as a single world in presentations I give. I'll change it to two words here.

and call the routines by name, e.g., re.search, re.match, re.escape

vadmium · 2017-12-03T22:24:17Z

Lib/ctypes/test/test_loading.py

+        if (sys.maxsize < 2**32):
+            libc_name = "libc.a(shr.o)"
+        else:
+            libc_name = "libc.a(shr_64.o)"


Is this really needed? Won’t your find_library("c") implementation find these?

vadmium · 2017-12-03T22:53:46Z

Lib/ctypes/_aix.py

+        universal_newlines=True, stdout=PIPE, stderr=DEVNULL) as p:
+            ld_header = get_ld_header(p)
+            if ld_header:
+                ldr_headers.append((ld_header, get_ld_header_info(p)))


Since you now only return one item, would it be better to just return the tuple or None, without building a list?

if ld_header: return (ld_header, get_ld_header_info(p)) else: return None

vadmium · 2017-12-03T23:09:06Z

Lib/ctypes/_aix.py

+
+def get_shared(ld_headers):
+    """
+    extract a the shareable objects from ld_headers


Extract the . . . [drop a]

vadmium · 2017-12-03T23:13:48Z

Lib/ctypes/_aix.py

+    return libpaths
+
+def find_library(name):
+    """AIX implemantation of ctypes.util.find_library()


Spelling: implementation

vadmium · 2017-12-03T23:22:37Z

Lib/ctypes/util.py

@@ -80,6 +81,14 @@ def find_library(name):
                continue
        return None

+if sys.platform.startswith("aix"):
+    # find .so members in .a files
+    # using dump loader header information + sys.


In the comment “using dump loader header information + sys”, I did not realize that you are referring to the sys Python module, nor the sys.platform string. I don’t understand how you can use sys.platform “to find members in files”. You are only using it to determine if the code is called on AIX.

vadmium · 2017-12-03T23:32:36Z

Lib/ctypes/util.py

+            from ctypes import CDLL
+            RTLD_MEMBER =  0x00040000
+            # print("crypto\t:: %s" % cdll.LoadLibrary(find_library("crypto")))
+            if (sys.maxsize < 2**32):


It would be easier to read (IMO) without any kind of brackets. I removed them when I modified your patch (https://bugs.python.org/review/26439/diff2/18236:18714/Lib/ctypes/util.py), and they stayed removed in your updated aix-library.161004.patch. See also test_loading.py.

vadmium · 2017-12-03T23:37:22Z

Lib/ctypes/_aix.py

-rather than as separate, detailed if: sections in utils.py
+"""
+Lib/ctypes.util.find_library() support for AIX
+Similar approach as done for Darwin support by using seperate files


vadmium · 2017-12-03T23:38:47Z

Lib/ctypes/_aix.py

+2.9 SVR4 linking affinity
+Nowadays, there are two major object file formats used by the operating systems:
+XCOFF: The COFF enhanced by IBM and others. The original COFF (Common
+Object FIle Format) was the base of SVR3 and BSD 4.2 systems.


Lowercase i in File?

vadmium · 2017-12-03T23:41:42Z

Lib/ctypes/_aix.py

+library object is referred to as the "member".
+
+For dlopen() on AIX (read initAndLoad()) the calls are similiar.
+Default activity is achived by not providing any path information. When path


Spelling: achieved (or archived?)

vadmium · 2017-12-03T23:45:25Z

Lib/ctypes/_aix.py

+
+For dlopen() on AIX (read initAndLoad()) the calls are similiar.
+Default activity is achived by not providing any path information. When path
+information is provided dlopen() does not search any alturnate directories.


Spelling: alternate (or to avoid the Americanism, alternative or other)

aixtools · 2017-12-06T21:10:17Z

On 04/12/2017 00:59, Martin Panter wrote: vadmium commented on this pull request. > @@ -13,6 +13,11 @@ def setUpModule(): libc_name = find_library("c") elif sys.platform == "cygwin": libc_name = "cygwin1.dll" + elif sys.platform.startswith("aix"): + if (sys.maxsize < 2**32): + libc_name = "libc.a(shr.o)" + else: + libc_name = "libc.a(shr_64.o)" Is this really needed? Won’t your find_library("c") implementation find these?

I did not realize the reason for the cygwin line was because find_library("c") was not working. I thought it was some self-documentation of alternate platform.

> + # these lines start with a digit + info = [] + for line in p.stdout: + if re.match("[0-9]", line): + info.append(line) + else: + # Should be a blank separator line, safe to consume + break + return info + + ldr_headers = [] + with Popen(["/usr/bin/dump", "-X%s" % aix_abi(), "-H", file], + universal_newlines=True, stdout=PIPE, stderr=DEVNULL) as p: + ld_header = get_ld_header(p) + if ld_header: + ldr_headers.append((ld_header, get_ld_header_info(p))) Since you now only return one item, would it be better to just return the tuple or None, without building a list?

Maybe if I knew in advance. Some archives have multiple entries. This is the 'processing' of the dump -Xnn -H. At least I hope it is reading the file to the end. Using 'with .... as p:' I assume it is still reading to the end of file. This is supposed to be a better way to assure p.close is called. I am just following the reviewer. e.g., A favorite of mine because it is yet another example of how people changed their mind on what to call a 64-bit member. (FOO64.so, FOO_64.o, or FOO.so) root@x071:[/root]dump -X64 -H /usr/lib/libcrypto.a /usr/lib/libcrypto.a[libcrypto64.so]: ***Loader Section*** Loader Header Information VERSION# #SYMtableENT #RELOCent LENidSTR 0x00000001 0x0000103f 0x00004a28 0x00000021 #IMPfilID OFFidSTR LENstrTBL OFFstrTBL 0x00000002 0x000628a0 0x0001560b 0x000628c1 ***Import File Strings*** INDEX PATH BASE MEMBER 0 /usr/lib:/lib 1 libc.a shr_64.o /usr/lib/libcrypto.a[libcrypto64.so.1.0.0]: ***Loader Section*** Loader Header Information VERSION# #SYMtableENT #RELOCent LENidSTR 0x00000001 0x0000103f 0x00004a28 0x00000021 #IMPfilID OFFidSTR LENstrTBL OFFstrTBL 0x00000002 0x000628a0 0x0001560b 0x000628c1 ***Import File Strings*** INDEX PATH BASE MEMBER 0 /usr/lib:/lib 1 libc.a shr_64.o /usr/lib/libcrypto.a[libcrypto64.so.0.9.8]: ***Loader Section*** Loader Header Information VERSION# #SYMtableENT #RELOCent LENidSTR 0x00000001 0x00000c47 0x0000430d 0x0000003e #IMPfilID OFFidSTR LENstrTBL OFFstrTBL 0x00000003 0x000557b0 0x0000f9e7 0x000557ee ***Import File Strings*** INDEX PATH BASE MEMBER 0 /usr/lib:/lib 1 libc.a shr_64.o 2 libpthreads.a shr_xpg5_64.o

``` python if ld_header: return (ld_header, get_ld_header_info(p)) else: return None ``` > + else: + # Should be a blank separator line, safe to consume + break + return info + + ldr_headers = [] + with Popen(["/usr/bin/dump", "-X%s" % aix_abi(), "-H", file], + universal_newlines=True, stdout=PIPE, stderr=DEVNULL) as p: + ld_header = get_ld_header(p) + if ld_header: + ldr_headers.append((ld_header, get_ld_header_info(p))) + return ldr_headers + +def get_shared(ld_headers): + """ + extract a the shareable objects from ld_headers Extract the . . . [drop _a_]

check

> + libpaths = environ.get("LIBPATH") + if libpaths is None: + libpaths = [] + else: + libpaths = libpaths.split(":") + objects = get_ld_headers(executable) + for (_, lines) in objects: + for line in lines: + # the second (optional) argument is PATH if it includes a / + path = line.split()[1] + if "/" in path: + libpaths.extend(path.split(":")) + return libpaths + +def find_library(name): + """AIX implemantation of ctypes.util.find_library() Spelling: _implementation_

check.

> @@ -80,6 +81,14 @@ def find_library(name): continue return None +if sys.platform.startswith("aix"): + # find .so members in .a files + # using dump loader header information + sys. In the comment “using dump loader header information + sys”, I did not realize that you are referring to the _sys_ Python module, nor the _sys.platform_ string. I don’t understand how you can use _sys.platform_ “to find members in files”. You are only using it to determine if the code is called on AIX.

Now I understand your comment. I'll think about a clarification.

> @@ -324,6 +322,20 @@ def test(): print(cdll.LoadLibrary("libcrypto.dylib")) print(cdll.LoadLibrary("libSystem.dylib")) print(cdll.LoadLibrary("System.framework/System")) + # issue-26439 - fix broken test call for AIX + elif sys.platform.startswith("aix"): + from ctypes import CDLL + RTLD_MEMBER = 0x00040000 + # print("crypto\t:: %s" % cdll.LoadLibrary(find_library("crypto"))) + if (sys.maxsize < 2**32): It would be easier to read (IMO) without any kind of brackets. I removed them when I modified your patch (https://bugs.python.org/review/26439/diff2/18236:18714/Lib/ctypes/util.py), and they stayed removed in your updated aix-library.161004.patch. See also test_loading.py.

As I had to start over again - guessed I missed that change. Will look again. Thx.

> @@ -1,19 +1,54 @@ -"""Lib/ctypes support for LoadLibrary interface to dlopen() for AIX -Similar kind of support (i.e., as a separate file) -as has been done for Darwin support ctypes.macholib.* -rather than as separate, detailed if: sections in utils.py +""" +Lib/ctypes.util.find_library() support for AIX +Similar approach as done for Darwin support by using seperate files separate

My muscles - remember wrong. Thx.

> dlopen() is an interface to AIX initAndLoad() - primary documentation at: https://www.ibm.com/support/knowledgecenter/en/ssw_aix_61/com.ibm.aix.basetrf1/dlopen.htm https://www.ibm.com/support/knowledgecenter/en/ssw_aix_61/com.ibm.aix.basetrf1/load.htm + +AIX supports two styles for dlopen(): svr4 (System V Release 4) which is common on posix +platforms, but also a BSD style - aka SVR3. + +From AIX 5.3 Difference Addendum (December 2004) +2.9 SVR4 linking affinity +Nowadays, there are two major object file formats used by the operating systems: +XCOFF: The COFF enhanced by IBM and others. The original COFF (Common +Object FIle Format) was the base of SVR3 and BSD 4.2 systems. Lowercase _i_ in _File_?

Yes, should be lowercase _i_ (COFF, not COFIF)

> +While the shared library content is identical on AIX - one is located as a filepath name +(svr4 style) and the other is located as a member of an archive (and the archive +is located as a filepath name). + +The key difference arises when supporting multiple abi formats (i.e., 32 and 64 bit). +For svr4 either only one ABI is supported, or there are two directories, or there +are different file names. The most common solution for multiple ABI is multiple +directories. + +For the XCOFF (aka AIX) style - one directory (one archive file) is sufficient +as multiple shared libraries can be in the archive - even sharing the same name. +In documentation the archive is also referred to as the "base" and the shared +library object is referred to as the "member". + +For dlopen() on AIX (read initAndLoad()) the calls are similiar. +Default activity is achived by not providing any path information. When path Spelling: _achieved_ (or _archived_?)

achieved - maybe either performed or executed is better yet.

> +(svr4 style) and the other is located as a member of an archive (and the archive +is located as a filepath name). + +The key difference arises when supporting multiple abi formats (i.e., 32 and 64 bit). +For svr4 either only one ABI is supported, or there are two directories, or there +are different file names. The most common solution for multiple ABI is multiple +directories. + +For the XCOFF (aka AIX) style - one directory (one archive file) is sufficient +as multiple shared libraries can be in the archive - even sharing the same name. +In documentation the archive is also referred to as the "base" and the shared +library object is referred to as the "member". + +For dlopen() on AIX (read initAndLoad()) the calls are similiar. +Default activity is achived by not providing any path information. When path +information is provided dlopen() does not search any alturnate directories. Spelling: _alternate_ (or to avoid the Americanism, _alternative_ or _other_)

"other" may be best.

vadmium · 2017-12-06T21:57:38Z

On 07/12/2017, Michael Felt ***@***.***> wrote: > + with Popen(["/usr/bin/dump", "-X%s" % aix_abi(), "-H", file], > + universal_newlines=True, stdout=PIPE, stderr=DEVNULL) as p: > + ld_header = get_ld_header(p) > + if ld_header: > + ldr_headers.append((ld_header, get_ld_header_info(p))) At least I hope it is reading the file to the end. Using 'with .... as p:' I assume it is still reading to the end of file. This is supposed to be a better way to assure p.close is called. I am just following the reviewer.

The "with" statement is a nice way to ensure the pipe is closed and the child is waited for, although it is not supported by Popen in Python 2. However the "with" statement is not a loop, so the code won't necessarily read until EOF. If you still need to return multiple items, you may have to put the "while True" loop back in. Also, if practical I would recommend adding a test case that fails if you only return the first item from "get_info".

aixtools · 2017-12-07T07:46:18Z

On 04/12/2017 00:59, Martin Panter wrote: Since you now only return one item, would it be better to just return the tuple or None, without building a list? ``` python if ld_header: return (ld_header, get_ld_header_info(p)) else: return None ```

I understand the question/review better. I believe I was returning a list because: a) it was new to me b) unsure if the answer was always going to be zero or one item. I wanted to accommodate "any number". c) there have been many suggestions on how to improve this (see a above) and I am losing focus d) I'll test drive it - later - but even it works now, I am not sure for the future. I did the research for this well over a year ago.

aixtools · 2017-12-07T09:44:54Z

On 30/11/2017 16:11, Victor Stinner wrote: + break + return info + + p = Popen(["/usr/bin/dump", "-X%s" % aixABI(), "-H", file], + universal_newlines=True, stdout=PIPE) + + objects = [] + while True: + object = get_object(p) + if object is None: + break + objects.append((object, get_info(p))) + + p.stdout.close() + p.wait() Please use "with p: ..." to automatically close stdout on error.

Restored this (with variable name changes) - as it only reads the first entry - not to the end-of-file.

aixtools · 2017-12-07T09:59:35Z

On 06/12/2017 22:57, Martin Panter wrote: The "with" statement is a nice way to ensure the pipe is closed and the child is waited for, although it is not supported by Popen in Python 2. However the "with" statement is not a loop, so the code won't necessarily read until EOF. If you still need to return multiple items, you may have to put the "while True" loop back in.

I returned to the old code - as it was missing members - and would make getting the latest version (if applicable) impossible. Also carried a major assumption that the first shareable member in the archive is the correct member.

Also, if practical I would recommend adding a test case that fails if you only return the first item from "get_info".

There might be only one "info" line. So, I do not understand the need to test for only one line item.

aixtools · 2017-12-07T10:08:06Z

On 02/12/2017 06:02, Martin Panter wrote: + # issue-26439 - fix broken test call for AIX + elif sys.platform.startswith("aix"): + from ctypes import CDLL + RTLD_MEMBER = 0x00040000 + # print("crypto\t:: %s" % cdll.LoadLibrary(find_library("crypto"))) + if (sys.maxsize < 2**32): Round brackets have re-appeared

Ah, yes - very very old C programmer. Always using brackets. I have restored the "crypto" test, as it is one of the special cases for 64-bit. While these added tests are useful to me for testing behavior, they do not all need to remain. I do want to point out the logic for tests such as: When/if the change in __init__.py is not added, the first test succeeds, but the second will fail. print(Using CDLL(name, RTLD_MEMBER): %s" % CDLL("libc.a(shr.o)", RTLD_MEMBER)) print("Using cdll.LoadLibrary(): %s" % cdll.LoadLibrary("libc.a(shr.o)")) FYI: using 64-bit and __init__.py updated I am getting: michael@x071:[/data/prj/python/git/python-3.7.0.1]./python Lib/ctypes/util.py Lib/ctypes/_aix.py None libc.a(shr_64.o) libbz2.a(libbz2.so.1) Using CDLL(name, RTLD_MEMBER): <CDLL 'libc.a(shr_64.o)', handle e at 0x7000000001ebac8> Using cdll.LoadLibrary(): <CDLL 'libc.a(shr_64.o)', handle f at 0x7000000001ebac8> crypt :: libcrypt.a(shr_64.o) crypt :: <CDLL 'libcrypt.a(shr_64.o)', handle 10 at 0x70000000026d9b0> crypto :: libcrypto.a(libcrypto64.so) crypto :: <CDLL 'libcrypto.a(libcrypto64.so)', handle 11 at 0x70000000029eb70>

Restored a "while True: loop to be sure file was being read to the end otherwise, only the first entry was being returned. Corrected spelling errors Clarified some comments Modified some (temporary) tests in util.py

aixtools · 2017-12-07T12:41:57Z

On 01/12/2017 14:58, Victor Stinner wrote: > is it a 'feature' because it is a separate file? There is no written rule to distinguish a feature from a bugfix. In this case, I think that what matters is the size of the change. You add a lot of code and modified many lines.

util.py (changes) * 4 code lines added (i.e., excluding comments) to call _aix.find_library * moved one internal function to a seperate _util.py file (to not have it duplicated in _aix.py) * added some lines to test() function to verify it is working. Could be removed if this is the back-breaker.

__init__.py * 3 lines of code * 5 lines for comments Added: _aix.py - yes all added, some new, some inspired from util.py _util.py - one function from util.py to prevent duplication (did not want to change more of util.py)

The risk of regression depends on the number of modified lines.

The changes listed above have been the most constant since I started on this in Feb 2016. My goal was to have _aix.py be portable for both python2 and python3 - to minimize risk of regression.

If you don't modify Python 3.6, you reduce the risk of regression in Python 3.6 :-) Adding more code to Python 3.6 also increased the cost of maintenance, and the cost is already high. Days are too short to handle all incoming bug reports :-) Well, maybe we can discuss again backporting this change to Python 3.6,

Personally, I do not care too much about Python3(.6). This was nearly accepted a year ago for Python3.6, but I lost contact, and the last bits never got completed. Personally, I care more about Python2.7 - because that is what I have seen people use on AIX. Most important though is that something be done.

once it will be merged into master. Right now, the quality is not good enough to be merged:

Hard for me to argue about quality - I consider my self a noob when it comes to python. However, some changes, now reverted, broke things. As to quality, I suppose more comments re: what and why may help there. Bottom line: there comes a day when I throw in the towel because "good enough" seems to be something unobtainable.

see all pending comments ;-)

Your call. I have done what I can in the time I have available.

aixtools · 2017-12-13T18:50:57Z

My thanks to the reviewers.

aixtools · 2017-12-14T16:02:34Z

The appveyor test failed in an area where this patch has not changed anything...

test_pipe_overlapped (test.test_asyncio.test_windows_utils.PipeTests) ... ok
test_popen (test.test_asyncio.test_windows_utils.PopenTests) ... test test_asyncio failed
C:\projects\cpython\lib\asyncio\base_events.py:499: ResourceWarning: unclosed event loop <_WindowsSelectorEventLoop running=False closed=False debug=False>
  source=self)
ok
======================================================================
ERROR: test_get_event_loop_returns_running_loop (test.test_asyncio.test_events.TestCGetEventLoop)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_asyncio\test_events.py", line 2738, in setUp
    watcher = asyncio.SafeChildWatcher()
AttributeError: module 'asyncio' has no attribute 'SafeChildWatcher'
======================================================================
ERROR: test_get_event_loop_returns_running_loop (test.test_asyncio.test_events.TestPyGetEventLoop)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_asyncio\test_events.py", line 2738, in setUp
    watcher = asyncio.SafeChildWatcher()
AttributeError: module 'asyncio' has no attribute 'SafeChildWatcher'
----------------------------------------------------------------------
Ran 1273 tests in 19.004s
FAILED (errors=2, skipped=56)
2 tests failed again:
    test_asyncio test_distutils
Total duration: 6 min 31 sec
Tests result: FAILURE
C:\projects\cpython>set lastexitcode=2 
C:\projects\cpython>set  1>C:\Users\appveyor\AppData\Local\Temp\1\tmp4D38.tmp 
C:\projects\cpython>echo C:\projects\cpython  1>C:\Users\appveyor\AppData\Local\Temp\1\tmp4D39.tmp 
C:\projects\cpython>exit /b 2

Nothing I can examine

michael@x071:[/data/prj/python/git/python-3.7.0.1]find . -name test_windows_utils\*
./Lib/test/test_asyncio/test_windows_utils.py
michael@x071:[/data/prj/python/git/python-3.7.0.1]-name-name
michael@x071:[/data/prj/python/git/python-3.7.0.1]./python ./Lib/test/test_asyncio/test_windows_utils.py
Traceback (most recent call last):
  File "./Lib/test/test_asyncio/test_windows_utils.py", line 10, in <module>
    raise unittest.SkipTest('Windows only')
unittest.case.SkipTest: Windows only

How does this move forward now?

vstinner · 2017-12-15T14:26:59Z

As Martin mentioned (and I guess I can add later), one of my goals was to have one version for both python2 and python3.

I suggest write code for Python 3.7, so using latest Python features like f-string.

If later we decide to backport the code Python 3.6 and 2.7, you can adapt the code for older version.

But I dislike having code written for Python 2 in the Python 3 stdlib.

aixtools · 2017-12-17T18:32:53Z

On 15/12/2017 15:27, Victor Stinner wrote: > As Martin mentioned (and I guess I can add later), one of my goals was to have one version for both python2 and python3. I suggest write code for Python 3.7, so using latest Python features like f-string.

As I said in a private e-mail. If there was something essential about f-strings I would take the trouble to learn about them. However, in my nearly 40 years of professional support I have never seen much good coming from forcing a new feature. "It's a feature, not a benefit".

If later we decide to backport the code Python 3.6 and 2.7, you can adapt the code for older version. But I dislike having code written for Python 2 in the Python 3 stdlib.

As I also said, if all of ctypes was written to use f-string - you would also have a strong point. So, I guess this is just a refusal. Sorry we cannot find a way to work together. After 21+ months of there always being something (well, subtract 12 as I lost contact with who was assisting then) - I surrender - you win. This will not be fixed for AIX. Thank you for your time. Best wishes for the holidays.

vstinner

f-strings are not mandatory. I propose to go without f-string, and if @Mariatta really want to get f-string in ctypes, maybe @Mariatta can write a more global f-string PR for the whole ctypes module, once the AIX code landed?

@aixtools: I rewiewed the latest version of your PR, and it's almost good to be merged. IMHO the remaining blocker issue the the creation of the _util.py file (I would prefer to not create such file for a single function, I proposed solutions).

@aixtools: It would be sad to lose this work just because of coding style. As I wrote, I'm ok to take your PR without f-string. It was more a proposal than a requirement.

Thank your for your work, and come on, it's almost done! It would be great to finally fix this very old AIX issue in Python 3.7!

vstinner · 2017-12-18T09:07:41Z

Lib/ctypes/_aix.py

+naming style.
+"""
+__author__ = "Michael Felt <aixtools@felt.demon.nl>"
+__version__ = "1.0.0"


I dislike having versions in stdlib modules, especially in submodules. IMHO the Python version is enough, since the only ship the stdlib with Python. I suggest to remove __version.

vstinner · 2017-12-18T09:08:18Z

Lib/ctypes/_aix.py

+    # 2. If "INDEX" in occurs in a following line - return ld_header
+    # 3. get info (lines starting with [0-9])
+
+    def get_ld_header(p):


Nested functions are slow, since they are created each time you call get_ld_headers(). Would it be possible to put them at the module scope?

vstinner · 2017-12-18T09:08:51Z

Lib/ctypes/_aix.py

+
+    ldr_headers = []
+    p = Popen(["/usr/bin/dump", "-X%s" % aix_abi(), "-H", file],
+        universal_newlines=True, stdout=PIPE, stderr=DEVNULL)


Would you mind to use "with p:" here to close the subprocess in case of error?

vstinner · 2017-12-18T09:09:12Z

Lib/ctypes/_aix.py

+        universal_newlines=True, stdout=PIPE, stderr=DEVNULL)
+    # be sure to read to the end-of-file - getting all entries
+    while True:
+            ld_header = get_ld_header(p)


Indentation should be fixed.

vstinner · 2017-12-18T09:10:23Z

Lib/ctypes/_aix.py

+    Return executable bit size - 32 or 64
+    Used to filter the search in an archive by size, e.g., -X64
+    """
+    return sizeof(c_void_p) * 8


Why not putting directly the result into a constant? AIX_ABI = sizeof(c_void_p) * 8. sizeof() is fast and cheap, and the result is not going to change :-)

vstinner · 2017-12-18T09:11:55Z

Lib/ctypes/_aix.py

+    If no archive+member pair is found, look for a .so file.
+    """
+
+    def find_shared(paths, name):


Again, would it be possible to put this function at the module scope for performance?

vstinner · 2017-12-18T09:14:47Z

Lib/ctypes/_util.py

@@ -0,0 +1,15 @@
+from sys import maxsize


Why not putting this code in util.py directly? If it's really specific to AIX, maybe put it in _aix.py instead?

Mariatta · 2017-12-18T19:26:45Z

I don't mind preparing another PR to convert %s to f-strings, once this is merged :)

aixtools · 2017-12-19T07:39:52Z

On 18/12/2017 10:19, Victor Stinner wrote: vstinner commented on this pull request. f-strings are not mandatory. I propose to go without f-string, and if @Mariatta really want to get f-string in ctypes, maybe @Mariatta can write a more global f-string PR for the whole ctypes module, once the AIX code landed?

@Mariatta - one word: "Thanks!"

@aixtools: I rewiewed the latest version of your PR, and it's almost good to be merged. IMHO the remaining blocker issue the the creation of the _util.py file (I would prefer to not create such file for a single function, I proposed solutions).

As I said before, was a suggestion of an earlier (in 2016) review. I'll look at your suggestions.

@aixtools: It would be sad to lose this work just because of coding style. As I wrote, I'm ok to take your PR without f-string. It was more a proposal than a requirement. Thank your for your work, and come on, it's almost done! It would be great to finally fix this very old AIX issue in Python 3.7!

Yes, basically as old as ctypes. "Bite my tongue" re: why noone seemed to notice. Thanks, while appreciated - are not needed. My thanks to cpython for including it. (Once _util.py is removed, plus your other suggestions).

aixtools · 2017-12-19T08:03:10Z

On 18/12/2017 10:19, Victor Stinner wrote: + + ldr_headers = [] + p = Popen(["/usr/bin/dump", "-X%s" % aix_abi(), "-H", file], + universal_newlines=True, stdout=PIPE, stderr=DEVNULL) Would you mind to use "with p:" here to close the subprocess in case of error?

No, I do not mine. I tried it. However, it only parses the first instance. Fine when there is only one, but that is not always. I could go back to an earlier proposal - where I read the entire file into a buffer, and then processed the buffer. What sort of error are you concerned about? I do not know Popen() well enough to guess at your concern. As far as the command 'dump' goes, it is next to impossible to not have that program installed: michael@x071:[/data/prj/python/git/python-3.7.0.1/Lib/ctypes]lslpp -w /usr/bin/dump File Fileset Type ---------------------------------------------------------------------------- /usr/bin/dump bos.rte.bind_cmds Symlink as it is part of the fileset that include the real-time loader, 'ar', librtl.a, etc.. So, Willing to use it, but I need to be sure all references to shared libraries are found. An archive (.a file) often has more than one member (shr.o, libFOO.so, libFOO.so.X.Y.Z) and all should be returned. e.g., michael@x071:[/data/prj/python/git/python-3.7.0.1/Lib/ctypes]ar -X64 -tv /usr/lib/libssl.a | grep \.so rwxr-xr-x 0/0 821330 Jun 14 10:27 2016 libssl64.so rwxr-xr-x 0/0 821330 Jun 14 10:27 2016 libssl64.so.1.0.0 rwxr-xr-x 0/0 577122 Jun 14 10:27 2016 libssl64.so.0.9.8 michael@x071:[/data/prj/python/git/python-3.7.0.1/Lib/ctypes]dump -X64 -H /usr/lib/libssl.a | grep \.so /usr/lib/libssl.a[libssl64.so]: 1 libcrypto.a libcrypto64.so.1.0.0 /usr/lib/libssl.a[libssl64.so.1.0.0]: 1 libcrypto.a libcrypto64.so.1.0.0 /usr/lib/libssl.a[libssl64.so.0.9.8]: 1 libcrypto.a libcrypto64.so.0.9.8 So, for now - my preference is to leave asis. Help me understand your concern and I'll think about what else could be done.

aixtools · 2017-12-19T08:44:41Z

On 18/12/2017 10:19, Victor Stinner wrote: > @@ -0,0 +1,15 @@ +from sys import maxsize Why not putting this code in util.py directly? If it's really specific to AIX, maybe put it in _aix.py instead?

Again, the problem I ran into then with:

diff -r 5a05c0eeefc3 Lib/ctypes/util.py --- a/Lib/ctypes/util.py Sat Aug 27 04:07:54 2016 +0000 +++ b/Lib/ctypes/util.py Sat Oct 01 06:26:31 2016 +0000 @@ -3,6 +3,20 @@ import subprocess import sys +def _last_version(libnames, sep): + def _num_version(libname): + # "libxyz.so.MAJOR.MINOR" => [MAJOR, MINOR] + parts = libname.split(sep) + nums = [] + try: + while parts: + nums.insert(0, int(parts.pop())) + except ValueError: + pass + return nums or [sys.maxsize] + + return max(reversed(libnames), key=_num_version) + # find_library(name) returns the pathname of a library, or None.

And my attempt today: michael@x071:[/data/prj/python/git/python-3.7.0.1]git diff Lib/ctypes/util.py diff --git a/Lib/ctypes/util.py b/Lib/ctypes/util.py index db8c96e..cadde0c 100644 --- a/Lib/ctypes/util.py +++ b/Lib/ctypes/util.py @@ -2,7 +2,21 @@ import os import shutil import subprocess import sys -from ctypes._util import _last_version + +def _num_version(libname): + from sys import maxsize + # "libxyz.so.MAJOR.MINOR" => [MAJOR, MINOR] + parts = libname.split(sep) + nums = [] + try: + while parts: + nums.insert(0, int(parts.pop())) + except ValueError: + pass + return nums or [maxsize] + +def _last_version(libnames, sep): + return max(reversed(libnames), key=_num_version) # find_library(name) returns the pathname of a library, or None. if os.name == "nt": is: michael@x071:[/data/prj/python/git/python-3.7.0.1]./python Lib/ctypes/util.py Traceback (most recent call last): File "Lib/ctypes/util.py", line 105, in <module> from ctypes._aix import find_library File "/data/prj/python/git/python-3.7.0.1/Lib/ctypes/_aix.py", line 53, in <module> from ctypes.util import _last_version File "/data/prj/python/git/python-3.7.0.1/Lib/ctypes/util.py", line 105, in <module> from ctypes._aix import find_library ImportError: cannot import name 'find_library' from 'ctypes._aix' (/data/prj/python/git/python-3.7.0.1/Lib/ctypes/_aix.py) My guess is that _aix.py - being imported from util.py cannot also import util.py. So, the 'solution' seemed to be to move _last_version() - which is a "support, not exported" function - to a separate file. So, please explain how to do it better? This is just something beyond my knowledge of python import rules. For now - I'll restore util.py to what it was, and have my explicit function in _aix.py as well. Duplication, but one less file.

vstinner · 2017-12-19T13:00:43Z

I merged @aixtools PR. I still had minor comments, but this PR has 116 comments, 19 commits, and @aixtools wrote that he was close to give up. Perfect is the enemy of good.

@vadmium, @Mariatta and me are free to write a new PR on top of the merged change to enhance the code even more ;-)

aixtools · 2017-12-19T19:11:27Z

On 19/12/2017 14:01, Victor Stinner wrote: @vadmium, @Mariatta and me are free to write a new PR on top of the merged change to enhance the code even more ;-)

And my thanks for your improvements. Just give me a head's up, and I'll test them for you too!

Mariatta · 2017-12-19T19:33:08Z

Thanks @aixtools for your contribution to CPython, and @vstinner and @vadmium for the reviews and getting this merged 🎉

aixtools added 4 commits November 20, 2017 23:01

fix test so that AIX does not traceback by default

372d3a8

patch to implement find_library() support in ctypes/util for AIX

0887719

after patchcheck

70c1122

add a standard .so library to the AIX test in util.py

e339f30

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

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

merwok changed the title ~~Issue 26439~~ bpo-26439 ... Nov 22, 2017

merwok changed the title ~~bpo-26439 ...~~ bpo-26439 Fix ctypes.util.find_library failure on AIX Nov 22, 2017

Mariatta removed the CLA not signed label Nov 24, 2017

the-knights-who-say-ni added the CLA signed label Nov 24, 2017

aixtools added 2 commits November 26, 2017 16:24

Add NEWS and last bits needed by configure and _ctypes

814258f

cannot start blurb with 'bpo-'

ec543ab

aixtools added 2 commits November 28, 2017 21:31

librpm.so is only 32-bit; compare cdll.LoadLibrary() and CDLL()

3200c57

set LD_LIBRARY_PATH as preferred env variable

8948968

Mariatta closed this Nov 29, 2017

Mariatta reopened this Nov 29, 2017

vstinner reviewed Nov 30, 2017

View reviewed changes

Rather than import three functions from re, import all of re

7630102

and call the routines by name, e.g., re.search, re.match, re.escape

vadmium reviewed Dec 3, 2017

View reviewed changes

remove changes to test_loading.py - for another day

b29a00e

Responding to reviewer comments

b63335d

Restored a "while True: loop to be sure file was being read to the end otherwise, only the first entry was being returned. Corrected spelling errors Clarified some comments Modified some (temporary) tests in util.py

made changes per reviewer comments/suggestions

109cc7b

vstinner reviewed Dec 18, 2017

View reviewed changes

updates per reviewer requests

9149a30

vstinner merged commit c5ae169 into python:master Dec 19, 2017

bedevere-bot removed the awaiting review label Dec 19, 2017

Mariatta mentioned this pull request Dec 23, 2017

bpo-26439: Convert %s to f-strings. #4986

Merged

aixtools deleted the issue-26439 branch January 8, 2018 15:28

aixtools restored the issue-26439 branch January 11, 2018 10:03

bpo-26439 Fix ctypes.util.find_library failure on AIX #4507

bpo-26439 Fix ctypes.util.find_library failure on AIX #4507

Conversation

aixtools commented Nov 22, 2017 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Nov 22, 2017

aixtools commented Nov 23, 2017

Mariatta commented Nov 23, 2017

aixtools commented Nov 24, 2017

aixtools commented Nov 24, 2017

Mariatta commented Nov 24, 2017

aixtools commented Nov 24, 2017 • edited Loading

aixtools commented Nov 26, 2017

aixtools commented Nov 28, 2017

aixtools commented Nov 28, 2017

aixtools commented Nov 29, 2017

Mariatta commented Nov 29, 2017

aixtools commented Nov 29, 2017 • edited Loading

vstinner 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

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

aixtools Dec 13, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aixtools commented Dec 6, 2017 via email

vadmium commented Dec 6, 2017 via email

aixtools commented Dec 7, 2017 via email

aixtools commented Dec 7, 2017 via email

aixtools commented Dec 7, 2017 via email

aixtools commented Dec 7, 2017 via email

aixtools commented Dec 7, 2017 via email

aixtools commented Dec 13, 2017

aixtools commented Dec 14, 2017

vstinner commented Dec 15, 2017

aixtools commented Dec 17, 2017 via email

vstinner 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

Choose a reason for hiding this comment

Mariatta commented Dec 18, 2017

aixtools commented Dec 19, 2017 via email

aixtools commented Dec 19, 2017 via email

aixtools commented Dec 19, 2017 via email

vstinner commented Dec 19, 2017

aixtools commented Dec 19, 2017 via email

Mariatta commented Dec 19, 2017

aixtools commented Nov 22, 2017 •

edited by bedevere-bot

Loading

aixtools commented Nov 24, 2017 •

edited

Loading

aixtools commented Nov 29, 2017 •

edited

Loading

aixtools Dec 13, 2017 •

edited

Loading