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

adds option to ignore TCL/Tk framework installs on OS X #1768

Closed
wants to merge 1 commit into from

Conversation

ddale
Copy link

@ddale ddale commented Mar 14, 2016

Fixes issue #950. Pillow's setup.py file incorrectly assumes if you are on OS X, you must link with a TCL/TK framework installation. Some OS X python installations (Anaconda, for example) provide their own TCL/TK installation, which results in segfaults when Pillow tries to load _imagingtk (usually observed with a call to PhotoImage). A simple mechanism is needed to instruct Pillow's setup.py file to not use the the TCL/TK Frameworks installations on OS X. This pull request provides an environment variable IGNORE_OSX_TCLTK_FRAMEWORKS which, if defined, allows setup.py to build against non-frameworks installations of TCL/TK.

@hugovk
Copy link
Member

hugovk commented Mar 14, 2016

Does Anaconda define IGNORE_OSX_TCLTK_FRAMEWORKS? If not, should we ask them to? Or is there a suitable env var they already define?

@ddale
Copy link
Author

ddale commented Mar 14, 2016

Recipes for conda packages include a simple build.sh (or on windows bld.bat) script, which for trivial python packages simply contain:

$PYTHON setup.py install

The conda recipe build.sh for Pillow would simply be modified:

export IGNORE_OSX_TCLTK_FRAMEWORKS=1
$PYTHON setup.py install

I also submitted a post alerting the anaconda mailing list to this pull request, and one of the developers responded on the list that this would be a good improvement.

@wiredfool
Copy link
Member

How does anaconda deal with the dynamic load at runtime if there are the two different libraries on the system?

@ddale
Copy link
Author

ddale commented Mar 14, 2016

If I install Anaconda to /opt/conda, and I run "conda install tk", it installs its own tk libraries to the /opt/conda prefix. Anaconda is configured to search this location first.

@aclark4life
Copy link
Member

@ddale Definitely interested in supporting Anaconda but not convinced this is the right approach. How about a --disable-osx-tclk-framework flag? https://github.com/ddale/Pillow/blob/e9c4ee33543421130140de5ecdd3eb034a9a0649/setup.py#L123

@ddale
Copy link
Author

ddale commented Mar 14, 2016

@aclark4life I like your solution. That would work just fine for Anaconda.

@aclark4life
Copy link
Member

@ddale Great! If you update your PR then we'll merge (assuming no other concerns from the @python-pillow/pillow-team )

@ddale
Copy link
Author

ddale commented Mar 14, 2016

I realize now though that I don't completely understand how to change my pull request to make use of a --disable-osx-tcltk-framework flag.

@hugovk
Copy link
Member

hugovk commented Mar 14, 2016

If you want to change a PR, just commit and push into your own osx-no-tk-framework branch and this PR'll be automatically updated.

@aclark4life
Copy link
Member

@ddale Try adding additional commits to your fork, else I can close this one and you can open another PR.

@wiredfool
Copy link
Member

Looking at this, I'm not actually understanding how the Tcl/Tk actually work on OSX.

By the time we get to

Pillow/setup.py

Line 490 in e9c4ee3

feature.tk = "tk" + TCL_VERSION
, we've found Tcl/Tk, but I don't think we've looked in any of the /Library or /System/Library directories.

We then later go through around line

Pillow/setup.py

Line 589 in e9c4ee3

"/System/Library/Frameworks"]
we search those directories, as long as we've already found those libraries.

@ddale
Copy link
Author

ddale commented Mar 14, 2016

@aclark4life what I meant was, I don't understand how --disable-x gets propagated to what variable that I should use in the code.

@aclark4life
Copy link
Member

@ddale I was picturing the disable flag could set a boolean that could then be checked where your environment check currently occurs. But it sounds like we need to sort out some bigger picture issues @wiredfool raised first.

@ddale
Copy link
Author

ddale commented Mar 14, 2016

@wiredfool I also don't understand why, on os x, after having found TCL/TK, when it is finally time to actually configure the extension module, pillow switches gears and searches for frameworks.

@aclark4life
Copy link
Member

@wiredfool Possibly because symlinks from /usr/include?:

