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

[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 attachment: trac_13807_graphics_filename.patch.

Component: misc

Author: John Palmieri

Reviewer: Karl-Dieter Crisman

Merged: sage-5.5.rc1

Issue created by migration from https://trac.sagemath.org/ticket/13807

[kcrisman]

comment:2

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).

[jhpalmieri]

comment:4

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:

diff --git a/sage/misc/misc.py b/sage/misc/misc.py
--- a/sage/misc/misc.py
+++ b/sage/misc/misc.py
@@ -39,7 +39,7 @@ import operator, os, stat, socket, sys, 
 import sage.misc.prandom as random
 from lazy_string import lazy_string
 
-from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles
+from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles, graphics_filename
 
 from banner import version, banner
 

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

[jhpalmieri]

Author: John Palmieri

[kcrisman]

comment:5

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?

[jhpalmieri]

comment:6

Attachment: trac_13807_graphics_filename.patch.gz

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.

[kcrisman]

comment:7

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.

[jhpalmieri]

comment:8

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.

[kcrisman]

comment:9

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!

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:10

That is now #13812.

Patchbot, apply trac_13807_graphics_filename.patch

[jdemeyer]

Merged: sage-5.5.rc1