Opened 9 years ago

Closed 8 years ago

#15178 closed defect (fixed)

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

Reported by: was, certik Owned by:
Priority: critical Milestone: sage-6.3
Component: packages: standard Keywords:
Cc: fbissey Merged in:
Authors: Bradly Schlenker Reviewers: William Stein, Peter Bruin
Report Upstream: N/A Work issues: Update `package-version.txt`
Branch: 4fdef74 (Commits, GitHub, GitLab) Commit: 4fdef7436a08d4ad2af8900d81816f440d7f9c34
Dependencies: Stopgaps:

Status badges

Description (last modified by was)

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.

Change History (25)

comment:1 Changed 9 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 8 years ago by was

  • Description modified (diff)

comment:3 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 8 years ago by bradlys

  • Branch set to u/bradlys/ticket/15178
  • Created changed from 09/08/13 17:51:46 to 09/08/13 17:51:46
  • Modified changed from 05/06/14 15:19:32 to 05/06/14 15:19:32

comment:5 Changed 8 years ago by bradlys

  • Commit set to 815dd05a67da0f011928c04db72d29fb812d8916
  • Status changed from new to needs_review

New commits:

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

comment:6 Changed 8 years ago by was

  • Status changed from needs_review to needs_work
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

comment:7 Changed 8 years ago by git

  • Commit changed from 815dd05a67da0f011928c04db72d29fb812d8916 to 5bd09123dafcf5df768a02365f6df93a44087777

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

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

comment:8 Changed 8 years ago by fbissey

  • Cc fbissey added

comment:9 Changed 8 years ago by leif

  • Authors set to Bradly Schlenker
  • Reviewers set to William Stein
  • Status changed from needs_work to needs_review

comment:10 follow-up: Changed 8 years ago by 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).

comment:11 Changed 8 years ago by leif

  • Status changed from needs_review to needs_work
  • Work issues set to Remove new patch, fix old one

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

  • pexpect.py

    a b  
    11301130    """
    11311131    # Special case where filename already contains a path.
    11321132    if os.path.dirname(filename) != '':
    1133         if os.access (filename, os.X_OK):
     1133        if os.access (filename, os.X_OK) and not os.path.isdir(f):
    11341134            return filename
    11351135
    11361136    if not os.environ.has_key('PATH') or os.environ['PATH'] == '':
     
    11451145
    11461146    for path in pathlist:
    11471147        f = os.path.join(path, filename)
    1148         if os.access(f, os.X_OK):
     1148        if os.access(f, os.X_OK) and not os.path.isdir(f):
    11491149            return f
    11501150    return None
    11511151

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

comment:12 follow-up: Changed 8 years ago by 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) )

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by 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...

comment:14 in reply to: ↑ 13 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by git

  • Commit changed from 5bd09123dafcf5df768a02365f6df93a44087777 to 8a42373dd85a0e0813c040685f1474e7b735f82a

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

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

comment:16 Changed 8 years ago by leif

  • Status changed from needs_work to needs_review
  • Work issues Remove new patch, fix old one deleted

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.

comment:17 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by 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?

comment:18 in reply to: ↑ 17 Changed 8 years ago by 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.

comment:19 Changed 8 years ago by 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.

comment:20 follow-up: Changed 8 years ago by rws

  • Status changed from needs_review to needs_work

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]

comment:21 in reply to: ↑ 20 Changed 8 years ago by 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?

comment:22 Changed 8 years ago by rws

  • Status changed from needs_work to needs_review

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.

Last edited 8 years ago by rws (previous) (diff)

comment:23 Changed 8 years ago by leif

  • Status changed from needs_review to needs_work
  • Work issues set to Update `package-version.txt`

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...

comment:24 Changed 8 years ago by pbruin

  • Branch changed from u/bradlys/ticket/15178 to u/pbruin/15178-pexpect_which
  • Commit changed from 8a42373dd85a0e0813c040685f1474e7b735f82a to 4fdef7436a08d4ad2af8900d81816f440d7f9c34
  • Reviewers changed from William Stein to William Stein, Peter Bruin
  • Status changed from needs_work to positive_review

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

comment:25 Changed 8 years ago by vbraun

  • Branch changed from u/pbruin/15178-pexpect_which to 4fdef7436a08d4ad2af8900d81816f440d7f9c34
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.