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: |
Description (last modified by )
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
- Milestone changed from sage-6.1 to sage-6.2
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:4 Changed 8 years ago by
- 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
- Commit set to 815dd05a67da0f011928c04db72d29fb812d8916
- Status changed from new to needs_review
comment:6 Changed 8 years ago by
- 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
- Commit changed from 815dd05a67da0f011928c04db72d29fb812d8916 to 5bd09123dafcf5df768a02365f6df93a44087777
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)
|
comment:8 Changed 8 years ago by
- Cc fbissey added
comment:9 Changed 8 years ago by
- Reviewers set to William Stein
- Status changed from needs_work to needs_review
comment:10 follow-up: ↓ 17 Changed 8 years ago by
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
- 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 1130 1130 """ 1131 1131 # Special case where filename already contains a path. 1132 1132 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): 1134 1134 return filename 1135 1135 1136 1136 if not os.environ.has_key('PATH') or os.environ['PATH'] == '': … … 1145 1145 1146 1146 for path in pathlist: 1147 1147 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): 1149 1149 return f 1150 1150 return None 1151 1151
(This is patches/pexpect.py-isdir_bug_fix.patch
, so that one should get fixed.)
comment:12 follow-up: ↓ 13 Changed 8 years ago by
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: ↓ 14 Changed 8 years ago by
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
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
- Commit changed from 5bd09123dafcf5df768a02365f6df93a44087777 to 8a42373dd85a0e0813c040685f1474e7b735f82a
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
|
comment:16 Changed 8 years ago by
- 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: ↓ 18 Changed 8 years ago by
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
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 itI 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
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: ↓ 21 Changed 8 years ago by
- 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
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
- 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.
comment:23 Changed 8 years ago by
- 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
- 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
- Branch changed from u/pbruin/15178-pexpect_which to 4fdef7436a08d4ad2af8900d81816f440d7f9c34
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Add filename.patch which patches pexpect-2.0 a bug found in ticket 15178.