Skip to content
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

loading TCL / Tk symbols dynamically #6442

Merged
merged 14 commits into from May 24, 2016

Conversation

matthew-brett
Copy link
Contributor

This is an attempt to load the symbols we need from the Tkinter.tkinter
module at run time, rather than by linking at build time.

It is one way of building a manylinux wheel that can build on a basic
Manylinux docker image, and then run with the TCL / Tk library installed
on the installing user's machine. It would also make the situation
better on OSX, where we have to build against ActiveState TCL for
compatibility with Python.org Python, but we would like to allow
run-time TCL from e.g. homebrew.

I have tested this on Debian Jessie Python 2.7 and 3.5, and on OSX 10.9
with Python 2.7.

Questions:

  • Would y'all consider carrying something like this approach in
    the matplotlib source, but not enabled by default, to help building
    binary wheels?
  • Do you have any better suggestions about how to do this?
  • My C fu is weak; is there a way of collecting the typedefs I need from
    the TCL / Tk headers rather than copying them into the _tkagg.cpp
    source file (typdefs starting around line 52)?
  • My fu for Python C extension modules is also weak; did I configure
    exceptions and handle references correctly?

This is an attempt to load the symbols we need from the Tkinter.tkinter
module at run time, rather than by linking at build time.

It is one way of building a manylinux wheel that can build on a basic
Manylinux docker image, and then run with the TCL / Tk library installed
on the installing user's machine.  It would also make the situation
better on OSX, where we have to build against ActiveState TCL for
compatibility with Python.org Python, but we would like to allow
run-time TCL from e.g. homebrew.

I have tested this on Debian Jessie Python 2.7 and 3.5, and on OSX 10.9
with Python 2.7.

Questions:

* Would y'all consider carrying something like this approach in
  the matplotlib source, but not enabled by default, to help building
  binary wheels?
* Do you have any better suggestions about how to do this?
* My C fu is weak; is there a way of collecting the typedefs I need from
  the TCL / Tk headers rather than copying them into the _tkagg.cpp
  source file (typdefs starting around line 52)?
* My fu for Python C extension modules is also weak; did I configure
  exceptions and handle references correctly?
@mdboom
Copy link
Member

mdboom commented May 17, 2016

Thanks for this. This is really helpful.

@matthew-brett wrote:

Would y'all consider carrying something like this approach in the matplotlib source, but not enabled by default, to help building binary wheels?

I'd prefer to give this loads of testing and just have this as the "one way to do it" if we can. We could eliminate tons of hacks in the build script and tons of pain and support using this approach, at the expense of the very minor dynamic linking at import time.

My C fu is weak; is there a way of collecting the typedefs I need from the TCL / Tk headers rather than copying them into the _tkagg.cpp source file (typdefs starting around line 52)?

I don't think so.

My fu for Python C extension modules is also weak; did I configure exceptions and handle references correctly?

I'll read through and try to verify.

#define TKINTER_PKG "tkinter"
#define TKINTER_MOD "_tkinter"
// From module __file__ attribute to char *string for dlopen.
#define FNAME2CHAR(s) (PyBytes_AsString(PyUnicode_EncodeFSDefault(s)))
Copy link
Member

Choose a reason for hiding this comment

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

This isn't exception proof -- PyUnicode_EncodeFSDefault could raise an exception here. Maybe just make a regular old function for this purpose?

@QuLogic
Copy link
Member

QuLogic commented May 17, 2016

My C fu is weak; is there a way of collecting the typedefs I need from the TCL / Tk headers rather than copying them into the _tkagg.cpp source file (typdefs starting around line 52)?

What's wrong with #includeing the header?

@mdboom
Copy link
Member

mdboom commented May 17, 2016

What's wrong with #includeing the header?

Because the point of this change is to replace actual direct function calls with function pointers. Numpy, for example, has a special generated header for this exact purpose (__multiarray_api.h), but most C libraries don't.

@QuLogic
Copy link
Member

QuLogic commented May 17, 2016

I was hoping since it's C++ you could use some auto magic, but that might be C++11 only.

