Fix setupext [backport to 1.4.x] #3510

blink1073 · 2014-09-13T05:11:03Z

Cleanup for build process on win32. Detects libpng and freetype and raises an error if they are not found. Also adds convenience method get_include_dirs to capture a pattern that was used several times in setupext.py.

This addresses #3509.

tacaswell · 2014-09-13T14:51:53Z

Also see #3503 which is merged on 1.4.x.

cc @jbmohler

blink1073 · 2014-09-13T15:28:27Z

My version actually does check for png.h, but would fail on Windows with Python 3 < 3.3.4 due to a lack of getstatusoutput. We can include the actual function inline instead of importing it:

def getstatusoutput(cmd):
    """Return (status, output) of executing cmd in a shell."""
    import os
    pipe = os.popen('{ ' + cmd + '; } 2>&1', 'r')
    text = pipe.read()
    sts = pipe.close()
    if sts is None: sts = 0
    if text[-1:] == '\n': text = text[:-1]
    return sts, text

blink1073 · 2014-09-13T15:32:54Z

Or, we could just use a dummy if it is not available:

try:
    if sys.version_info[0] < 3:
        from commands import getstatusoutput
    else:
        from subprocess import getstatusoutput
except ImportError:
    def getstatusoutput(text):
        return 1, ''

tacaswell · 2014-09-13T16:03:47Z

I have merged 1.4.x into master (which may have generated a conflict on the link correction which I think this is the third time it has been fixed).

@blink1073 @jbmohler Can you two reach a consensus on how to deal with this?

blink1073 · 2014-09-13T17:36:10Z

The latest reflects what I vote to do. It respects the INCLUDE environmental variable on all platforms, and provides a dummy getstatusoutput. Here is the build text:

