the pexpect in sage has a major bug in it -- the which command is broken (easy to fix)

[was, certik]

It's pretty obvious that there is a problem. This was reported in the thread:

​https://groups.google.com/forum/#!topic/sage-cloud/8NgkWfTbU78

There is an easy 1-line fix, which is to replace the line

if os.access (filename, os.X_OK) and not os.path.isdir(f):

with

if os.access (filename, os.X_OK) and not os.path.isdir(filename):

at the beginning of the "which" function.

[bradlys]

New commits:

​815dd05

Add filename.patch which patches pexpect-2.0 a bug found in ticket 15178.

[was]

sage -f pexpect

patching file pexpect.py
patching file pexpect.py
patching file pexpect.py
Hunk #1 FAILED at 1134.
1 out of 1 hunk FAILED -- saving rejects to file pexpect.py.rej
Error applying '../patches/filename.patch'
 
real    0m0.092s
user    0m0.003s
sys     0m0.009s
************************************************************************
Error installing package pexpect-2.0.p5

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​5bd0912

Add zzfilename.patch to resolve issues with patching. (Out of order patching, I believe)

[leif]

Probably the patch level in the package version (package-version.txt) should get bumped (to .p6), although with the "new git workflow" others just drop it (and also delete the changelog in SPKG.txt, which I don't like).

[leif]

Actually, the bug is introduced by us, trying to fix something else:

pexpect.py

@@ -1130 +1130 @@
     """
     # 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
 
     if not os.environ.has_key('PATH') or os.environ['PATH'] == '':
@@ -1145 +1145 @@
 
     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
 

(This is patches/pexpect.py-isdir_bug_fix.patch, so that one should get fixed.)

[leif]

Oh, that one apparently dates back to 2009:

=== pexpect-2.0.p2 (William Stein, January 23rd, 2009) ===
 * created proper SPKG.txt
 * fixed bug in pexpect where it tried to run directories

(Sorry William, could not resist... B) )

[was]

Replying to leif:

Oh, that one apparently dates back to 2009:

=== pexpect-2.0.p2 (William Stein, January 23rd, 2009) ===
 * created proper SPKG.txt
 * fixed bug in pexpect where it tried to run directories

(Sorry William, could not resist... B) )

Thanks! Isn't revision control awesome...

[leif]

Replying to was:

Thanks! Isn't revision control awesome...

Well, in this case it was our hand-made changelog in SPKG.txt, and others don't update but delete it when converting "legacy" spkgs to "new style" spkgs.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​8a42373

Removed zzfilename.patch and modified an existing patch that was the root cause of the error for ticket 15178

[leif]

Ok. I personally would have started from scratch, then pushing a new branch to trac (with -f, force), but the commits aren't that large, so I don't really mind.

[pbruin]

Replying to leif:

Probably the patch level in the package version (package-version.txt) should get bumped (to .p6), although with the "new git workflow" others just drop it

I do think you have to update package-version.txt; isn't that how Sage knows that it should reinstall the package with the updated patches?

[leif]

Replying to pbruin:

Replying to leif:

Probably the patch level in the package version (package-version.txt) should get bumped (to .p6), although with the "new git workflow" others just drop it

I do think you have to update package-version.txt; isn't that how Sage knows that it should reinstall the package with the updated patches?

Yes.

[fbissey]

Leif beat me to it. The only reason not to bump the version is if you fix a building bug which prevented some people (but not everyone) to install. People with successful install shouldn't have to re-install and the people for whom you provided the fix couldn't install.

[rws]

patchbot:

sage -t --long /scratch/sage/src/sage/interfaces/expect.py
**********************************************************************
File "/scratch/sage/src/sage/interfaces/expect.py", line 786, in sage.interface
s.expect.Expect._eval_line
Failed example:
    singular.interrupt(timeout=3)  # sometimes very slow (up to 60s on sage.mat
h, 2012)
Expected:
    False
Got:
    True
**********************************************************************
1 item had failures:
   1 of  15 in sage.interfaces.expect.Expect._eval_line
    [81 tests, 1 failure, 20.20 s]

[leif]

Replying to rws:

patchbot:

sage -t --long /scratch/sage/src/sage/interfaces/expect.py
**********************************************************************
File "/scratch/sage/src/sage/interfaces/expect.py", line 786, in sage.interface
s.expect.Expect._eval_line
Failed example:
    singular.interrupt(timeout=3)  # sometimes very slow (up to 60s on sage.mat
h, 2012)
Expected:
    False
Got:
    True
**********************************************************************
1 item had failures:
   1 of  15 in sage.interfaces.expect.Expect._eval_line
    [81 tests, 1 failure, 20.20 s]

How could the change here be related to this?

[rws]

Is it not? I was only superficially skimming. Then it's another patchbot problem. Sorry for the fuss.

Edit: Actually it was #10476 that hit.

[leif]

So, are we going to update package-version.txt?

The changelog entry in SPKG.txt refers to .p6, but we could also just delete the whole changelog (later?).

In the latter case, should package-version.txt still refer to .p5? ;-)

But here we actually change a patch to pexpect, so...

[pbruin]

Merged the previous commits into one and added the package-version.txt update as a reviewer patch.