@dopplershift
Copy link
Contributor

auto is static (i.e. compile-time) typing only, so there's no way that could work for runtime type inference. That's the realm of C++ virtual functions/polymorphism.

@QuLogic
Copy link
Member

QuLogic commented May 17, 2016

We don't need runtime typing; we just need the type of the function at compile time so that we can set up a pointer to it.

@dopplershift
Copy link
Contributor

The return type of dlsym() (and hence _dfunc) is void *; there is absolutely no way for the compiler to help you out here.

@QuLogic
Copy link
Member

QuLogic commented May 17, 2016

I didn't really mean auto, just some "C++ magic" like it. And that something would be typeof or decltype, but both of these come with their own issues (one's a GNU-ism, the other's C++11 only).

Refactoring for style.  Check Python 3 filename encoding result.
#ifdef DYNAMIC_TKINTER
// Functions to fill global TCL / Tk function pointers from tkinter module.

#include <dlfcn.h>
Copy link

@ogrisel ogrisel May 18, 2016

Choose a reason for hiding this comment

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

This include fails when using Visual C++ for Python 2.7:

src/_tkagg.cpp(270) : fatal error C1083: Cannot open include file: 'dlfcn.h': No such file or directory

https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.1545/job/724qv5hcfngqa2r9#L1033

Copy link

Choose a reason for hiding this comment

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

Actually more recent versions of Visual C++ also fail, e.g. in the Python 3.5 Windows build on AppVeyor:

src/_tkagg.cpp(270): fatal error C1083: Cannot open include file: 'dlfcn.h': No such file or directory 
error: command 'C:\\Program Files (x86)\\Microsoft Visual Studio 14.0\\VC\\BIN\\amd64\\cl.exe' failed with exit status 2
Command exited with code 1

https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.1545/job/rk6safsaid63i7ch#L1053

@matthew-brett
Copy link
Contributor Author

@mdboom - addressed your comments - I think. I've tested disabling the linker flags for TCL / Tk libraries in setupext.py - works on OSX Python 2.7, 3.5, Debian Jessie 2.7 and 3.5.

@mdboom
Copy link
Member

mdboom commented May 18, 2016

Cool. If this works, we might as well take most of the crazy header-file-hunting out of setupext.py as well.

As for the Windows-specific issues, @cgohlke, any thoughts?

@matthew-brett
Copy link
Contributor Author

I've done some speculative changes to support Windows, trying to work out how to test them now.

@matthew-brett matthew-brett force-pushed the dynamic-tkagg branch 2 times, most recently from 1dbaa4f to c33657e Compare May 18, 2016 17:26
@matthew-brett
Copy link
Contributor Author

matthew-brett commented May 18, 2016

OK - Windows changes are compiling and passing tests on Python 2.7 at least.

I propose to remove the not-dynamic branch, and disable linking the TCL / Tk libs in setupext.py to see if this can work without build-time linking.

Try adding defines etc for using LoadLibrary on Windows to get the TCL /
Tk routines.
Remove ifdefs that allowed not-dynamic library resolution of TCL / Tk
symbols.
Disable link to TCL / Tk libraries now we are loading symbols at
run-time.
@matthew-brett matthew-brett changed the title WIP: loading TCL / Tk symbols dynamically MRG: loading TCL / Tk symbols dynamically May 18, 2016
@matthew-brett
Copy link
Contributor Author

Build-time linking disabled, Appveyor tests passing, failure on travis I believe is unrelated, so from my point of view, I think this is ready.

@cgohlke
Copy link
Contributor

cgohlke commented May 18, 2016

Maybe I am missing something, but how can this work on Windows where the Tcl/Tk symbols are not exported from _tkinter.pyd? Is appveyor testing TkAgg?

