Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

froody
Copy link
Contributor

@froody froody commented Dec 7, 2020

This fixes #75

@froody
Copy link
Contributor Author

froody commented Dec 7, 2020

Looks like the test failures aren't caused by my change, as #77 also fails

@hobu hobu closed this Dec 8, 2020
@hobu hobu reopened this Dec 8, 2020
@hobu
Copy link
Member

hobu commented Dec 8, 2020

oops, I forgot to merge #78 that cures the test issues.

@hobu
Copy link
Member

hobu commented Dec 8, 2020

It looks like tests are ✅ .

Can you provide a test that actually exercises your fix?

@froody
Copy link
Contributor Author

froody commented Dec 8, 2020

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
Copy link
Member

hobu commented Dec 9, 2020

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

@abellgithub
Copy link
Collaborator

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
Copy link
Contributor Author

froody commented Dec 10, 2020

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
Copy link
Contributor Author

froody commented Dec 10, 2020

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
Copy link
Collaborator

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 hobu merged commit 03f4af3 into PDAL:master Dec 10, 2020
@hobu
Copy link
Member

hobu commented Dec 10, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pdal.Pipeline + filters.python is broken on Python >= 3.8
3 participants