PDAL / python

PDAL's Python Support
Other
117 stars 35 forks source link

Allow using "filters.python" in a python process on OS X #76

Closed froody closed 3 years ago

froody commented 3 years ago

This fixes https://github.com/PDAL/python/issues/75

froody commented 3 years ago

Looks like the test failures aren't caused by my change, as https://github.com/PDAL/python/pull/77 also fails

hobu commented 3 years ago

oops, I forgot to merge https://github.com/PDAL/python/pull/78 that cures the test issues.

hobu commented 3 years ago

It looks like tests are ✅ .

Can you provide a test that actually exercises your fix?

froody commented 3 years ago

Added test, verified that before my change the test fails with:

test_pipeline_construction (test.test_pypy.TestPythonInPython) ... python(19158,0x111c7ae00) malloc: *** error for object 0x10cd86080: pointer being freed was not allocated
python(19158,0x111c7ae00) malloc: *** set a breakpoint in malloc_error_break to debug
[1]    19158 abort      python setup.py test
hobu commented 3 years ago

Is -Wl,-flat_namespace going to have any consequences on other Python configurations (3.7, or self- or conda- built static)?

abellgithub commented 3 years ago

I don't really know anything about the build process for the python extension at this point, but all this seems to do is add -flat_namespace to the link. Is PYTHON_LINK_LIBRARY set to the Python executable, rather than the python shared object, on OSX? Can you explain what -flat_namespace is doing in this instance that addresses the problem?

froody commented 3 years ago

Can you explain what -flat_namespace is doing in this instance that addresses the problem?

-flat_namespace is the implicit default for linux, but OS X defaults to a two-level namespace. As I mentioned in the comment in the code, -flat_namespace allows libpdal_plugin_filter_python.dylib to use whichever definition of a symbol (e.g. Py_IsInitialized) is present at runtime. If python is the main executable, libpdal_plugin_filter_python.dylib will get symbols from that. If pdal (or anything other than python) is the main executable in a process, then it will get symbols from libpython.dylib instead. Before this change, libpdal_plugin_filter_python.dylib would always get symbols from libpython.dylib, so running with python as the main executable would cause all kinds of problems as there would be two instances of the python runtime in the same process.

froody commented 3 years ago

Is -Wl,-flat_namespace going to have any consequences on other Python configurations (3.7, or self- or conda- built static)?

No, it's safe to use for any version of python.

abellgithub commented 3 years ago

If pdal (or anything other than python) is the main executable in a process, then it will get symbols from libpython.dylib instead. Before this change, libpdal_plugin_filter_python.dylib would always get symbols from libpython.dylib, so running with python as the main executable would cause all kinds of problems as there would be two instances of the python runtime in the same process.

(Writing this mostly for the history...)

So, to be clear, we're still linking against libpython.dylib in order to resolve build-time symbols, but when using the extension, we're resolving those symbols against the python executable because 1) they're exported and 2) they're already loaded so they're found in the already loaded code, rather than the shared lib, despite the fact that there's a DT_NEEDED record in libpdal_plugin_filter_python.dylib for libpython.dylib.

I wrote one of the Python devs but didn't hear back, but still have not heard what's going on that causes the actual failure. I can't think of any general reason why having two copies of the interpreter that come from different libraries should be a problem. I'm disappointed that they haven't described this in detail or tried to fix it and we're left with this brittle situation.

hobu commented 3 years ago

2.3.6 pushed with this fix. https://pypi.org/project/PDAL/