David Kirkby discovered that a recent SVN version of matplotlib did not compile when he was testing a new matplotlib for inclusion in Sage. A bug was opened here: https://sourceforge.net/tracker/?func=detail&aid=3022815&group_id=80706&atid=560720 It appears that a patch has been committed to 1.0.0 which tries to fix the issue. However, David still reports that matplotlib 1.0.0 still doesn't compile on Solaris. See: http://trac.sagemath.org/sage_trac/ticket/9221?#comment:7 I'm not sure what the right procedure for reopening a ticket is. Thanks, Jason -- Jason Grout
Apologies for the long message---I've really tried to be precise and complete. On 7/7/10 10:23 AM, Jason Grout wrote: > David Kirkby discovered that a recent SVN version of matplotlib did not > compile when he was testing a new matplotlib for inclusion in Sage. A > bug was opened here: > > https://sourceforge.net/tracker/?func=detail&aid=3022815&group_id=80706&atid=560720 > > It appears that a patch has been committed to 1.0.0 which tries to fix > the issue. However, David still reports that matplotlib 1.0.0 still > doesn't compile on Solaris. See: > > http://trac.sagemath.org/sage_trac/ticket/9221?#comment:7 > > I'm not sure what the right procedure for reopening a ticket is. > I've had some time and access to a solaris box today to try to deal with this issue. I think I've solved it, based on a hint from David Kirkby. Again, the issue is that matplotlib 1.0.0 will not compile on the solaris box t2.sagemath.org (and on another Solaris box too). David Kirkby provided the logs and specs for the systems at http://trac.sagemath.org/sage_trac/ticket/9221#comment:7 The error is in compiling CXX: ================================================================== gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -fPIC -DPY_ARRAY_UNIQUE_SYMBOL=MPL_ARRAY_API -DPYCXX_ISO_CPP_LIB=1 -I/scratch/grout/sage-4.5.3/local/lib/python2.6/site-packages/numpy/core/include -I/scratch/grout/sage-4.5.3/local/include -I. -I/scratch/grout/sage-4.5.3/local/lib/python2.6/site-packages/numpy/core/include -Isrc -Iagg24/include -I. -I/scratch/grout/sage-4.5.3/local/lib/python2.6/site-packages/numpy/core/include -I/scratch/grout/sage-4.5.3/local/include/freetype2 -I/scratch/grout/sage-4.5.3/local/include -I. -I/scratch/grout/sage-4.5.3/local/include/python2.6 -c src/backend_agg.cpp -o build/temp.solaris-2.10-sun4v-2.6/src/backend_agg.o cc1plus: warning: command line option "-Wstrict-prototypes" is valid for Ada/C/ObjC but not for C++ In file included from /scratch/grout/sage-4.5.3/local/include/python2.6/Python.h:8, from ./CXX/WrapPython.h:61, from ./CXX/Extensions.hxx:37, from src/ft2font.h:4, from src/backend_agg.cpp:10: /scratch/grout/sage-4.5.3/local/include/python2.6/pyconfig.h:1013:1: warning: "_FILE_OFFSET_BITS" redefined In file included from /usr/include/sys/types.h:18, from /scratch/grout/sage-4.5.3/local/include/zconf.h:364, from /scratch/grout/sage-4.5.3/local/include/zlib.h:34, from /scratch/grout/sage-4.5.3/local/include/png.h:470, from src/backend_agg.cpp:3: /usr/local/gcc-4.4.1-sun-linker/bin/../lib/gcc/sparc-sun-solaris2.10/4.4.1/include-fixed/sys/feature_tests.h:197:1: warning: this is the location of the previous definition In file included from /scratch/grout/sage-4.5.3/local/include/python2.6/Python.h:42, from ./CXX/WrapPython.h:61, from ./CXX/Extensions.hxx:37, from src/ft2font.h:4, from src/backend_agg.cpp:10: /usr/include/stdlib.h:144: error: declaration of C function 'void swab(const char*, char*, ssize_t)' conflicts with /usr/include/unistd.h:496: error: previous declaration 'void swab(const void*, void*, ssize_t)' here error: command 'gcc' failed with exit status 1 ================================================================== This was reportedly fixed in 1.0.0, but apparently the fix does not work for this Solaris box. Based on a hint from David Kirkby, I changed the CXX/WrapPython.h file to be: ================================================================== #ifndef __PyCXX_wrap_python_hxx__ #define __PyCXX_wrap_python_hxx__ // On some platforms we have to include time.h to get select defined #if !defined(__WIN32__) && !defined(WIN32) && !defined(_WIN32) && !defined(_WIN64) #include <sys/time.h> #endif // pull in python definitions #include <Python.h> #endif ================================================================== (in other words, I just deleted all the defining and undefining of variables). The resulting matplotlib compiled finally, and the result of the testing was: ================================================================== In [1]: import matplotlib In [2]: matplotlib.__version__ Out[2]: '1.0.0' In [3]: matplotlib.test() /scratch/grout/sage-4.5.3/local/lib/python2.6/site-packages/matplotlib/axes.py:2369: UserWarning: Attempting to set identical left==right results in singular transformations; automatically expanding. left=730139.0, right=730139.0 + 'left=%s, right=%s') % (left, right)) ---------------------------------------------------------------------- Ran 138 tests in 755.419s OK (KNOWNFAIL=42) Out[3]: True ================================================================== so I guess that means everything works. I also compiled it on Ubuntu 10.04 (64-bit) and OSX 10.6.4, and it seemed to compile fine on those systems, though on OSX 10.6.4, I got some warnings like: /home/grout/sage/local/include/python2.6/pyconfig.h:1028:1: warning: "_POSIX_C_SOURCE" redefined /home/grout/sage/local/include/python2.6/pyconfig.h:1037:1: warning: "_XOPEN_SOURCE" redefined This brings up another issue: going against the Python API As you probably already know, http://docs.python.org/c-api/intro.html#includes says that: "Since Python may define some pre-processor definitions which affect the standard headers on some systems, you must include Python.h before any standard headers are included." I notice a lot of other #undef statements for _POSIX_C_SOURCE and _XOPEN_SOURCE in matplotlib code. To my (uneducated) eyes, that seems like a recipe for trouble, since defining it one way while including some other files, and then undefining the macros and defining them a different way may lead to problems. For example, in _png.cpp, I see: #include <png.h> // To remove a gcc warning #ifdef _POSIX_C_SOURCE #undef _POSIX_C_SOURCE #endif According to the Python API, we should include python.h before including png.h. Thanks, Jason
As a follow-up, I've implemented the patch for CXX and also patches for the other files which do not include Python.h first here: http://github.com/jasongrout/matplotlib/commit/a961c299f5d589dae87e06caf54975eb657ebf3b I've also attached the patch. This patch gets rid of the warnings about redefining things on OSX 10.6.4 (see my last message on this thread). Thanks, Jason
On 09/16/2010 09:50 AM, Jason Grout wrote: > As a follow-up, I've implemented the patch for CXX and also patches for > the other files which do not include Python.h first here: > > http://github.com/jasongrout/matplotlib/commit/a961c299f5d589dae87e06caf54975eb657ebf3b > > > I've also attached the patch. > > This patch gets rid of the warnings about redefining things on OSX > 10.6.4 (see my last message on this thread). > > Thanks, > > Jason Jason, I tested your patch with Ubuntu 10.10, and it failed. The problem is that something is including setjmp.h before libpng.h tries to do so via pngconf.h, resulting in an error as the compiler trips over the following: # ifndef PNG_SKIP_SETJMP_CHECK # ifdef __linux__ # ifdef _BSD_SOURCE # define PNG_SAVE_BSD_SOURCE # undef _BSD_SOURCE # endif # ifdef _SETJMP_H /* If you encounter a compiler error here, see the explanation * near the end of INSTALL. */ __pngconf.h__ in libpng already includes setjmp.h; __dont__ include it again.; # endif # endif /* __linux__ */ # endif /* PNG_SKIP_SETJMP_CHECK */ The relevant part of INSTALL is: If you encounter a compiler error message complaining about the lines __png.h__ already includes setjmp.h; __dont__ include it again.; this means you have compiled another module that includes setjmp.h, which is hazardous because the two modules might not include exactly the same setjmp.h. If you are sure that you know what you are doing and that they are exactly the same, then you can comment out or delete the two lines. Better yet, use the cexcept interface instead, as demonstrated in contrib/visupng of the libpng distribution. For the most part your patch looks like the right thing, but I don't know what to do about this show-stopping glitch. I looked around, but could not figure out what was including setjmp.h after your patch, but not before. Maybe Mike will see it. Eric
On 9/16/10 5:24 PM, Eric Firing wrote: > On 09/16/2010 09:50 AM, Jason Grout wrote: >> As a follow-up, I've implemented the patch for CXX and also patches for >> the other files which do not include Python.h first here: >> >> http://github.com/jasongrout/matplotlib/commit/a961c299f5d589dae87e06caf54975eb657ebf3b >> >> >> I've also attached the patch. >> >> This patch gets rid of the warnings about redefining things on OSX >> 10.6.4 (see my last message on this thread). >> >> Thanks, >> >> Jason > > Jason, > > I tested your patch with Ubuntu 10.10, and it failed. The problem is > that something is including setjmp.h before libpng.h tries to do so via > pngconf.h, resulting in an error as the compiler trips over the following: What file caused the error (i.e., what file was compiling?) Thanks, Jason
On 9/16/10 8:00 PM, Eric Firing wrote: >>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>> that something is including setjmp.h before libpng.h tries to do so via >>> pngconf.h, resulting in an error as the compiler trips over the >>> following: >> > > Python.h includes pyfpe.h which includes setjmp.h. > > Eric > Ah, good catch. So we just need to include Python.h first, and then set that extra #def so that libpng doesn't try to include it? #include "Python.h" #def PNG_SKIP_SETJMP_CHECK #include <png.h> or something like that? Jason
On 9/16/10 9:03 PM, Jason Grout wrote: > On 9/16/10 8:00 PM, Eric Firing wrote: > >>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>> that something is including setjmp.h before libpng.h tries to do so via >>>> pngconf.h, resulting in an error as the compiler trips over the >>>> following: >>> >> >> Python.h includes pyfpe.h which includes setjmp.h. >> >> Eric >> > > Ah, good catch. So we just need to include Python.h first, and then set > that extra #def so that libpng doesn't try to include it? > > #include "Python.h" > #def PNG_SKIP_SETJMP_CHECK > #include<png.h> > Let me try again: In _backend_agg.cpp and _png.cpp, just add #define PNG_SKIP_SETJMP_CHECK right above #include<png.h> Does that fix it? Thanks, Jason
On 09/16/2010 04:12 PM, Jason Grout wrote: > On 9/16/10 9:03 PM, Jason Grout wrote: >> On 9/16/10 8:00 PM, Eric Firing wrote: >> >>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>> following: >>>> >>> >>> Python.h includes pyfpe.h which includes setjmp.h. >>> >>> Eric >>> >> >> Ah, good catch. So we just need to include Python.h first, and then set >> that extra #def so that libpng doesn't try to include it? >> >> #include "Python.h" >> #def PNG_SKIP_SETJMP_CHECK >> #include<png.h> >> > > Let me try again: > > In _backend_agg.cpp and _png.cpp, just add > > #define PNG_SKIP_SETJMP_CHECK > > right above > > #include<png.h> > > Does that fix it? Sure does. Your patch with that modification is committed to branch and trunk, 8706, 8707. Thank you! Eric > > Thanks, > > Jason
On 9/16/10 10:00 PM, Eric Firing wrote: > On 09/16/2010 04:12 PM, Jason Grout wrote: >> On 9/16/10 9:03 PM, Jason Grout wrote: >>> On 9/16/10 8:00 PM, Eric Firing wrote: >>> >>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>> following: >>>>> >>>> >>>> Python.h includes pyfpe.h which includes setjmp.h. >>>> >>>> Eric >>>> >>> >>> Ah, good catch. So we just need to include Python.h first, and then set >>> that extra #def so that libpng doesn't try to include it? >>> >>> #include "Python.h" >>> #def PNG_SKIP_SETJMP_CHECK >>> #include<png.h> >>> >> >> Let me try again: >> >> In _backend_agg.cpp and _png.cpp, just add >> >> #define PNG_SKIP_SETJMP_CHECK >> >> right above >> >> #include<png.h> >> >> Does that fix it? > > Sure does. Your patch with that modification is committed to branch and > trunk, 8706, 8707. Thank you! > Did someone check on Windows? I was hoping things wouldn't break in WrapPython.h when I switched the order of includes, but you never know... Jason
On 9/16/10 10:15 PM, Jason Grout wrote: >> Sure does. Your patch with that modification is committed to branch and >> trunk, 8706, 8707. Thank you! >> > > Did someone check on Windows? I was hoping things wouldn't break in > WrapPython.h when I switched the order of includes, but you never know... > Also, since you guys already have a relationship with upstream (CXX), could you report the bug and fix upstream? Thanks, Jason
On 9/16/2010 8:15 PM, Jason Grout wrote: > On 9/16/10 10:00 PM, Eric Firing wrote: >> On 09/16/2010 04:12 PM, Jason Grout wrote: >>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>> >>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>> following: >>>>>> >>>>> >>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>> >>>>> Eric >>>>> >>>> >>>> Ah, good catch. So we just need to include Python.h first, and then set >>>> that extra #def so that libpng doesn't try to include it? >>>> >>>> #include "Python.h" >>>> #def PNG_SKIP_SETJMP_CHECK >>>> #include<png.h> >>>> >>> >>> Let me try again: >>> >>> In _backend_agg.cpp and _png.cpp, just add >>> >>> #define PNG_SKIP_SETJMP_CHECK >>> >>> right above >>> >>> #include<png.h> >>> >>> Does that fix it? >> >> Sure does. Your patch with that modification is committed to branch and >> trunk, 8706, 8707. Thank you! >> > > Did someone check on Windows? I was hoping things wouldn't break in > WrapPython.h when I switched the order of includes, but you never know... > > Jason Trunk and 1.0 branch build OK on Windows. -- Christoph
On 09/16/2010 05:15 PM, Jason Grout wrote: > On 9/16/10 10:00 PM, Eric Firing wrote: >> On 09/16/2010 04:12 PM, Jason Grout wrote: >>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>> >>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>> following: >>>>>> >>>>> >>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>> >>>>> Eric >>>>> >>>> >>>> Ah, good catch. So we just need to include Python.h first, and then set >>>> that extra #def so that libpng doesn't try to include it? >>>> >>>> #include "Python.h" >>>> #def PNG_SKIP_SETJMP_CHECK >>>> #include<png.h> >>>> >>> >>> Let me try again: >>> >>> In _backend_agg.cpp and _png.cpp, just add >>> >>> #define PNG_SKIP_SETJMP_CHECK >>> >>> right above >>> >>> #include<png.h> >>> >>> Does that fix it? >> >> Sure does. Your patch with that modification is committed to branch and >> trunk, 8706, 8707. Thank you! >> > > Did someone check on Windows? I was hoping things wouldn't break in > WrapPython.h when I switched the order of includes, but you never know... Jason, Big trouble, even without Windows. First, after doing more reading, I am far from sure that skipping the check is OK. Second, it doesn't work on earlier Linux versions, for which pngconf.h lacks that SKIP variable entirely. What a pain. I see that Andrew Straw ran into this wall a couple years ago. Time to revert and re-think. It may be that your patch is almost OK, but will need a tweak for Linux. Eric > > Jason > > ------------------------------------------------------------------------------ > Start uncovering the many advantages of virtual appliances > and start using them to simplify application deployment and > accelerate your shift to cloud computing. > http://p.sf.net/sfu/novell-sfdev2dev > _______________________________________________ > Matplotlib-devel mailing list > Mat...@li... > https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
On 9/16/10 11:16 PM, Eric Firing wrote: > On 09/16/2010 05:15 PM, Jason Grout wrote: >> On 9/16/10 10:00 PM, Eric Firing wrote: >>> On 09/16/2010 04:12 PM, Jason Grout wrote: >>>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>>> >>>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>>> following: >>>>>>> >>>>>> >>>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>>> >>>>>> Eric >>>>>> >>>>> >>>>> Ah, good catch. So we just need to include Python.h first, and then set >>>>> that extra #def so that libpng doesn't try to include it? >>>>> >>>>> #include "Python.h" >>>>> #def PNG_SKIP_SETJMP_CHECK >>>>> #include<png.h> >>>>> >>>> >>>> Let me try again: >>>> >>>> In _backend_agg.cpp and _png.cpp, just add >>>> >>>> #define PNG_SKIP_SETJMP_CHECK >>>> >>>> right above >>>> >>>> #include<png.h> >>>> >>>> Does that fix it? >>> >>> Sure does. Your patch with that modification is committed to branch and >>> trunk, 8706, 8707. Thank you! >>> >> >> Did someone check on Windows? I was hoping things wouldn't break in >> WrapPython.h when I switched the order of includes, but you never know... > > Jason, > > Big trouble, even without Windows. First, after doing more reading, I > am far from sure that skipping the check is OK. Second, it doesn't work > on earlier Linux versions, for which pngconf.h lacks that SKIP variable > entirely. > > What a pain. I see that Andrew Straw ran into this wall a couple years ago. > > Time to revert and re-think. It may be that your patch is almost OK, > but will need a tweak for Linux. An equivalent, but very hackish, fix is to just undef _SETJMP_H. That's almost as bad as the original undef, though. Maybe putting this: #ifdef __linux__ #undef _SETJMP_H #endif #define PNG_SKIP_SETJMP_CHECK right before including png.h would work, at least until distros (eventually) upgrade to a libpng past April 2009 (when the check was added) or until we stop supporting the old distros. Thanks, Jason
On 09/16/2010 08:21 PM, Christoph Gohlke wrote: > > > On 9/16/2010 8:15 PM, Jason Grout wrote: >> On 9/16/10 10:00 PM, Eric Firing wrote: >>> On 09/16/2010 04:12 PM, Jason Grout wrote: >>>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>>> >>>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>>> following: >>>>>>> >>>>>> >>>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>>> >>>>>> Eric >>>>>> >>>>> >>>>> Ah, good catch. So we just need to include Python.h first, and then set >>>>> that extra #def so that libpng doesn't try to include it? >>>>> >>>>> #include "Python.h" >>>>> #def PNG_SKIP_SETJMP_CHECK >>>>> #include<png.h> >>>>> >>>> >>>> Let me try again: >>>> >>>> In _backend_agg.cpp and _png.cpp, just add >>>> >>>> #define PNG_SKIP_SETJMP_CHECK >>>> >>>> right above >>>> >>>> #include<png.h> >>>> >>>> Does that fix it? >>> >>> Sure does. Your patch with that modification is committed to branch and >>> trunk, 8706, 8707. Thank you! >>> >> >> Did someone check on Windows? I was hoping things wouldn't break in >> WrapPython.h when I switched the order of includes, but you never know... >> >> Jason > > Trunk and 1.0 branch build OK on Windows. Christoph, Once again, thanks very much for testing on Windows. I found a problem on linux (or rather, the buildbot found it because it is running an older version) so I have committed one more change to the branch. If that survives the buildbot, I will propagate it to the trunk, but I don't consider it the last word; I am hoping that someone else can do a better job of cleaning this up. Eric > > -- > Christoph
On 9/17/10 1:57 AM, Eric Firing wrote: > On 09/16/2010 08:21 PM, Christoph Gohlke wrote: >> >> >> On 9/16/2010 8:15 PM, Jason Grout wrote: >>> On 9/16/10 10:00 PM, Eric Firing wrote: >>>> On 09/16/2010 04:12 PM, Jason Grout wrote: >>>>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>>>> >>>>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>>>> following: >>>>>>>> >>>>>>> >>>>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>>>> >>>>>>> Eric >>>>>>> >>>>>> >>>>>> Ah, good catch. So we just need to include Python.h first, and then set >>>>>> that extra #def so that libpng doesn't try to include it? >>>>>> >>>>>> #include "Python.h" >>>>>> #def PNG_SKIP_SETJMP_CHECK >>>>>> #include<png.h> >>>>>> >>>>> >>>>> Let me try again: >>>>> >>>>> In _backend_agg.cpp and _png.cpp, just add >>>>> >>>>> #define PNG_SKIP_SETJMP_CHECK >>>>> >>>>> right above >>>>> >>>>> #include<png.h> >>>>> >>>>> Does that fix it? >>>> >>>> Sure does. Your patch with that modification is committed to branch and >>>> trunk, 8706, 8707. Thank you! >>>> >>> >>> Did someone check on Windows? I was hoping things wouldn't break in >>> WrapPython.h when I switched the order of includes, but you never know... >>> >>> Jason >> >> Trunk and 1.0 branch build OK on Windows. > > Christoph, > > Once again, thanks very much for testing on Windows. > > I found a problem on linux (or rather, the buildbot found it because it > is running an older version) so I have committed one more change to the > branch. If that survives the buildbot, I will propagate it to the > trunk, but I don't consider it the last word; I am hoping that someone > else can do a better job of cleaning this up. > I see the change that you made (keep the old order for linux, do the new thing for everything else). This seems like a bad thing to do. Looking at setjmp.h, it includes features.h. features.h relies on the POSIX and XOPEN variables that are defined in Python.h to set up the environment, and the actions from setjmp.h depend on that environment. I think the warning from the Python docs (if I read correctly) is that the environment must be set up according to the requirements, and Python.h sets up those requirements. In other words, by undefining the POSIX and XOPEN variables, it seems you have a very real likelihood of including a different setjmp.h in the Python.h compared to the libpng (because the conditions set up by features.h may be different). Again, I don't see a good way out of the mess. And I guess in this imperfect world, I can't argue with what appears to work! And as a big disclaimer; I knew almost nothing about all of this a couple of days ago. Thanks, Jason
On 09/16/2010 09:27 PM, Jason Grout wrote: > On 9/17/10 1:57 AM, Eric Firing wrote: >> On 09/16/2010 08:21 PM, Christoph Gohlke wrote: >>> >>> >>> On 9/16/2010 8:15 PM, Jason Grout wrote: >>>> On 9/16/10 10:00 PM, Eric Firing wrote: >>>>> On 09/16/2010 04:12 PM, Jason Grout wrote: >>>>>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>>>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>>>>> >>>>>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>>>>> following: >>>>>>>>> >>>>>>>> >>>>>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>>>>> >>>>>>>> Eric >>>>>>>> >>>>>>> >>>>>>> Ah, good catch. So we just need to include Python.h first, and then set >>>>>>> that extra #def so that libpng doesn't try to include it? >>>>>>> >>>>>>> #include "Python.h" >>>>>>> #def PNG_SKIP_SETJMP_CHECK >>>>>>> #include<png.h> >>>>>>> >>>>>> >>>>>> Let me try again: >>>>>> >>>>>> In _backend_agg.cpp and _png.cpp, just add >>>>>> >>>>>> #define PNG_SKIP_SETJMP_CHECK >>>>>> >>>>>> right above >>>>>> >>>>>> #include<png.h> >>>>>> >>>>>> Does that fix it? >>>>> >>>>> Sure does. Your patch with that modification is committed to branch and >>>>> trunk, 8706, 8707. Thank you! >>>>> >>>> >>>> Did someone check on Windows? I was hoping things wouldn't break in >>>> WrapPython.h when I switched the order of includes, but you never know... >>>> >>>> Jason >>> >>> Trunk and 1.0 branch build OK on Windows. >> >> Christoph, >> >> Once again, thanks very much for testing on Windows. >> >> I found a problem on linux (or rather, the buildbot found it because it >> is running an older version) so I have committed one more change to the >> branch. If that survives the buildbot, I will propagate it to the >> trunk, but I don't consider it the last word; I am hoping that someone >> else can do a better job of cleaning this up. >> > > > I see the change that you made (keep the old order for linux, do the new > thing for everything else). This seems like a bad thing to do. Looking > at setjmp.h, it includes features.h. features.h relies on the POSIX and > XOPEN variables that are defined in Python.h to set up the environment, > and the actions from setjmp.h depend on that environment. I think the > warning from the Python docs (if I read correctly) is that the > environment must be set up according to the requirements, and Python.h > sets up those requirements. In other words, by undefining the POSIX and > XOPEN variables, it seems you have a very real likelihood of including a > different setjmp.h in the Python.h compared to the libpng (because the > conditions set up by features.h may be different). Before making that change, I verified that at least on my linux system, Python.h defines those two variables the same way that features.h does, so the redefinition would be harmless if we did not undefine the variables. I don't think we are any worse off than before with respect to linux, and we should be better off with respect to other platforms--mpl on Solaris now compiles, right?. But I am certainly still not comfortable with the whole setjmp mess. I would love to see someone come up with a clean, clearly understandable solution, and resolve it once and for all. Eric > > Again, I don't see a good way out of the mess. And I guess in this > imperfect world, I can't argue with what appears to work! And as a big > disclaimer; I knew almost nothing about all of this a couple of days ago. > > Thanks, > > Jason > > ------------------------------------------------------------------------------ > Start uncovering the many advantages of virtual appliances > and start using them to simplify application deployment and > accelerate your shift to cloud computing. > http://p.sf.net/sfu/novell-sfdev2dev > _______________________________________________ > Matplotlib-devel mailing list > Mat...@li... > https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
On Fri, Sep 17, 2010 at 3:04 AM, Eric Firing <ef...@ha...> wrote: > Before making that change, I verified that at least on my linux system, > Python.h defines those two variables the same way that features.h does, > so the redefinition would be harmless if we did not undefine the > variables. I don't think we are any worse off than before with respect > to linux, and we should be better off with respect to other > platforms--mpl on Solaris now compiles, right?. But I am certainly > still not comfortable with the whole setjmp mess. I would love to see > someone come up with a clean, clearly understandable solution, and > resolve it once and for all. One more test point -- on my solaris x86 box running python2.4, svn HEAD (and r8707) appear to work fine. johnh@udesktop191:tests> uname -a SunOS udesktop191 5.10 Generic_139556-08 i86pc i386 i86pc johnh@udesktop191:tests> python -V Python 2.4.5 johnh@udesktop191:tests> /opt/app/g++lib6/gcc-4.2/bin/gcc --version gcc (GCC) 4.2.2
On 09/17/2010 03:04 AM, Eric Firing wrote: > On 09/16/2010 09:27 PM, Jason Grout wrote: >> >> >> I see the change that you made (keep the old order for linux, do the new >> thing for everything else). This seems like a bad thing to do. Looking >> at setjmp.h, it includes features.h. features.h relies on the POSIX and >> XOPEN variables that are defined in Python.h to set up the environment, >> and the actions from setjmp.h depend on that environment. I think the >> warning from the Python docs (if I read correctly) is that the >> environment must be set up according to the requirements, and Python.h >> sets up those requirements. In other words, by undefining the POSIX and >> XOPEN variables, it seems you have a very real likelihood of including a >> different setjmp.h in the Python.h compared to the libpng (because the >> conditions set up by features.h may be different). > Before making that change, I verified that at least on my linux system, > Python.h defines those two variables the same way that features.h does, > so the redefinition would be harmless if we did not undefine the > variables. I don't think we are any worse off than before with respect > to linux, and we should be better off with respect to other > platforms--mpl on Solaris now compiles, right?. But I am certainly > still not comfortable with the whole setjmp mess. I would love to see > someone come up with a clean, clearly understandable solution, and > resolve it once and for all. Okay, I'm glad you checked on your box. I agree that we wouldn't be any worse than before. Hopefully everyone's feature.h defaults are the same as what Python assumes. And hopefully Python hasn't changed those definitions over the supported releases. However, wouldn't it be good to leave the redefinitions in, though? They are warnings (and they are not spurious), and that would remind us that we should investigate the situation in the future (maybe after we stop supporting old versions of libpng). At least on your system, the redefinition would not change anything. I would prefer a warning reminding me of an ugly situation that could really potentially be a problem, rather than a hack to disable the warning because it doesn't affect a particular system or two. Thanks, Jason
On 09/16/2010 01:04 PM, Jason Grout wrote: > On 9/16/10 5:24 PM, Eric Firing wrote: >> On 09/16/2010 09:50 AM, Jason Grout wrote: >>> As a follow-up, I've implemented the patch for CXX and also patches for >>> the other files which do not include Python.h first here: >>> >>> http://github.com/jasongrout/matplotlib/commit/a961c299f5d589dae87e06caf54975eb657ebf3b >>> >>> >>> I've also attached the patch. >>> >>> This patch gets rid of the warnings about redefining things on OSX >>> 10.6.4 (see my last message on this thread). >>> >>> Thanks, >>> >>> Jason >> >> Jason, >> >> I tested your patch with Ubuntu 10.10, and it failed. The problem is >> that something is including setjmp.h before libpng.h tries to do so via >> pngconf.h, resulting in an error as the compiler trips over the following: > > > What file caused the error (i.e., what file was compiling?) Sorry, I wasn't thinking straight, or I would have included that. It was _backend_agg.cpp. However, if I swap the order of inclusion of png.h and Python.h in that file, then redefinition warnings are generated when it compiles, In file included from /usr/include/python2.6/Python.h:8, from src/backend_agg.cpp:6: /usr/include/python2.6/pyconfig.h:1031: warning: "_POSIX_C_SOURCE" redefined //usr/include/features.h:162: note: this is the location of the previous definition /usr/include/python2.6/pyconfig.h:1040: warning: "_XOPEN_SOURCE" redefined //usr/include/features.h:164: note: this is the location of the previous definition ... the compilation proceeds, and then fails with _png.cpp: In file included from /usr/include/libpng12/png.h:518, from src/_png.cpp:4: /usr/include/libpng12/pngconf.h:371: error: expected constructor, destructor, or type conversion before ‘.’ token /usr/include/libpng12/pngconf.h:372: error: ‘__dont__’ does not name a type error: command 'gcc' failed with exit status 1 Eric > > Thanks, > > Jason > > > ------------------------------------------------------------------------------ > Start uncovering the many advantages of virtual appliances > and start using them to simplify application deployment and > accelerate your shift to cloud computing. > http://p.sf.net/sfu/novell-sfdev2dev > _______________________________________________ > Matplotlib-devel mailing list > Mat...@li... > https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
On 09/16/2010 08:45 PM, Jason Grout wrote: > On 9/16/10 11:16 PM, Eric Firing wrote: >> On 09/16/2010 05:15 PM, Jason Grout wrote: >>> On 9/16/10 10:00 PM, Eric Firing wrote: >>>> On 09/16/2010 04:12 PM, Jason Grout wrote: >>>>> On 9/16/10 9:03 PM, Jason Grout wrote: >>>>>> On 9/16/10 8:00 PM, Eric Firing wrote: >>>>>> >>>>>>>>> I tested your patch with Ubuntu 10.10, and it failed. The problem is >>>>>>>>> that something is including setjmp.h before libpng.h tries to do so via >>>>>>>>> pngconf.h, resulting in an error as the compiler trips over the >>>>>>>>> following: >>>>>>>> >>>>>>> >>>>>>> Python.h includes pyfpe.h which includes setjmp.h. >>>>>>> >>>>>>> Eric >>>>>>> >>>>>> >>>>>> Ah, good catch. So we just need to include Python.h first, and then set >>>>>> that extra #def so that libpng doesn't try to include it? >>>>>> >>>>>> #include "Python.h" >>>>>> #def PNG_SKIP_SETJMP_CHECK >>>>>> #include<png.h> >>>>>> >>>>> >>>>> Let me try again: >>>>> >>>>> In _backend_agg.cpp and _png.cpp, just add >>>>> >>>>> #define PNG_SKIP_SETJMP_CHECK >>>>> >>>>> right above >>>>> >>>>> #include<png.h> >>>>> >>>>> Does that fix it? >>>> >>>> Sure does. Your patch with that modification is committed to branch and >>>> trunk, 8706, 8707. Thank you! >>>> >>> >>> Did someone check on Windows? I was hoping things wouldn't break in >>> WrapPython.h when I switched the order of includes, but you never know... >> >> Jason, >> >> Big trouble, even without Windows. First, after doing more reading, I >> am far from sure that skipping the check is OK. Second, it doesn't work >> on earlier Linux versions, for which pngconf.h lacks that SKIP variable >> entirely. >> >> What a pain. I see that Andrew Straw ran into this wall a couple years ago. >> >> Time to revert and re-think. It may be that your patch is almost OK, >> but will need a tweak for Linux. > > > An equivalent, but very hackish, fix is to just undef _SETJMP_H. That's > almost as bad as the original undef, though. > > Maybe putting this: > > #ifdef __linux__ > #undef _SETJMP_H > #endif > #define PNG_SKIP_SETJMP_CHECK > > right before including png.h would work, at least until distros > (eventually) upgrade to a libpng past April 2009 (when the check was > added) or until we stop supporting the old distros. I don't think that any of these hacks is desirable, so I am trying what I hope is a less-bad hack. Maybe Mike or John will be able to figure out a cleaner solution. I'm still worried about the possibility of a conflict between the versions of setjmp expected by png and by python. I think that if there were such a conflict, it would show up as a crash as part of the error handling in one or the other. Therefore it could lurk undetected for a long time. On the other hand, if there were such a conflict, I don't see how the pre-patched versions would have avoided it any better than the present version. I could not find any evidence that _backend_agg even needs to include png.h, so I deleted that inclusion, leaving only _png.cpp as the trouble spot. Eric > > Thanks, > > Jason