Opened 11 years ago

Last modified 8 years ago

#11821 needs_work defect

When running .sage files, pass the preparsed file through a pipe

Reported by: John Palmieri Owned by:
Priority: major Milestone: sage-6.4
Component: scripts Keywords:
Cc: Merged in:
Authors: John Palmieri Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #71 Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

If you run "sage new.sage" on a script "new.sage", it creates a preparsed file "new.py". Then when a file in the twisted package tries to import the "new" Python module, it ends up importing this preparsed file instead, which leads to problems which can be hard to track down.

I can think of several solutions:

  1. hope that users will know not to use names like "new.sage". This is the current state of affairs, and it seems overly optimistic to me.
  2. give a warning, or fail outright with an error, if the name of the file is the same as that of a Python module.
  3. name the preparsed file something which is less likely to cause a clash, for example, turn "file.sage" into "file_preparsed.py" (see #16955).
  4. don't save the preparsed file at all: write it to stdout, then run "sage-python -c <PREPARSED>" on it.

The attached patch implements number 4.


Apply

Attachments (3)

trac_11821-root.patch (1.9 KB) - added by John Palmieri 10 years ago.
root repo
trac_11821-doctests.patch (2.7 KB) - added by John Palmieri 10 years ago.
Sage library
trac_11821-preparse.v3.patch (18.9 KB) - added by John Palmieri 10 years ago.
scripts repo

Download all attachments as: .zip

Change History (23)

comment:1 Changed 11 years ago by John Palmieri

Status: newneeds_review

comment:2 Changed 11 years ago by Leif Leonhardy

I'd opt for either (2) or (3).

(3) is nicer from a user perspective, but may have other side effects.

(2) should be easy to implement (try: import ...), without potentially breaking other code because of assumptions on the filename, so I tend to prefer this variant.


P.S.: Looks like the patch is not yet based on #9739. (Haven't tried.)

If we change the way .sage files are preparsed when doctesting, we might run into errors due to left-over <file>.py files from attach() or load(). Just a thought.

comment:3 Changed 11 years ago by John Palmieri

Dependencies: #9739, #10952, #8708
Description: modified (diff)

Yet a fourth option, implemented in the "v2" patch: don't save the preparsed file at all. Here are the changes in this patch:

  • sage-preparse is split into two scripts: sage-preparse-one-file, which preparses a single file and writes the output to stdout. Then sage-preparse calls this, and saves the output to the appropriate place. Note that for sage-preparse, the help message in sage-sage says that it should "preparse file.sage and produce corresponding file_sage.py". This patch fixes both the behavior and the documentation: it saves to "file_sage.py", which should be a valid Python module name, instead of to "file.py" or "file.sage.py". This should help to prevent name clashes.
  • Then sage-run calls sage-preparse-one-file and feeds the output into "sage-python -c <OUTPUT>", rather than feeding the preparsed py file as "sage-python <FILE>".
  • Independently of this patch, running "sage <file1.sage> <file2.sage> ..." acts just like running "sage <file1.sage>" -- it ignores the later arguments. So the documentation has been altered to match this.
  • On the other hand, sage-preparse can handle multiple files as inputs, so change its description (at the top of the file) to match.

To do: add some tests to sage/tests/cmdline.py.

comment:4 Changed 11 years ago by John Palmieri

Description: modified (diff)

The new patch adds some doctests (not very sophisticated ones, but we can expand on them later).

comment:5 Changed 11 years ago by John Palmieri

Dependencies: #9739, #10952, #8708#9739, #10952, #8708, #12069
Status: needs_reviewneeds_work
Work issues: rebase on #12069

This needs rebasing relative to #12069. I'll get to it eventually.

comment:6 Changed 11 years ago by John Palmieri

Status: needs_workneeds_review

Okay, now rebased.

comment:7 Changed 11 years ago by John Palmieri

Work issues: rebase on #12069

Rebased to Sage 5.0.beta8 and #12069.

comment:8 Changed 11 years ago by John Palmieri

Description: modified (diff)

comment:9 Changed 10 years ago by David Roe

Status: needs_reviewneeds_work

This needs to be rebased against #12415 (sage-doctest is no longer relevant).

comment:10 Changed 10 years ago by John Palmieri

Status: needs_workneeds_review

Now rebased.

Changed 10 years ago by John Palmieri

Attachment: trac_11821-root.patch added

root repo

Changed 10 years ago by John Palmieri

Attachment: trac_11821-doctests.patch added

Sage library

Changed 10 years ago by John Palmieri

scripts repo

comment:11 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:12 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:13 Changed 8 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:14 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:15 Changed 8 years ago by Jeroen Demeyer

Dependencies: #9739, #10952, #8708, #12069#71

I don't like this patch because of #71. I regularly use the .py file when I get an exception to find out where the exception happened.

comment:16 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work
Summary: sage-preparse: give 'safer' names to .py filesWhen running .sage files, pass the preparsed file through a pipe

comment:17 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:18 Changed 8 years ago by Jeroen Demeyer

See also #16955.

comment:19 Changed 8 years ago by John Palmieri

Should this be closed as "won't fix" because of #16955? Or is there a reason to preparse to stdout and pipe the results through sage?

comment:20 in reply to:  19 Changed 8 years ago by Jeroen Demeyer

Replying to jhpalmieri:

Or is there a reason to preparse to stdout and pipe the results through sage?

Not having preparsed .py files cluttered around.

Note: See TracTickets for help on using tickets.