sage.misc.latex: Replace have_... functions by Features
mkoeppe opened this issue · 62 comments
... or more generally, replacing all uses of sage.misc.os_tools.have_program by Executable
Depends on #32634
CC: @kwankyu
Component: refactoring
Author: Sébastien Labbé
Branch: 2ea4d9e
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/32650
Description changed:
---
+++
@@ -1 +1 @@
-
+... or more generally, replacing all uses of `sage.misc.os_tools.have_program` by `Executable`Author: Sébastien Labbé
Here is some work towards that goal removing all uses of have_program in misc/latex.py.
New commits:
8068ba5 | 32650: Creating feature dvipng |
a96e94d | 32650: Creating feature pdf2svg |
870d461 | 32650: Creating features latex, xelatex, pdflatex, lualatex |
385569e | 32650: adding latex, xelatex, pdflatex, lualatex, pdf2svg, dvipng to doctest/external.py |
f3291d8 | 32650: using features instead of have_program in misc/latex.py |
Branch: u/slabbe/32650
Remainings usage of have_program are:
$ git grep have_program
interfaces/chomp.py: from sage.misc.sage_ostools import have_program
interfaces/chomp.py: _have_chomp[program] = have_program(program)
interfaces/phc.py: from sage.misc.sage_ostools import have_program
interfaces/phc.py: if not have_program('phc'):
misc/dist.py: from sage.misc.sage_ostools import have_program
misc/dist.py: cmd_inside_sage = have_program(cmd, path=SAGE_BIN)
misc/dist.py: cmd_outside_sage = have_program(cmd, path=PATH)
misc/sage_ostools.pyx:def have_program(program, path=None):
misc/sage_ostools.pyx: sage: from sage.misc.sage_ostools import have_program
misc/sage_ostools.pyx: sage: have_program('ls')
misc/sage_ostools.pyx: sage: have_program('there_is_not_a_program_with_this_name')
misc/sage_ostools.pyx: sage: have_program('sage', os.path.join(SAGE_VENV, 'bin'))
misc/sage_ostools.pyx: sage: have_program('sage', '/there_is_not_a_path_with_this_name')
misc/sage_ostools.pyx: sage: have_program('there_is_not_a_program_with_this_name', os.path.join(SAGE_VENV, 'bin'))
misc/viewer.py: from sage.misc.sage_ostools import have_program
misc/viewer.py: elif have_program('xdg-open'):
misc/viewer.py: if have_program(cmd):
misc/viewer.py: if have_program(cmd):
misc/viewer.py: if have_program(cmd):
plot/animate.py: from sage.misc.sage_ostools import have_program
plot/animate.py: have_convert = have_program('convert')
plot/animate.py: from sage.misc.sage_ostools import have_program
plot/animate.py: return have_program('ffmpeg')
plot/graphics.py: from sage.misc.sage_ostools import have_program
plot/graphics.py: if have_program(i)]
plot/multigraphics.py: from sage.misc.sage_ostools import have_program
plot/multigraphics.py: if have_program(i)]
Branch pushed to git repo; I updated commit sha1. New commits:
a5aebb9 | 32650: using features in plot/animate.py (currently with .require(need_msg) which is not yet supported) |
Branch pushed to git repo; I updated commit sha1. New commits:
2e9a6b1 | 32650: using features in plot/graphics.py and plot/multigraphics.py |
I suggest this ticket should handle only the removal of have_program in misc.latex.py and sage.plot.
TODO: Answer this question: do we want .require(needed_for_msg) to provide an additional message explaining why the feature is needed for?
Replying to @seblabbe:
TODO: Answer this question: do we want
.require(needed_for_msg)to provide an additional message explaining why the feature is needed for?
I decided to get rid of the .require(needed_for_msg). I did a forced push to clean the commit about plot/animate.py file.
On my side, the command
$ sage -tp --optional=sage,optional,external --long src/sage/features src/sage/plot src/sage/doctest
currently gives:
Git branch: 32650
Using --optional=4ti2,bliss,cbc,ccache,cryptominisat,database_symbolic_data,debian,dot2tex,e_antic,external,fricas,glucose,latte_int,libnauty,lidia,normaliz,notedown,pandoc_attributes,pip,pycosat,pynormaliz,rst2ipynb,rubiks,sage,sage.geometry.polyhedron,sage.rings.real_double,sage_numerical_backends_coin,sage_spkg,sage_sws2rst
External software to be detected: 4ti2,cplex,dvipng,ffmpeg,graphviz,gurobi,imagemagick,internet,latex,lualatex,macaulay2,magma,maple,mathematica,matlab,octave,pandoc,pdf2svg,pdflatex,rubiks,scilab,xelatex
Sorting sources by runtime so that slower doctests are run first....
Doctesting 95 files using 8 threads.
[...]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 455.6 seconds
cpu time: 680.6 seconds
cumulative wall time: 2815.7 seconds
External software detected for doctesting: dvipng,ffmpeg,graphviz,imagemagick,internet,latex,lualatex,octave,pandoc,pdf2svg,pdflatex,xelatex
Branch pushed to git repo; I updated commit sha1. New commits:
28f1f4a | 32650: deprecating have_* functions in misc/latex.py in favor of features |
Needs review!
Branch pushed to git repo; I updated commit sha1. New commits:
5d9414d | 32650: .. note -> .. NOTE |
I added a commit to fix a pyflakes warning.
@mkoeppe: I have a question for you with respect to a choice I made I am not sure of. Within the class class latex(Executable), I addded the method is_functional where I reused the code which was in the previous have_latex in sage.misc.latex.py. This allows to have a behavior which matches the previous behavior when have_latex was called. But doing so, there are now imports from sage.misc.latex import run_latex, _latex_file_ within the file sage/features/latex.py. Is this a file dependency we want to avoid with respect to modularization?
Great question. In this code:
+ def is_functional(self):
...
+ from sage.misc.latex import _run_latex_, _latex_file_
it would probably be good to use try...except around the import and return a False test result if sage.misc.latex is not available.
Replying to @mkoeppe:
and return a
Falsetest result ifsage.misc.latexis not available.
False? But maybe latex is available and functional even if sage.misc.latex module is not available, so we could return True.
Do we have code that calls latex without going through sage.misc.latex?
In fact, I hate the function _run_latex_ as it is a bunch of spaghetti code involving everything:
https://gitlab.com/sagemath/sage/-/blob/develop/src/sage/misc/latex.py#L643
So I think it is preferable to not call it within features/latex.py at all.
(note to myself: I just noticed that have_latex should be updated in src/sage/doctest/external.py to use features as well.)
Are you still planning to add the distro info for pdf2svg here as you wrote in #20343?
Yes.
Branch pushed to git repo; I updated commit sha1. New commits:
345c760 | 32650: using features in has_latex method of doctest/external.py |
Branch pushed to git repo; I updated commit sha1. New commits:
6c6b332 | 32650: independent and improved method latex().is_functional() |
So I created the build/pkgs/pdf2svg by immitating what was done for build/pkgs/pandoc and build/pkgs/graphviz. I hope it makes sense.
While testing sage -i pdf2svg, I noticed this strange part in the output:
Error: pdf2svg is a dummy script package that the Sage distribution uses
to provide information about equivalent system packages.
It cannot be installed using the Sage distribution.
Please install it manually, for example using the system packages
recommended at the end of a run of './configure'
See below for package-specific information.
maybe this message should be fixed in another ticket.
What did you expect to happen?
Branch pushed to git repo; I updated commit sha1. New commits:
2ea4d9e | 32650: fixing two doctests now that we latex().require() it when we use it |
Replying to @mkoeppe:
What did you expect to happen?
Well, the part that surprised me is Error: pdf2svg is a dummy script package, which I attributed to _prereq, _recommmended and _bootstrap. But, now, rereading it again, I understand what it means. It is just that it does not contain the source. Maybe dummy could be replaced by another word which better describe what it is? Or just leave it as it is...
Ok, so this ticket now needs review. I did not have anything else in mind to be done with respect to have_... within sage.misc.latex.py and sage.plot.
While at it, also dummy script packages for ffmpeg and imagemagick could be added (see #30930)
ok, I can do this, but next week. Going to bed now!
Reviewer: Matthias Koeppe
LGTM. One green patchbot, one patchbot that is not feeling well.
Changed branch from u/slabbe/32650 to 2ea4d9e
There does not seem to be a Cygwin package for pdf2svg.
At least I could not find one by searching Cygwin packages via
and the command apt-cyg install pdf2svg fails.
Do you mean that the file build/pkgs/pdf2svg/distros/cygwin.txt should be removed?
Yes, otherwise configure ends with a recommendation
to run apt-cyg install pdf2svg.
Or package pdf2svg for Cygwin... : )
A request on the Cygwin mailing list to package pdf2svg
was answered as follows:
The pdf2svg home page recommends instead pdftocairo
from the poppler package, which does the same and
is maintained as part of the package, rather than
being a standalone utility which integrates the two,
and is not as well maintained:$ pdftocairo --help [...]
That suggests pdftocairo --svg file.pdf
will transform file.pdf to file.svg.
It seems poppler is widely packaged:
Then, should we replace pdf2svg feature by pdftocairo?
why do we have has_xelatex and have_xelatex ?! (which seem to be doing the same thing, no?)