>>> from ctypes import *
>>> from ctypes.wintypes import *
>>> windll.kernel32.LoadLibraryW.restype = HMODULE
>>> windll.kernel32.LoadLibraryW.argtypes = [LPCWSTR]
>>> windll.kernel32.GetProcAddress.argtypes = [HMODULE, LPCSTR]
>>> windll.kernel32.GetProcAddress.restype = c_void_p
>>> from tkinter import _tkinter
>>> hdll = windll.kernel32.LoadLibraryW(_tkinter.__file__)
>>> print(windll.kernel32.GetProcAddress(hdll, b"PyInit__tkinter"))
140732219092336
>>> print(windll.kernel32.GetProcAddress(hdll, b"Tcl_CreateCommand"))
None

@efiring
Copy link
Member

efiring commented May 18, 2016

Quick test with a conda python 3.5 environment on OS X 10.9.5: it works! I was not able to get a working Tk backend without this.
Maybe unrelated to this, but I get lots of warnings while executing:

2016-05-18 10:51:04.230 python[18741:507] setCanCycle: is deprecated.  Please use setCollectionBehavior instead

@tacaswell tacaswell added this to the 1.5.2 (Critical bug fix release) milestone May 23, 2016
@efiring
Copy link
Member

efiring commented May 23, 2016

Travis passed. Appveyor stalled in the build stage on the Py3.5 job. I didn't see anything indicating the build failure was related to the PR, so I am closing and reopening to try again.

@efiring
Copy link
Member

efiring commented May 24, 2016

Well, I don't have any idea what the problem is, but Appveyor failed to build on Python 3.5 just like last time. Very little output is generated. It seems to stall before actually doing any conda installations.

Check that we can correctly import tkagg on travis and appveyor.
@matthew-brett
Copy link
Contributor Author

Appveyor now passing on all Pythons for some reason. Travis failure looks unrelated. Appveyor and Travis tests now testing tkagg import and succeeding. Tests of this branch on OSX at MacPython/matplotlib-wheels finding and importing tkagg correctly (unrelated failures but tests passing on Pythons 2.7 3.4 3.5).

@efiring
Copy link
Member

efiring commented May 24, 2016

Everything passed and Appveyor showed the tkagg test result correctly, but it looks like Travis either didn't execute the tkagg import test, or swallowed its output:

The command "echo Testing import of tkagg backend
MPLBACKEND="tkagg" python -c 'import matplotlib.pyplot as plt; print(plt.get_backend())'
echo Testing using $NPROC processes

@matthew-brett
Copy link
Contributor Author

OSX builds now green - successful tkagg import checks for Python 2.7 Python 3.4 Python 3.5.

@matthew-brett
Copy link
Contributor Author

Eric - I think the missing output is just because the command is in a block with other commands. The output is further down after the whole command input block - e.g. https://travis-ci.org/matplotlib/matplotlib/jobs/132588520#L1018

@efiring efiring merged commit f3e5576 into matplotlib:master May 24, 2016
efiring added a commit to efiring/matplotlib that referenced this pull request May 24, 2016
MRG: loading TCL / Tk symbols dynamically

cherry-pick of commit f3e5576

Conflicts resolved:
    .travis.yml
    appveyor.yml
    setupext.py
efiring added a commit that referenced this pull request May 24, 2016
MRG: loading TCL / Tk symbols dynamically

cherry-pick of commit f3e5576

Conflicts resolved:
    .travis.yml
    appveyor.yml
    setupext.py
@efiring
Copy link
Member

efiring commented May 24, 2016

backported to v1.5.x as b80e0f1

@QuLogic QuLogic changed the title MRG: loading TCL / Tk symbols dynamically loading TCL / Tk symbols dynamically Dec 7, 2016
@cournape
Copy link

When updating matplotlib for canopy, I am seeing the error "Could not find TCL routines" with 1.5.3.

At first, I suspect an issue in how we build python for canopy, but I have been able to reproduce the issue with python 2.7.12 from python.org and just installing wheels from Gholke website. The failure appears for both 1.5.3 and 2.0.0.

I could also reproduce the issue w/ 1.5.3 using conda (2.7.13 this time)

@cournape
Copy link

Hm, that should really be an issue on its own, sorry

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

Successfully merging this pull request may close these issues.

None yet

9 participants