Opened 7 years ago

Closed 7 years ago

#13807 closed defect (fixed)

Fix remaining temporary filename issues (like in animate.py)

Reported by: kcrisman Owned by: jason
Priority: blocker Milestone: sage-5.5
Component: misc Keywords: beginner
Cc: Merged in: sage-5.5.rc1
Authors: John Palmieri Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

In #13579, for security reasons and modularity, we moved temporary filename constructors from sage.misc.misc to their own module. Most things were moved with it, but a couple places in animate.py were missed. Let's make sure that all such things, including these, are fixed in this ticket.

See this ask.sagemath.org question for a real-life bug this caused.

Apply trac_13807_graphics_filename.patch.

Attachments (1)

trac_13807_graphics_filename.patch (2.7 KB) - added by jhpalmieri 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by kcrisman

  • Description modified (diff)

comment:2 Changed 7 years ago by kcrisman

  • Keywords beginner added
  • Priority changed from critical to blocker

In Sage-5.5.rc0:

$ grep -r sage.misc.misc.graphics_filename .
./matrix/matrix_modn_sparse.c: *             filename = sage.misc.misc.graphics_filename()
./matrix/matrix_modn_sparse.c: *             filename = sage.misc.misc.graphics_filename()             # <<<<<<<<<<<<<<
./matrix/matrix_modn_sparse.c: *             filename = sage.misc.misc.graphics_filename()
./matrix/matrix_modn_sparse.pyx:            filename = sage.misc.misc.graphics_filename()
./plot/animate.py:                savefile = sage.misc.misc.graphics_filename(ext='gif')
./plot/animate.py:                savefile = sage.misc.misc.graphics_filename(ext=output_format)
$ grep -r sage.misc.misc.tmp_filename .
./interfaces/phc.py:        input_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        output_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:            start_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        input_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        output_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:            start_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        input_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        output_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        log_filename = sage.misc.misc.tmp_filename()
./misc/interpreter.py:    tmpfilename = sage.misc.misc.tmp_filename('preparse', ext='.py')
./plot/plot3d/base.c: *         self.output_file = sage.misc.misc.tmp_filename()             # <<<<<<<<<<<<<<
./plot/plot3d/base.c: *         self.output_file = sage.misc.misc.tmp_filename()
./plot/plot3d/base.c: *         self.output_file = sage.misc.misc.tmp_filename()
./plot/plot3d/base.pyx:        self.output_file = sage.misc.misc.tmp_filename()
./rings/number_field/totallyreal_phc.py:        tmpfile = sage.misc.misc.tmp_filename()
$ grep -r sage.misc.misc.tmp_dir .
./server/support.py:        sage: os.chdir(sage.misc.misc.tmp_dir('server_doctest'))
$ grep -r sage.misc.misc.delete_tmpfiles .

Ignoring c files, that means we need to fix

./server/support.py:        sage: os.chdir(sage.misc.misc.tmp_dir('server_doctest'))
./plot/plot3d/base.pyx:        self.output_file = sage.misc.misc.tmp_filename()
./rings/number_field/totallyreal_phc.py:        tmpfile = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        input_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        output_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:            start_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        input_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        output_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:            start_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        input_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        output_filename = sage.misc.misc.tmp_filename()
./interfaces/phc.py:        log_filename = sage.misc.misc.tmp_filename()
./misc/interpreter.py:    tmpfilename = sage.misc.misc.tmp_filename('preparse', ext='.py')
./matrix/matrix_modn_sparse.pyx:            filename = sage.misc.misc.graphics_filename()
./plot/animate.py:                savefile = sage.misc.misc.graphics_filename(ext='gif')
./plot/animate.py:                savefile = sage.misc.misc.graphics_filename(ext=output_format)

Huh, that's more than I was expecting. Anyway, it's seven files.

I'm making this a 5.5 blocker because of the bug when EMBEDDED_MODE=True for showing animate, which is a regression due to a bugfix. I'm not sure whether any of these can actually be doctested, though, and if a few people disagree we can make it critical. I just really hate regressions caused by bug fixes (even when I cause them, luckily not the case here).

comment:3 Changed 7 years ago by kcrisman

  • Milestone changed from sage-5.6 to sage-5.5

comment:4 follow-up: Changed 7 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Status changed from new to needs_review

The file sage/misc/misc.py has a line

from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles

so I think that sage.misc.misc.tmp_filename should work, and the same for tmp_dir. For example, coding/linear_code.py has a function code2leon with the lines

    from sage.misc.misc import tmp_filename
    ...
    file_loc = tmp_filename()

