Change graphics_filename() to return a tmp_filename() except from the notebook

[jdemeyer]

The function graphics_filename() in sage/misc/temporary_file.py was created for the notebook. When used in other places, it is unsafe (race condition + creates predicatable filenames) and can clutter the current working directory with files. Therefore, we should use tmp_filename() in graphics_filename() except when EMBEDDED_MODE is True. This also simplifies the logic in various show() methods.

Component: misc

Author: Jeroen Demeyer

Branch: 69b285d

Reviewer: Martin von Gagern

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

[jdemeyer]

Branch: u/jdemeyer/ticket/15515

[jdemeyer]

comment:4

Attachment: doctest_no_graphics_filename.patch.gz

[rwst]

Commit: 43137d0

[rwst]

New commits:

43137d0

graphics_filename: return a tmp_filename() if not in EMBEDDED_MODE

[rwst]

Work Issues: rebase

[jdemeyer]

comment:9

rws, do you care about this patch, would you review it? I can rebase the patch, but only if I know that somebody is interested in reviewing it.

[rwst]

comment:10

No, I don't care about the notebook.

[rwst]

comment:11

Oh it's not the notebook. Yes, I do care.

[gagern]

Changed branch from u/jdemeyer/ticket/15515 to u/gagern/ticket/15515

[gagern]

Changed commit from 43137d0 to b3e656b

[gagern]

Changed work issues from rebase to none

[gagern]

comment:12

Rebased this. Funny, I was coding something along the same lines at the moment, before #16533 comment:34 made me aware of this ticket here.

My own modifications (which I have just modified to build on this here) also move some other tasks into that function, namely passing a caller-provided filename back (either unaltered or with added suffix) unless it is None. That way, one can simply do graphics_filename(filename=filename) without worrying whether the user specified a file name or not.

But since I might end up reviewing your commit, I probably should file my own as a separate ticket… I haven't looked at all your modifications yet, but I notice that code in plot3d/base.pyx is still pretty broken with regard to graphics_filename: they call it to generate some name, then strip the original extension and add an assortment of other extensions. That's just asking for a name clash. And if SAGE_TMP would happen to be world writable (although I can't imagine how that might be), then a security relevant race condition would be very likely.

If I were to fix this, I would be using my own code for this. Jeroen, could we review one another's code for this?

I guess I'd also like to use this function (with my own additions) all over the place for animations. Doing that would fix #16608 as well, would be a useful basis for my next stab at #16533, and would make 005b83f from #16571 superfluous as well.

---

New commits:

b3e656b

graphics_filename: return a tmp_filename() if not in EMBEDDED_MODE

[jdemeyer]

comment:13

Replying to @gagern:

If I were to fix this, I would be using my own code for this. Jeroen, could we review one another's code for this?

Yes, but not before the end of July. Please remind me then in case I forget.

[gagern]

comment:14

I had a closer look at the code. Decided to restore explicit arguments in two cases because they might have been used as positional arguments, not keyword arguments, so we'd break backward compatibility. This is in response to the discussion staring at #16533 comment:25. If we want to deprecate them properly, I'd suggest doing so along the lines of #16607.

Apart from that, the changes all look good. I'm doing the build, doc build, long test suite and so on tests now, and will push this change and set positive review once I'm done, unless any problems pop up.

[gagern]

comment:15

I just filed #16640 for the Graphics3d problem, since that may be harder to fix, but is already a problem without this change here applied to it, so it should be dealt with independently. Once it has been dealt with, that code can also be changed to use graphics_filename in all cases, instead of only the embedded case.

[sagetrac-git]

Changed commit from b3e656b to 69b285d

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

69b285d

Restore possibly positional arguments.

[gagern]

comment:17

OK, now even the old API with positional parameters is restored. Everything looks good: look at the code, interactive experiments, doctests, documentation build. I hope it's OK if I give this a positive review even though the last commit is mine. After all, its a minor change, mostly restoring previous code.

[gagern]

Reviewer: Martin von Gagern

[vbraun]

Changed branch from u/gagern/ticket/15515 to 69b285d

[gagern]

comment:19

Replying to @jdemeyer:

Jeroen, could we review one another's code for this?

Yes, but not before the end of July. Please remind me then in case I forget.

Jeroen, can you please have a look at #16640? Of course I'll welcome reviews for all my other modifications as well, but that ticket is closely related to what you did here.

[gagern]

Changed commit from 69b285d to none