Opened 11 years ago
Closed 9 years ago
#9191 closed defect (fixed)
Running .spyx files from the command line doesn't work anymore
Reported by: | was | Owned by: | jason |
---|---|---|---|
Priority: | major | Milestone: | sage-5.5 |
Component: | misc | Keywords: | |
Cc: | Merged in: | sage-5.5.beta1 | |
Authors: | Karl-Dieter Crisman, Jeroen Demeyer | Reviewers: | Jeroen Demeyer, Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13533, #13579, #13631, #13681 | Stopgaps: |
Description (last modified by )
Create a file like this:
flat:tmp wstein$ cat a.spyx print "hello"
We have:
flat:tmp wstein$ sage a.spyx Traceback (most recent call last): File "/Users/wstein/sage/build/sage/local/bin/sage-sagex", line 5, in <module> from sage.misc.interpreter import load_sagex ImportError: cannot import name load_sagex
Apply
- 9191_run_cython.patch to the SCRIPTS repo.
- 9191_doctest_spyx.patch to the SAGE library.
Attachments (2)
Change History (26)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
GC04855:sage-5.4.beta1-again $ ./sage a.spyx Compiling a.spyx... hello
It works. Needs review.
comment:3 Changed 9 years ago by
- Status changed from new to needs_review
comment:4 Changed 9 years ago by
- Description modified (diff)
Changed 9 years ago by
comment:5 Changed 9 years ago by
- Description modified (diff)
I expanded on your patch, added a doctest, renamed sage-sagex
to sage-run-cython
.
comment:6 follow-up: ↓ 8 Changed 9 years ago by
Wow, nice work! Very minor concerns below.
I'm a little concerned about why .pyx files worked before anyway. Did it just make it to the
os.execv(os.path.join(binpath, 'sage-python'), ['sage-python', fn] + opts)
line and sage -python
(which is all sage-python
is) just knew what to do with it? And this is better for some reason for pyx files, right?
Also, any reason for making the messages print to stderr when they aren't errors? As well as for changing things to the 'new' print statements? I guess you did the work so I shouldn't complain :) but it always means I worry about missing some small detail.
Finally, if you're going to add pyx files to those which this command does, you should probably add a testing part to the doctest patch for that as well...
comment:7 Changed 9 years ago by
- Reviewers set to Jeroen Demeyer, Karl-Dieter Crisman
comment:8 in reply to: ↑ 6 Changed 9 years ago by
Replying to kcrisman:
I'm a little concerned about why .pyx files worked before anyway.
They worked only because they were treated as plain Python files. No Cython was involved. Treating them like .spyx
files is the logical thing to do.
Also, any reason for making the messages print to stderr when they aren't errors?
These kind of diagnostic messages are often printed to stderr
in Unix-land. For example, gcc -v
will output what it's doing to stderr
. This makes it much easier to use the output of the script non-interactively: if I want to run a .spyx
file in a shell script, I have to manually remove the "Compiling..." line if its output to stdout
.
I'm happy with simply removing the "Compiling..." line also.
As well as for changing things to the 'new' print statements?
I really dislike the
print >>file
syntax in Python 2. Besides, it doesn't hurt to be more compatible with Python 3.
Finally, if you're going to add pyx files to those which this command does, you should probably add a testing part to the doctest patch for that as well...
I'm not sure, because running .pyx
files from the command line is not documented. The documentation suggests using .spyx
files, not .pyx
files.
comment:9 Changed 9 years ago by
Okay, that's all good enough for me. I'll test it sometime later but it all makes sense and looks nice.
comment:10 follow-up: ↓ 14 Changed 9 years ago by
I confirmed that nontrivial .pyx files did fail before - nice catch.
Somewhat ironically, the second patch doesn't apply to Sage 5.4.beta2. Does this depend on the patch which does or does not remove gcc optional from Sage?
comment:11 Changed 9 years ago by
- Dependencies set to #13533
- Milestone changed from sage-5.4 to sage-pending
- Status changed from needs_review to positive_review
In fact, I guess this patch *must* be based on this assumption, given that all the spyx doctest in cmdline.py has no optional gcc! Modulo that issue, this is fine, so putting positive review and sage-pending.
On a separate issue, I'm puzzled. I guess the optional tests weren't run very often. Here's an error even without the patches! This is Mac OS X 10.7.4. Just reporting here, I assume this is another (perhaps already opened) ticket.
sage -t -only-optional=gcc "devel/sage-main/sage/misc/cython.py" ********************************************************************** File "/Users/.../sage-5.4.beta2/devel/sage-main/sage/misc/cython.py", line 617: sage: g(2,3) # optional -- gcc Expected: 55.200000000000003 Got: 55.2 ********************************************************************** File "/Users/.../sage-5.4.beta2/devel/sage-main/sage/misc/cython.py", line 619: sage: g(0,0) # optional -- gcc Expected: 3.2000000000000002 Got: 3.2 ********************************************************************** File "/Users.../sage-5.4.beta2/devel/sage-main/sage/misc/cython.py", line 626: sage: f(10) # optional -- gcc Exception raised: KeyError: 'math' ********************************************************************** File "/Users/.../sage-5.4.beta2/devel/sage-main/sage/misc/cython.py", line 629: sage: f(10) # optional -- gcc Exception raised: KeyError: 'math' ********************************************************************** 1 items had failures: 4 of 12 in __main__.example_0 ***Test Failed*** 4 failures.
comment:12 Changed 9 years ago by
- Milestone changed from sage-pending to sage-5.4
comment:13 Changed 9 years ago by
- Dependencies changed from #13533 to #13533 OR #13540
- Description modified (diff)
Or, if #13540 comes in, then I guess we could use the other patch. I'm putting a probably illegal dependency listing now.
comment:14 in reply to: ↑ 10 Changed 9 years ago by
comment:15 Changed 9 years ago by
- Milestone changed from sage-5.4 to sage-5.5
comment:16 Changed 9 years ago by
- Dependencies changed from #13533 OR #13540 to #13533
comment:17 Changed 9 years ago by
- Summary changed from Running pyx files from the command line doesn't work anymore to Running .spyx files from the command line doesn't work anymore
comment:18 Changed 9 years ago by
- Description modified (diff)
comment:19 Changed 9 years ago by
- Description modified (diff)
comment:20 Changed 9 years ago by
- Dependencies changed from #13533 to #13533, #13579
- Milestone changed from sage-5.5 to sage-pending
This will need to be rebased to #13579.
comment:21 Changed 9 years ago by
- Dependencies changed from #13533, #13579 to #13533, #13579, #13631, #13681
Changed 9 years ago by
comment:22 Changed 9 years ago by
Rebased to sage-5.4.rc4. I assume the positive_review still stands.
comment:23 Changed 9 years ago by
- Milestone changed from sage-pending to sage-5.5
comment:24 Changed 9 years ago by
- Merged in set to sage-5.5.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Well, no surprise!
This is a very easy fix, as it turns out. I haven't got a clue how to doctest it, though.