and it seems to work fine. So I think we only need to fix the graphics_filename issue. One way to do it:

  • sage/misc/misc.py

    diff --git a/sage/misc/misc.py b/sage/misc/misc.py
    a b import operator, os, stat, socket, sys,  
    3939import sage.misc.prandom as random
    4040from lazy_string import lazy_string
    4141
    42 from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles
     42from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles, graphics_filename
    4343
    4444from banner import version, banner
    4545

But it's also not much harder (I think) to fix each individual use of graphics_filename. See attached patch.

comment:5 in reply to: ↑ 4 Changed 7 years ago by kcrisman

The file sage/misc/misc.py has a line

from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles

I was wondering about this, but since this was broken I figured it was all broken... sorry for that wasted time.

But it's also not much harder (I think) to fix each individual use of graphics_filename. See attached patch.

Agreed. This patch is fine, modulo the next comment.


This could have been caught if we had a way to enable this currently untested doctest. The same problem in the matrix file - it's precisely when we don't pass in the filename that this branch of the code happens... sigh. Of course, that function is completely untested.

Can you think of any way to get around that and add a doctest to this patch, or is it hopeless without doing some pointless refactoring of code? William is very clear about not wanting to leave random files around... maybe we could at least doctest the matrix one and then remove that file? We've tried to get away from ad hoc file creation and removal, but in this case I think that's the only way to do it right.


Incidentally, ran into the sage -n not starting while testing this... so I'm glad that will be fixed before the final 5.5, right?

Last edited 7 years ago by kcrisman (previous) (diff)

Changed 7 years ago by jhpalmieri

comment:6 follow-up: Changed 7 years ago by jhpalmieri

I've added a doctest (not a perfect one, but the best I can think of) in matrix_modn_sparse.pyx. I don't think it's possible to add a doctest to animate.py, since those require ImageMagick or similar to run.

For the sage -n problem, see #13794. I think the scripts patch at #11409 will fix it.

comment:7 in reply to: ↑ 6 Changed 7 years ago by kcrisman

I've added a doctest (not a perfect one, but the best I can think of) in matrix_modn_sparse.pyx.

This makes sense, but unfortunately it doesn't actually show up in the notebook. In fact, even at the command line it shows up in the current working directory, which is not standard Sage behavior for graphics, though it is the documented behavior. My personal preference would be to have it changed, but that wouldn't be this ticket. What do you think the "right" behavior should be for the notebook and EMBEDDED_MODE? Not that I'm angling for a line for that situation, but it would be nice to have.

Actually, this must be a really old function, since matrix_plot does what we want here. hg blame tells me it's an addition by malb from 2007!

I don't think it's possible to add a doctest to animate.py, since those require ImageMagick or similar to run.

Sounds reasonable, as I suspected but hoped against.


Sorry about asking the matrix notebook question. I hate it when we discover bugs or unneeded functionality when trying to do very routine fixes. I'd be tempted to just can the function entirely, except there's the deprecation period and we should probably ask Martin if it's needed...


Random annoyance this has nothing to do with.

warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:110:8: Unreachable code
warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:136:8: Unreachable code
warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:201:8: Unreachable code
warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:204:8: Unreachable code
warning: sage/matrix/../modules/vector_integer_sparse_c.pxi:264:16: local variable 'tmp' referenced before assignment

Are these actual unused branches or is the compiler just blowing off steam? Anyway, not worrisome here.

For the sage -n problem, see #13794. I think the scripts patch at #11409 will fix it.

Yes, I just couldn't remember the ticket numbers, I wasn't actually complaining.

comment:8 Changed 7 years ago by jhpalmieri

It looks like visualize_matrix does one thing that matrix_plot does not: it lets you put a bound on the size of the figure, and then scales the picture accordingly. So if you have a 20x20 matrix and plot it in a 10x10 picture, each pixel corresponds to the entries in a 2x2 block. Maybe it would make sense to merge this functionality into matrix_plot and then deprecate visualize_matrix. Then we wouldn't have to worry about what this should do in the notebook. I agree that it would be more consistent for it to display the picture, but at least the current behavior is documented.

I have no idea about those warnings about unreachable code, by the way.

comment:9 Changed 7 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Okay, sounds good enough. I'll open a new ticket to ask about this other stuff for visualize_matrix, then. Thanks for the quick patch!

comment:10 Changed 7 years ago by kcrisman

  • Description modified (diff)

That is now #13812.

Patchbot, apply trac_13807_graphics_filename.patch

comment:11 Changed 7 years ago by jdemeyer

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