replace os.system calls in latex.py with appropriate replacements

[dandrake]

This is a followup to #8486, which uses os.system('which xelatex') to see if XeLaTeX is available. With #8474 now merged, we should use have_program to do that, and also replace other uses of os.system with appropriate subprocess replacements, since we are supposed to use subprocess, and not os.system

---

Apply only attachment: trac_8552-all-in-one.patch.

CC: @jhpalmieri

Component: misc

Author: Dan Drake, John Palmieri

Reviewer: John Palmieri, Dan Drake

Merged: sage-4.7.alpha4

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

[dandrake]

Attachment: trac_8552_whitespace.patch.gz

clean up unnecessary whitespace in latex.py

[dandrake]

replace os.system with subprocess.call; apply on top of whitespace patch

[dandrake]

comment:2

Attachment: trac_8552.patch.gz

These patches depend on the "v2" patch at #8486.

Please look this patch over. I think I've tested all the execution paths and have everything working, but I only tested it on one system, so it needs some review and testing.

A design decision that needs to be addressed: it's easiest to just do subprocess.call(), which waits for the command to finish; there are a few places where the os.system call ended with & to put the command in the background. I can reproduce that behavior with the subprocess module, but it's not as convenient, since I have to spawn the process and poll and so on. I can't detect much of a pattern or necessity to those places that possibly put the command in the background; is it okay if we just eliminate that option?

Another issue: the viewer commands from misc.viewer on Linux all return strings with a space in them: 'sage-native-execute xdg-open', which does not play nicely with subprocess; when you put that string into its call list, it tries to execute a single command with a space in it, named "sage-native-execute xdg-open" and this does not work well. It's easy enough to snag the "xdg-open" part, but if we eventually are using subprocess everywhere, we should switch the viewer commands to returning lists of strings.

[jhpalmieri]

Author: Dan Drake

[jhpalmieri]

comment:3

Overall, it looks good.

I think line 615, debug=True should be deleted. I also think that before line 1793

print 'viewer: "{0}"'.format(viewer)

we should have if debug:

I notice that you don't seem to be using "base" in the switch from

lt = 'cd "%s"&& sage-native-execute %s \\\\nonstopmode \\\\input{%s.tex} %s'%(base, command, filename, redirect)

to

lt = ['sage-native-execute', command, r'\nonstopmode', r'\input{' + filename + '.tex}'] 

But it seems to work with your patch, so I guess it's okay.

is it okay if we just eliminate that [background] option?

I think so. If you think it's worth asking around, you could post on sage-devel. Anyway, I think we can eliminate it, but we should probably keep the argument there for backwards compatibility, but have it do nothing -- this is what your patch does, right? We (meaning you) just need to document that the option no longer does anything.

Another issue: the viewer commands from misc.viewer on Linux all return strings with a space in them

If "s" is the output of one of these commands, can we do s.split() to turn it into a list, split at spaces (if there are any)? Oh, I guess that's what you're doing.

---

Summary: fix the debugging issues (the print statement), and document the fact that "do_in_background" now has no effect, and I think this is ready to go.

[jhpalmieri]

Reviewer: John Palmieri

[jhpalmieri]

comment:4

I'm attaching two new patches here. One is a referee patch, present for review only: do not apply it. The other combines all of the patches into one. Dan, if you're happy with my changes, please give this a positive review.

[jhpalmieri]

Attachment: trac_8552-ref.patch.gz

for review only, do not apply (diff between Dan's two patches and the all-in-one patch)

[jhpalmieri]

apply only this patch

[dandrake]

comment:5

Attachment: trac_8552-all-in-one.patch.gz

Thanks for finishing this, John. Sorry I left it unfinished. Your changes look good.

[jdemeyer]

Changed author from Dan Drake to Dan Drake, John Palmieri

[jdemeyer]

Changed reviewer from John Palmieri to John Palmieri, Dan Drake

[jdemeyer]

Merged: sage-4.7.alpha4