-
-
Notifications
You must be signed in to change notification settings - Fork 565
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
Race condition in matplotlib mkdir() #11686
Comments
comment:1
Seen this before (i.e., similar), haven't we? |
comment:2
Looks like the same issue (with the same fix) as in #10159. I reported this to the matplotlib mailing list in October 2010, but no response. In fact, what happened to the patches from #10159 which dealt with this issue? Leif has noted on #10588 that those patches seem to have been lost in the update to version 1.0.1... |
comment:3
Replying to @nexttime:
This is a regression introduced by #10588, previously fixed in John's spkg from #10159, because the spkg of the former had not been based on the latter (and the bug hasn't [yet] been fixed upstream, at least not in 1.0.1). Also, the current spkg is full of crap in |
comment:4
Replying to @jhpalmieri:
It's different to what I first thought; see my previous comment... ;-) With a bit of luck your patches from #10159 still apply seamlessly to the current upstream version; haven't tested that yet. |
comment:5
Just checked: John's 1.0.0.p0 spkg has been in Sage 4.6.1 and 4.6.2.alpha0, then the one from #10588 got merged into Sage 4.6.2.alpha1. |
comment:6
Replying to @nexttime:
Hmmm, a little work has to be done: ~/Sage/spkgs/matplotlib-1.0.1.p1/patches$ patch __init__.py < __init__.py.patch
patching file __init__.py
Hunk #1 succeeded at 103 with fuzz 2.
~/Sage/spkgs/matplotlib-1.0.1.p1/patches$ patch finance.py < finance.py.patch
patching file finance.py
Hunk #1 FAILED at 4.
Hunk #2 succeeded at 173 (offset 4 lines).
1 out of 2 hunks FAILED -- saving rejects to file finance.py.rej
~/Sage/spkgs/matplotlib-1.0.1.p1/patches$ patch texmanager.py < texmanager.py.patch
patching file texmanager.py
$ John, are you going to do this? |
Author: John Palmieri |
This comment has been minimized.
This comment has been minimized.
comment:7
I've created a new spkg, link in the ticket description. I'm posting the diff here for review purposes. |
comment:8
(I also deleted |
comment:9
(Actually, I didn't delete 'src/build', I just dropped in a freshly downloaded vanilla source as the 'src' directory, in case the previous version was corrupted in other ways than just the presence of 'build'.) |
comment:10
Replying to @jhpalmieri:
Yep, the upstream sources of Ryan's and your spkg differ: Hundreds of instances of diff -r matplotlib-1.0.1/src/lib/pytz/tzfile.py matplotlib-1.0.1.p0/src/lib/pytz/tzfile.py
1c1
< #!/usr/bin/env python2
---
> #!/usr/bin/env python and also diff -r matplotlib-1.0.1/src/setup.py matplotlib-1.0.1.p0/src/setup.py
21c21
< rc = dict({'backend':'GTKAgg', 'numerix':'numpy'})
---
> rc = {'backend':'Agg'}
Only in matplotlib-1.0.1/src/: setupext.pyc
Only in matplotlib-1.0.1.p0/src/src: backend_agg.cpp
Only in matplotlib-1.0.1.p0/src/src: backend_gdk.c
Only in matplotlib-1.0.1.p0/src/src: image.cpp
Only in matplotlib-1.0.1.p0/src/src: path.cpp |
comment:11
Is this normal?
I do have
So maybe either --- src/setupext.py 2010-06-11 10:50:55.000000000 -0500
+++ src/setupext.py 2010-06-11 14:42:15.000000000 -0500
@@ -538,7 +538,7 @@
module.include_dirs.append(numpy.get_include())
def add_png_flags(module):
- try_pkgconfig(module, 'libpng', 'png')
+ try_pkgconfig(module, 'libpng12', 'png12')
add_base_flags(module)
add_numpy_flags(module)
module.libraries.append('z') Probably this patch should only be applied on MacOS X, since
(Currently running doctests, so haven't checked if not applying the patch on Linux changes anything w.r.t. that.) We should add freetype (and now also GNU patch) to the Dependencies:
|
comment:12
On my OS X box, I see the same thing with the old 1.0.1 and with the new one:
I see that on linux, or at least on sage.math, it reports the correct version for these. I can
Opinions? I'll certainly add freetype and patch to the dependencies. |
comment:13
Impressive: $ du -h matplotlib-1.0.1*.spkg
12M matplotlib-1.0.1.p0.spkg
19M matplotlib-1.0.1.spkg So the Sage tarball will get a bit smaller again. (It'll get even smaller if someone someday removes the JSMath fonts and other obsolete files from SageNB's Mercurial repository; I'm not sure if we even ship duplicates in other packages.) |
comment:14
The version MPL reports for freetype2 clearly comes from Sage's Is |
comment:15
I'm confused now. On sage.math, if I look at the install log from the old matplotlib-1.0.1 (during a fresh Sage build), it says
But if I do "sage -f ..." on the same spkg, I get this in its install log:
And of course, in this old spkg, this was the only patch installed, and it was installed regardless of the OS. I don't know what's going on, but I'm posting a new spkg which only applies the patch on Darwin. I hope this improves things on non-Darwin systems, or at least doesn't hurt anything on such systems. |
comment:16
Replying to @jhpalmieri:
I think modifying the patch is easier, but do what you prefer. Note that the change should only be made if the OS is Darwin and
|
comment:17
I should perhaps open another ticket for changing |
comment:18
Replying to @nexttime:
Here's a version which does this. I'm not a shell script expert; is this a good way to accomplish it?
|
comment:19
Replying to @jhpalmieri:
if [ "$UNAME" = "Darwin" ]; then
command -v pkg-config || patch -p1 < "$patch" I see you've redirected the output of IMHO a bit more readable would be if [ "$UNAME" = Darwin ] && ! command -v pkg-config >/dev/null; then
patch -p1 < "$patch" # Apply only on Darwin if pkg-config isn't installed
fi but for simplicity (if you're not going to change the patch) I would move the specific patch to # Apply patches for all platforms. See SPKG.txt for why and what.
for patch in ../patches/*.patch; do
patch -p1 <"$patch"
if [ $? -ne 0 ]; then
echo >&2 "Error applying '$patch'"
exit 1
fi
done
# Apply patch(es) for Darwin.
if [ "$UNAME" = Darwin ]; then
for patch in ../patches/Darwin/*.patch; do
patch -p1 <"$patch"
if [ $? -ne 0 ]; then
echo >&2 "Error applying '$patch'"
exit 1
fi
done
fi (The latter would have to be changed from a loop to an Having the patch in a separate directory it's also immediately clear that it is platform-specific; unfortunately we here have another precondition. |
comment:20
For whatever reason, I still get
i.e. an "unknown" libpng version, even without the patch to |
comment:21
I think that using the vanilla source is best when possible, so I'm only going to apply the patch on Darwin. However, it's possible that someone will install (or uninstall) pkg-config and not recompile Sage, so I've put the test for that inside the patch. |
comment:22
Haha:
So somehow Sage's |
comment:23
Replying to @nexttime:
Yep, apparently by If I reinstall The offending line was
I.e.,
does no longer work unless We may have to really wrap #!/bin/sh
if [ -z "$SAGE_ROOT" ]; then
echo >&2 "Error: $0: SAGE_ROOT not defined."
exit 1
fi
exec /usr/bin/env PATH=`echo $PATH | sed -e "s|$SAGE_ROOT/local/bin:||"` \
pkg-config --define-variable=SAGE_ROOT="$SAGE_ROOT" "$@" (Or just call the system's Defining |
comment:52
Replying to @nexttime:
And perhaps also on iras by first installing the new libpng spkg and afterwards your old matplotlib spkg (the one that only applies the patch on Darwin). |
Changed keywords from MPL Errno 14 libpng to MPL Errno 17 libpng |
comment:54
I'll leave the old SPKG around for a while, in case anyone needs it. I also just made a version which doesn't patch setupext.py at all, to help test #11696. |
This comment has been minimized.
This comment has been minimized.
comment:55
Is the new spkg now ready to be merged independently of #11696? |
comment:57
Replying to @jdemeyer:
Yes. I just forgot to delete the work issues when giving it positive review again. I think it doesn't hurt if #11696 also (partially) fixes this in a different way, too, though, and the spkg there contains other important changes to the libpng spkg. (Currently, the latter needs work though, i.e. I have to update the spkg there, but as said, the MPL one here can be merged regardless of that.) |
Changed work issues from Either check for pkg-config on other platforms as well, or make the ticket depend on #11696. to none |
Merged: sage-4.7.2.alpha2 |
Attachment: matplotlib-1.0.1.p0.log Log of a failed build |
comment:59
Can this ticket be reopened again? My build failed (see http://groups.google.com/group/sage-release/browse_thread/thread/6daea0d0dfef0562). |
comment:60
Replying to @sagetrac-johanbosman:
Cf. my comment here. (You could try building with my current libpng spkg there, though it needs work for other constellations.) I'd prefer opening a follow-up ticket, as this ticket has already been merged; haven't looked at your build log yet, though. |
comment:61
Replying to @nexttime:
(And this ticket was intended to just fix a regression, because the first MPL 1.0.1 spkg had accidentally been based on an obsolete one; therefore its title.) |
comment:62
Okay, we'll move the discussion somewhere else then. ;). |
comment:63
Apparently MPL has problems using your
The patched (The note "(no pkg-config)" also appears if |
comment:64
Johan, please install this spkg (which provides some debugging output, not trying to fix anything yet), and inspect the top of the install log (only up to "[Edit setup.cfg to suppress the above messages]" if it fails, which I expect it to). Thanks. (I think I now know where the problem lies. :) ) |
comment:65
How do I install this? |
comment:66
Replying to @sagetrac-johanbosman:
For example by typing $ ./sage -f http://sage.math.washington.edu/home/leif/Sage/spkgs/matplotlib-1.0.1.p0-debug.spkg in your |
comment:67
The point is that Sage did not build. Or is it okay for this purpose to use another version of Sage? |
comment:68
Replying to @sagetrac-johanbosman:
This AFAIK shouldn't matter (that Sage didn't build), but I'm not totally sure. Since I assume previous versions of Sage did build for you on that machine (with an older MPL spkg), I'd use the Sage installation where it failed (i.e., Sage 4.7.2.alpha2), since the error might (at least in theory) originate from something else. To be on the safe side, copy the downloaded package to (In case Sage doesn't pick up the right one, which I don't think, you could (re)move the original MPL spkg from there. A matter of file modification times, so you could equally just To see in advance which version Sage would install, do $ (cd $SAGE_ROOT/spkg/standard/ && ./newest_version matplotlib) |
comment:69
Thanks for explaining. I'm trying to build it now. ;) |
comment:70
Hmm, I tried to upload the top of the build log, but I got an error message: IOError: [Errno 28] No space left on device Any suggestions? |
comment:71
Replying to @sagetrac-johanbosman:
To trac? I didn't think of the (full) MPL log (which you attached) being that large btw.
Try replacing the already attached one with the newer, smaller one. Requires it having the same filename of course. Or upload it onto Or mail it to me (notdotreallyatonlinedotde, replacing the obvious), then I can upload it to |
comment:72
Hahaha, my guess was right:
Johan, can you give the output of $ pkg-config --version ; echo $?
$ pkg-config --help ; echo $?
? |
comment:73
Replying to @nexttime:
For the record, I have a (preliminary) MPL 1.0.1.p1 spkg fixing this, which Johan already tested. (Cf. http://groups.google.com/group/sage-release/msg/49a4ded0bc168976.) I'll open a follow-up for that one at some uncertain point in the future... (Remind me of that in case nothing should happen for a while.) ;-) |
comment:74
Some follow-up, for upgrading to MPL 1.1.0: #11915 |
I regularly see this error when testing on the buildbot:
The problem looks to be the following race condition in the matplotlib code:
New spkg available at
http://sage.math.washington.edu/home/palmieri/SPKG/matplotlib-1.0.1.p0.spkg
CC: @jhpalmieri @jasongrout
Component: packages: standard
Keywords: MPL Errno 17 libpng
Author: John Palmieri
Reviewer: Leif Leonhardy
Merged: sage-4.7.2.alpha2
Issue created by migration from https://trac.sagemath.org/ticket/11686
The text was updated successfully, but these errors were encountered: