On Apr 16, 11:43 pm, Ondrej Certik <ond...@certik.cz> wrote:
> On Thu, Apr 16, 2009 at 11:28 PM, mabshoff <mabsh...@googlemail.com> wrote:
>
> > Hi Ondrej,
>
> > Why would you say that this bug is introduced in the modified version
> > of Sage ships? Just because it fixes the problem for you when you
> > remove the patch that does not mean it is the cause (I am not day it
> > isn't without looking at the other changes you did for SPD). If you
> > could argue what the bug is I think we would be happy to fix it.
>
> I am a bit confused, what is the original pexpect and what is the
> modified pexpect for Sage.
>
> In the pexpect Sage package, there is the patch:
>
> patches/pexpect.py-isdir_bug_fix.diff
>
> and the file src/pexpect.py has the patch applied. Did you ship this
> patch upstream? Or is it just locally modified pexpect.py for Sage?
The patch was not pushed upstream AFAIK. I do not recall if it wasn't
considered a bug or not and upstream pexpect has seen a couple
releases since the version we use. Last time we looked into updating
pexpect we same massive performance regressions, so we didn't update.
The ticket might still be open in trac.
> Assuming it is locally modified for Sage, then I think the bug is just
> in the Sage's pexpect, because clearly the patch is wrong, let me copy
> it here again:
>
> $ cat patches/pexpect.py-isdir_bug_fix.diff
> --- pexpect.py 2007-04-16 07:08:24.000000000 -0700
> +++ ../../pexpect-2.0.p1.0/pexpect-2.0.p1/src/pexpect.py 2009-01-23
> 01:49:18.000000000 -0800
> @@ -1130,7 +1130,7 @@
> """
> # Special case where filename already contains a path.
> if os.path.dirname(filename) != '':
> - if os.access (filename, os.X_OK):
> + if os.access (filename, os.X_OK) and not os.path.isdir(f):
> return filename
Ok, I see the problem now. Sorry for being dense earlier. In Sage that
codepath clearly doesn't seem to be hit.
> if not os.environ.has_key('PATH') or os.environ['PATH'] == '':
> @@ -1145,7 +1145,7 @@
>
> for path in pathlist:
> f = os.path.join(path, filename)
> - if os.access(f, os.X_OK):
> + if os.access(f, os.X_OK) and not os.path.isdir(f):
> return f
> return None
>
> The second hunk is fine, it just adds " and not os.path.isdir(f)" to
> the if statement, where "f" is defined on the previous line. The first
> hunk is wrong, because it also adds " and not os.path.isdir(f)" to the
> if statement, but there the "filename" is a parameter of the which()
> function, while "f" is not defined at all, that's why I got:
>
> exceptions.UnboundLocalError: local variable 'f' referenced before assignment
>
> in the traceback above.
>
> Hopefully this makes it clear, that this is a bug, no matter what
> other packages I modified. Or do you disagree? If so, what is the
> purpose of introducing the undefined variable "f" then?
It is a bug, without looking at the context: Does removing the first
hunk fix the problem for you? I am not sure why the hunk was added, it
has been a while since I did that review ;)
> Ondrej
Cheers,
Michael
--~--~---------~--~----~------------~-------~--~----~
To post to this group, send email to sage-devel@googlegroups.com
To unsubscribe from this group, send email to
sage-devel-unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/sage-devel
URLs: http://www.sagemath.org
-~----------~----~----~----~------~----~------~--~---
