sage.misc.latex: Replace have_... functions by Features

[mkoeppe]

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

[mkoeppe]

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+... or more generally, replacing all uses of `sage.misc.os_tools.have_program` by `Executable`

[seblabbe]

Commit: f3291d8

[seblabbe]

Author: Sébastien Labbé

[seblabbe]

comment:2

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

[seblabbe]

Branch: u/slabbe/32650

[seblabbe]

comment:3

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f3291d8 to a5aebb9

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2e9a6b1

32650: using features in plot/graphics.py and plot/multigraphics.py

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a5aebb9 to 2e9a6b1

[seblabbe]

comment:6

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?

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ddd2287	32650: using features in plot/animate.py (currently with .require(need_msg) which is not yet supported)
d438c45	32650: using features in plot/graphics.py and plot/multigraphics.py

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2e9a6b1 to d438c45

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d455993	32650: using features in plot/animate.py
cf02094	32650: using features in plot/graphics.py and plot/multigraphics.py

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d438c45 to cf02094

[seblabbe]

comment:9

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.

[seblabbe]

comment:10

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

28f1f4a

32650: deprecating have_* functions in misc/latex.py in favor of features

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from cf02094 to 28f1f4a

[seblabbe]

comment:12

Needs review!

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 28f1f4a to 5d9414d

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

5d9414d

32650: .. note -> .. NOTE

[seblabbe]

comment:14

I added a commit to fix a pyflakes warning.

[seblabbe]

comment:15

@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_runlatex, _latex_file_ within the file sage/features/latex.py. Is this a file dependency we want to avoid with respect to modularization?

[mkoeppe]

comment:16

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.

[seblabbe]

comment:17

Replying to @mkoeppe:

and return a False test result if sage.misc.latex is not available.

False? But maybe latex is available and functional even if sage.misc.latex module is not available, so we could return True.

[mkoeppe]

comment:18

Do we have code that calls latex without going through sage.misc.latex?

[seblabbe]

comment:19

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.

[seblabbe]

comment:20

Replying to @mkoeppe:

Do we have code that calls latex without going through sage.misc.latex?

Yes, it can be done without, by using a simple subprocess.run. I do that in #20343, for instance in the method def pdf(self).

[seblabbe]

comment:21

(note to myself: I just noticed that have_latex should be updated in src/sage/doctest/external.py to use features as well.)

[mkoeppe]

comment:22

Are you still planning to add the distro info for pdf2svg here as you wrote in #20343?

[seblabbe]

comment:23

Yes.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 5d9414d to 345c760

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

345c760

32650: using features in has_latex method of doctest/external.py

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

6c6b332

32650: independent and improved method latex().is_functional()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 345c760 to 6c6b332

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 6c6b332 to c8330fe

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

63555d2	32650: adding spkg pdf2svg in build/pkgs
c8330fe	32650: linking pdf2svg feature to spkg pdf2svg

[seblabbe]

comment:27

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.

[seblabbe]

comment:28

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.

[mkoeppe]

comment:29

What did you expect to happen?

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from c8330fe to 2ea4d9e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

[seblabbe]

comment:31

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

[seblabbe]

comment:32

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.

[mkoeppe]

comment:33

While at it, also dummy script packages for ffmpeg and imagemagick could be added (see #30930)

[seblabbe]

comment:34

ok, I can do this, but next week. Going to bed now!

[mkoeppe]

Reviewer: Matthias Koeppe

[mkoeppe]

comment:36

LGTM. One green patchbot, one patchbot that is not feeling well.

[vbraun]

Changed branch from u/slabbe/32650 to 2ea4d9e