$ python matplotlib-winbuild/buildall.py -p
Obtaining file:///c:/Users/silvester/workspace/matplotlib
  Running setup.py (path:c:/Users/silvester/workspace/matplotlib\setup.py) egg_info for package from file:///c:/Users/silvester/workspace/matplotlib
    ============================================================================
    Edit setup.cfg to change the build options

    BUILDING MATPLOTLIB
                matplotlib: yes [1.5.x]
                    python: yes [2.7.8 |Continuum Analytics, Inc.| (default, Jul
                            2 2014, 15:13:35) [MSC v.1500 32 bit (Intel)]]
                  platform: yes [win32]

    REQUIRED DEPENDENCIES AND EXTENSIONS
                     numpy: yes [version 1.8.2]
                       six: yes [using six version 1.7.3]
                  dateutil: yes [using dateutil version 1.5]
                   tornado: yes [using tornado version 4.0.1]
                 pyparsing: yes [using pyparsing version 2.0.1]
                     pycxx: yes [Couldn't import.  Using local copy.]
                    libagg: yes [pkg-config information for 'libagg' could not
                            be found. Using local copy.]
                  freetype: yes [version Failed to identify version.]
                       png: yes [pkg-config information for 'libpng' could not
                            be found. Using unknown version found on system.]
                     qhull: yes [pkg-config information for 'qhull' could not be
                            found. Using local copy.]

    OPTIONAL SUBPACKAGES
               sample_data: yes [installing]
                  toolkits: yes [installing]
                     tests: yes [using nose version 1.3.3 / using mock 1.0.1]
            toolkits_tests: yes [using nose version 1.3.3 / using mock 1.0.1]

    OPTIONAL BACKEND EXTENSIONS
                    macosx: no  [Mac OS-X only]
                    qt5agg: no  [PyQt5 not found]
                    qt4agg: yes [installing, Qt: 4.8.6, PyQt: 4.8.6]
                    pyside: no  [PySide not found]
                   gtk3agg: no  [Requires pygobject to be installed.]
                 gtk3cairo: no  [Requires cairocffi or pycairo to be installed.]
                    gtkagg: no  [Requires pygtk]
                     tkagg: yes [installing, version 81008]
                     wxagg: no  [requires wxPython]
                       gtk: no  [Requires pygtk]
                       agg: yes [installing]
                     cairo: no  [cairocffi or pycairo not found]
                 windowing: yes [installing, installing]

    OPTIONAL LATEX DEPENDENCIES
                    dvipng: no
               ghostscript: yes [version 9.09]
                     latex: no
                   pdftops: no

cimarronm · 2014-09-13T17:59:00Z

setupext.py

-                raise CheckFailed(
-                    "pkg-config information for '%s' could not be found." %
-                    package)
+                msg = "pkg-config information for '%s' could not be found."


I don't think this fits in the spirit of what this function does. It is checking for pkg-config data and not if the header exists. The function which calls this has the responsibility of doing any additional checks (like looking for a header file if that is the correct behavior for the particular dependency).

jbmohler · 2014-09-13T18:47:13Z

There was an issue building on windows, but #3503 fixes some of the same things on this PR and now I'm a little unsure what else this PR is addressing.

I have a couple of comments in general after reading this in some more detail:

I agree with @cimarronm about the _check_for_pkg_config changes
The special case for win32 in check_include_file should probably be pulled in to has_include_file
After item 2 is addressed, then Png.check method's has_include_file call will probably succeed on windows
Making a null version of getstatusoutput for win32 seems a little sneaky to me. I'd probably prefer explicit win32 branches in the check functions, but I can appreciate the economy and it is similar to sneakiness in PkgConfig

(BTW, getstatusoutput actually exists on windows, but it is known not to work -- I'd vote to call that a bug in the standard library.)

blink1073 · 2014-09-13T18:49:05Z

@cimarronm, does that look better?

blink1073 · 2014-09-13T18:52:02Z

@jbmohler, #3503 does not check for the existence of libpng, so the user would not find out until the build failed later on.

blink1073 · 2014-09-13T19:10:37Z

@jbmohler, I added an implementation of getstatusoutput for win32, which I tested separately.

blink1073 · 2014-09-13T19:40:20Z

@jbmohler, I'm back with you on the separate code path way. What do you think now?

tacaswell · 2014-09-13T19:44:37Z

setupext.py

@@ -1003,6 +1011,10 @@ class Png(SetupPackage):
    name = "png"

    def check(self):
+        if sys.platform == 'win32':
+            check_include_file(get_include_dirs(), 'png.h', 'png')


I merged all of the other branches up into master. This seems to have introduced a conflict here. Can you rebase this PR?

blink1073 · 2014-09-13T19:58:50Z

Rebased.

tacaswell · 2014-09-13T20:15:22Z

not quite what I meant, but it seems to have worked.

blink1073 · 2014-09-13T20:16:42Z

Do you not use rebase -i upstream/master in matplotlib?

tacaswell · 2014-09-13T20:19:25Z

Never mind, it looks right now. Either I was confused or gh was showing me strangely cached results.

jbmohler · 2014-09-13T23:12:56Z

I like this PR -- it errors early now as appropriate & tests just fine on my win7 box. The commit message should more succintly say what it currently is.

A very minor grumble is that in Png.check (and Freetype.check), it seems that the check_include_file check and has_include_file check could become the same code. However, I can't see an easy way to get the equivalent flow with the conditionals and exceptions that is really any clearer than what you have.

tacaswell · 2014-09-13T23:51:29Z

Great, unless there are protests I will merge this tomorrow.

Allow for getstatusoutput on win32 and use INCLUDE and LIB env vars Back off on the use of the environmental variables Pinpointed necessary location for INCLUDE Fix link to matplotlib-winbuild Normalize handling of library location finding Include more paths in the search Restore _check for_pkg_config behavior Implement getstatusoutput on win32 Remove win32 getstatusoutput in favor of explicit check_include_file

blink1073 · 2014-09-14T01:16:34Z

I updated the commit message and PR description.

cimarronm · 2014-09-14T01:21:51Z

LGTM 👍 good work to improve the build for windows

BLD : improve setupext.py on windows

tacaswell · 2014-09-18T02:34:34Z

cherry-picked as d10e26e

pelson · 2014-09-23T13:00:12Z

LGTM 👍 good work to improve the build for windows

Indeed. @blink1073 - incidentally do you have any experience of appveyor? (e.g. https://github.com/sunpy/sunpy/blob/master/appveyor.yml)

blink1073 · 2014-09-25T02:12:13Z

@pelson, not personally, but we are looking to add it to scikit-image.

tacaswell modified the milestones: v1.4.x, v1.4.1 Sep 13, 2014

tacaswell added the Documentation label Sep 13, 2014

tacaswell changed the title ~~Fix setupext~~ Fix setupext [backport to 1.4.x] Sep 13, 2014

cimarronm reviewed Sep 13, 2014
View reviewed changes

tacaswell reviewed Sep 13, 2014
View reviewed changes

blink1073 force-pushed the fix-setupext branch from 5817724 to b45be96 Compare September 13, 2014 19:57

tacaswell added the Build label Sep 13, 2014

tacaswell added the status: needs review label Sep 13, 2014

blink1073 force-pushed the fix-setupext branch from b45be96 to 3c99de4 Compare September 14, 2014 01:16

tacaswell added a commit that referenced this pull request Sep 18, 2014

Merge pull request #3510 from blink1073/fix-setupext

2c11590

BLD : improve setupext.py on windows

tacaswell merged commit 2c11590 into matplotlib:master Sep 18, 2014

tacaswell added a commit that referenced this pull request Sep 18, 2014

Merge pull request #3510 from blink1073/fix-setupext

d10e26e

BLD : improve setupext.py on windows

QuLogic removed the status: needs review label Nov 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setupext [backport to 1.4.x] #3510

Fix setupext [backport to 1.4.x] #3510

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 13, 2014

cimarronm Sep 13, 2014

jbmohler commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

jbmohler commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 14, 2014

cimarronm commented Sep 14, 2014

tacaswell commented Sep 18, 2014

pelson commented Sep 23, 2014

blink1073 commented Sep 25, 2014

Fix setupext [backport to 1.4.x] #3510

Fix setupext [backport to 1.4.x] #3510

Conversation

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 13, 2014

cimarronm Sep 13, 2014

Choose a reason for hiding this comment

jbmohler commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell Sep 13, 2014

Choose a reason for hiding this comment

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 13, 2014

tacaswell commented Sep 13, 2014

jbmohler commented Sep 13, 2014

tacaswell commented Sep 13, 2014

blink1073 commented Sep 14, 2014

cimarronm commented Sep 14, 2014

tacaswell commented Sep 18, 2014

pelson commented Sep 23, 2014

blink1073 commented Sep 25, 2014