~/.../pillow/Pillow $ ll /usr/include/{tcl,tk}*
lrwxr-xr-x  1 root  wheel    72B Dec  9 14:30 /usr/include/tcl.h@ -> ../../System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers/tcl.h
lrwxr-xr-x  1 root  wheel    77B Dec  9 14:30 /usr/include/tclDecls.h@ -> ../../System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers/tclDecls.h
lrwxr-xr-x  1 root  wheel    81B Dec  9 14:30 /usr/include/tclPlatDecls.h@ -> ../../System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers/tclPlatDecls.h
lrwxr-xr-x  1 root  wheel    79B Dec  9 14:30 /usr/include/tclTomMath.h@ -> ../../System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers/tclTomMath.h
lrwxr-xr-x  1 root  wheel    84B Dec  9 14:30 /usr/include/tclTomMathDecls.h@ -> ../../System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers/tclTomMathDecls.h
lrwxr-xr-x  1 root  wheel    70B Dec  9 14:30 /usr/include/tk.h@ -> ../../System/Library/Frameworks/Tk.framework/Versions/8.5/Headers/tk.h
lrwxr-xr-x  1 root  wheel    75B Dec  9 14:30 /usr/include/tkDecls.h@ -> ../../System/Library/Frameworks/Tk.framework/Versions/8.5/Headers/tkDecls.h
lrwxr-xr-x  1 root  wheel    82B Dec  9 14:30 /usr/include/tkIntXlibDecls.h@ -> ../../System/Library/Frameworks/Tk.framework/Versions/8.5/Headers/tkIntXlibDecls.h
lrwxr-xr-x  1 root  wheel    76B Dec  9 14:30 /usr/include/tkMacOSX.h@ -> ../../System/Library/Frameworks/Tk.framework/Versions/8.5/Headers/tkMacOSX.h
lrwxr-xr-x  1 root  wheel    79B Dec  9 14:30 /usr/include/tkPlatDecls.h@ -> ../../System/Library/Frameworks/Tk.framework/Versions/8.5/Headers/tkPlatDecls.h

@ddale It would not be surprising at all to discover redundancy and/or bad logic in Pillow's setup.py, unfortunately …

@wiredfool
Copy link
Member

Right, so there are symlinks from /usr/lib and /usr/include into the OSX frameworks. In the absence of the framework path, these get linked anyway, as /usr/lib is one of the standard locations we look at.

If we disable that stanza completely, I still get a valid build on the mac (mavericks):

        if feature.tcl and feature.tk:
            if (False and sys.platform == "darwin" and
                os.environ.get('IGNORE_OSX_TCLTK_FRAMEWORKS', False)==False):
                # locate Tcl/Tk frameworks

yields:

Pillow 3.2.0.dev0 TEST SUMMARY 
--------------------------------------------------------------------
Python modules loaded from /Users/erics/vpy27/lib/python2.7/site-packages/Pillow-3.2.0.dev0-py2.7-macosx-10.9-x86_64.egg/PIL
Binary modules loaded from /Users/erics/.python-eggs/Pillow-3.2.0.dev0-py2.7-macosx-10.9-x86_64.egg-tmp/PIL
--------------------------------------------------------------------
--- PIL CORE support ok
--- TKINTER support ok
--- FREETYPE2 support ok
--- LITTLECMS2 support ok
*** WEBP support not installed
--- JPEG support ok
--- OPENJPEG (JPEG2000) support ok
--- ZLIB (PNG/ZIP) support ok
--- LIBTIFF support ok

And tests pass.

The framework builds were added in a commit labeled "make it work" 3b8ae97. It's an ancient @aclark4life commit, so I'm guessing the reasoning was shear hacking to make it work.

@cjrh
Copy link

cjrh commented May 2, 2016

FWIW This PR also resolves the same problem happening when using Homebrew Python 3 (3.5.1), including virtualenvs. (OS X 10.11.4).

@aclark4life
Copy link
Member

@cjrh I think we're currently pondering use of the --disable-x flag instead of environment variable check.

@cjrh
Copy link

cjrh commented May 2, 2016

@aclark4life Yep I saw that.

@wiredfool
Copy link
Member

Alternative version of this in #1883, using the command line flag. Can someone check that it solves their issues, as I'm not getting the conda install working here.

@wiredfool wiredfool closed this May 23, 2016
@wiredfool
Copy link
Member

Closed in favor of #1883

@wiredfool
Copy link
Member

I'd like anyone who's interested in the "Which tk library to use" saga to check out PR #1932 and let me know if it works for you.

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.

None yet

5 participants