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:

Status badges

Description (last modified by jdemeyer)

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

  1. 9191_run_cython.patch to the SCRIPTS repo.
  2. 9191_doctest_spyx.patch to the SAGE library.

Attachments (2)

9191_run_cython.patch (1.5 KB) - added by jdemeyer 9 years ago.
9191_doctest_spyx.patch (4.5 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 9 years ago by kcrisman

Well, no surprise!

sage: sage.misc.interpreter.loa[tab]
sage.misc.interpreter.load_a_file
sage.misc.interpreter.load_cython
sage.misc.interpreter.load_startup_file
sage.misc.interpreter.load_wrap

This is a very easy fix, as it turns out. I haven't got a clue how to doctest it, though.

comment:2 Changed 9 years ago by kcrisman

GC04855:sage-5.4.beta1-again $ ./sage a.spyx 
Compiling a.spyx...
hello

It works. Needs review.

comment:3 Changed 9 years ago by kcrisman

  • Authors set to Karl-Dieter Crisman
  • Status changed from new to needs_review

comment:4 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Changed 9 years ago by jdemeyer

comment:5 Changed 9 years ago by jdemeyer

  • Authors changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Jeroen Demeyer
  • Description modified (diff)

I expanded on your patch, added a doctest, renamed sage-sagex to sage-run-cython.

Last edited 9 years ago by jdemeyer (previous) (diff)

comment:6 follow-up: Changed 9 years ago by kcrisman

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 kcrisman

  • Reviewers set to Jeroen Demeyer, Karl-Dieter Crisman

comment:8 in reply to: ↑ 6 Changed 9 years ago by jdemeyer

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 kcrisman

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: Changed 9 years ago by kcrisman

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 kcrisman

  • 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.
Version 0, edited 9 years ago by kcrisman (next)

comment:12 Changed 9 years ago by kcrisman

  • Milestone changed from sage-pending to sage-5.4

comment:13 Changed 9 years ago by kcrisman

  • 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 jdemeyer

Replying to kcrisman:

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?

You're right, I developed it on top of #13533.

comment:15 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.4 to sage-5.5

comment:16 Changed 9 years ago by jdemeyer

  • Dependencies changed from #13533 OR #13540 to #13533

comment:17 Changed 9 years ago by jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:19 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:20 Changed 9 years ago by jdemeyer

  • 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 jdemeyer

  • Dependencies changed from #13533, #13579 to #13533, #13579, #13631, #13681

Changed 9 years ago by jdemeyer

comment:22 Changed 9 years ago by jdemeyer

Rebased to sage-5.4.rc4. I assume the positive_review still stands.

comment:23 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.5

comment:24 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.5.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.