Make feature TeXFile check latex first

[soehms]

Currently, as observed in comment 7 of #34185, the method is_present of instances of class TeXFile runs into an exception if latex is not installed. For example:

sage: from sage.features.latex import LaTeXPackage
sage: LaTeXPackage("tkz-graph").is_present()
Traceback (most recent call last):
...
FileNotFoundError: [Errno 2] No such file or directory: 'kpsewhich'

The aim of the ticket is to fix this.

CC: @seblabbe

Component: doctest framework

Keywords: latex texfile feature

Author: Sebastian Oehms, Kwankyu Lee

Branch/Commit: a77ff0c

Reviewer: Kwankyu Lee, Sebastian Oehms

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

[soehms]

Branch: u/soehms/join_feature_texfile_with_pdflatex_34282

[soehms]

Commit: c599e49

[soehms]

New commits:

c599e49

34282: initial

[soehms]

Author: Sebastian Oehms

[mkoeppe]

comment:3

I think it would also make sense to add spkg='texlive' to these features

[sagetrac-git]

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

7358dc2

34282: dd spkg='texlive', refactoring, pycodestyle

[sagetrac-git]

Changed commit from c599e49 to 7358dc2

[soehms]

comment:5

Replying to @mkoeppe:

I think it would also make sense to add spkg='texlive' to these features

Done! Furthermore, I refactor the executable features not only to avoid duplicated code, but also since in my point of view it is not reasonable that just the latex feature has a stronger is_functional check.

[kwankyu]

comment:6

diff --git a/src/sage/features/latex.py b/src/sage/features/latex.py
index cf65aea6af..7db89f6190 100644
--- a/src/sage/features/latex.py
+++ b/src/sage/features/latex.py
@@ -37,7 +37,7 @@ class LaTeX(Executable):
             sage: isinstance(latex(), latex)
             True
         """
-        Executable.__init__(self, name, executable=name, spkg=latex_spkg, url=latex_url)
+        super().__init__(name, executable=name, spkg=latex_spkg, url=latex_url)
 
     def is_functional(self):
         r"""
@@ -217,13 +217,10 @@ class TeXFile(StaticFile, JoinFeature):
             sage: from sage.features.latex import LaTeXPackage, pdflatex
             sage: f = LaTeXPackage("tkz-graph")
             sage: g = pdflatex()
-            sage: bool(f.is_present()) == bool(g.is_present())  # indirect doctest
+            sage: not f.is_present() or bool(g.is_present())  # indirect doctest
             True
         """
-        test = JoinFeature._is_present(self)
-        if not test:
-            return test
-        return super(TeXFile, self)._is_present()
+        return JoinFeature._is_present(self) and StaticFile._is_present(self)
 

[kwankyu]

comment:7

By the way, why presence of kpsewhich is checked by the presence of pdflatex? kpsewhich is a component of tex installation. Checking if texlive is present isn't enough?

[soehms]

comment:8

Replying to @kwankyu:

diff --git a/src/sage/features/latex.py b/src/sage/features/latex.py
index cf65aea6af..7db89f6190 100644
--- a/src/sage/features/latex.py
+++ b/src/sage/features/latex.py
@@ -37,7 +37,7 @@ class LaTeX(Executable):
             sage: isinstance(latex(), latex)
             True
         """
-        Executable.__init__(self, name, executable=name, spkg=latex_spkg, url=latex_url)
+        super().__init__(name, executable=name, spkg=latex_spkg, url=latex_url)
 
     def is_functional(self):
         r"""
@@ -217,13 +217,10 @@ class TeXFile(StaticFile, JoinFeature):
             sage: from sage.features.latex import LaTeXPackage, pdflatex
             sage: f = LaTeXPackage("tkz-graph")
             sage: g = pdflatex()
-            sage: bool(f.is_present()) == bool(g.is_present())  # indirect doctest
+            sage: not f.is_present() or bool(g.is_present())  # indirect doctest
             True
         """
-        test = JoinFeature._is_present(self)
-        if not test:
-            return test
-        return super(TeXFile, self)._is_present()
+        return JoinFeature._is_present(self) and StaticFile._is_present(self)
 

I'm fine with your first suggestion. Concerning the second, the advantage is not clear to me.

With your third suggestion I have a problem since in general I prefer to serialise condition checks if they only work in a certain order. I know that it is no problem any more with modern languages and compilers, since they implicitly serialise in the order they are written. But the syntax using and / or pretends a parallelism which is not given in such a situation.

[soehms]

comment:9

Replying to @kwankyu:

By the way, why presence of kpsewhich is checked by the presence of pdflatex? kpsewhich is a component of tex installation. Checking if texlive is present isn't enough?

AFAIK there is no feature that checks if texlive is just installed, no? Do you want to create a new feature or just replace the join with pdflatex by latex?

[kwankyu]

comment:10

Replying to @soehms:

Concerning the second, the advantage is not clear to me.

Here you want to check that "if f.is_present, then g.is_present". Right? My test just does that expressly.

With your third suggestion I have a problem since in general I prefer to serialise condition checks if they only work in a certain order. I know that it is no problem any more with modern languages and compilers, since they implicitly serialise in the order they are written. But the syntax using and / or pretends a parallelism which is not given in such a situation.

There is no parallelism here. My simple code exactly does what your code does. Note that

sage: 0 and 1
0
sage: 1 and 2
2

In A and B, B is never evaluated if A is false.

[soehms]

comment:11

Replying to @kwankyu:

Replying to @soehms:

Concerning the second, the advantage is not clear to me.

Here you want to check that "if f.is_present, then g.is_present". Right? My test just does that expressly.

Convinced! I had if and only if in mind since AFAIK in reallity f is always present if g is.

With your third suggestion I have a problem since in general I prefer to serialise condition checks if they only work in a certain order. I know that it is no problem any more with modern languages and compilers, since they implicitly serialise in the order they are written. But the syntax using and / or pretends a parallelism which is not given in such a situation.

There is no parallelism here.

I was saying pretends a parallelism.

My simple code exactly does what your code does.

So what is the advantage?

Note that

sage: 0 and 1
0
sage: 1 and 2
2

In A and B, B is never evaluated if A is false.

Maybe I'm old fashioned, but I know a time where you could not be sure that B is never evaluated if A is false (at least in C).

[kwankyu]

comment:12

Replying to @soehms:

My simple code exactly does what your code does.

So what is the advantage?

It took me some time to understand the purpose of your code. My code expresses the purpose.

In A and B, B is never evaluated if A is false.

Maybe I'm old fashioned, but I know a time where you could not be sure that B is never evaluated if A is false (at least in C).

Yeah, this is just Python:

https://docs.python.org/3/reference/expressions.html#boolean-operations

[kwankyu]

comment:13

Replying to @soehms:

Replying to @kwankyu:

By the way, why presence of kpsewhich is checked by the presence of pdflatex? kpsewhich is a component of tex installation. Checking if texlive is present isn't enough?

...or just replace the join with pdflatex by latex?

Yes. I know that they are practically the same, but the name latex can be understood as the latex system(texlive) but pdflatex refers to the program.

[soehms]

comment:14

Replying to @kwankyu:

Replying to @soehms:

My simple code exactly does what your code does.

So what is the advantage?

It took me some time to understand the purpose of your code. My code expresses the purpose.

But it might be misleading for people who think and is a commutative binary operator which is not very unusual for mathematicians :)

[soehms]

comment:15

Replying to @kwankyu:

Replying to @soehms:

Replying to @kwankyu:

By the way, why presence of kpsewhich is checked by the presence of pdflatex? kpsewhich is a component of tex installation. Checking if texlive is present isn't enough?

...or just replace the join with pdflatex by latex?

Yes. I know that they are practically the same, but the name latex can be understood as the latex system(texlive) but pdflatex refers to the program.

Agreed!

I'm on vaccation the next two week. If you don't want to wait that long, feel free to do all of the changes you suggested by yourself.

[kwankyu]

comment:16

Replying to @soehms:

But it might be misleading for people who think and is a commutative binary operator which is not very unusual for mathematicians :)

But for python programmers, the above use of and is a common idiom. Don't worry.

[kwankyu]

comment:17

Replying to @soehms:

I'm on vaccation the next two week. If you don't want to wait that long, feel free to do all of the changes you suggested by yourself.

Okay. Happy vacation!

[soehms]

comment:18

Replying to @kwankyu:

Replying to @soehms:

I'm on vaccation the next two week. If you don't want to wait that long, feel free to do all of the changes you suggested by yourself.

Okay. Happy vacation!

Thanks!

[kwankyu]

Changed branch from u/soehms/join_feature_texfile_with_pdflatex_34282 to u/klee/join_feature_texfile_with_pdflatex_34282

[kwankyu]

comment:20

While fixing things, I could not resist from doing more than I intended. Sorry...

It seemed that TeXFile doesn't need to be a subclass of JoinFeature.

Instead, TeXFile now simply checks latex presence first.

---

New commits:

a5900ac	Merge branch 'develop' into t/34282/join_feature_texfile_with_pdflatex_34282
9e6ca2a	Replace pdflatex to latex
553f6ff	Do not use JoinFeature

[kwankyu]

Changed commit from 7358dc2 to 553f6ff

[kwankyu]

Changed author from Sebastian Oehms to Sebastian Oehms, Kwankyu Lee

[sagetrac-git]

Changed commit from 553f6ff to a31205a

[sagetrac-git]

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

a31205a

A tiny edit

[soehms]

comment:23

Replying to @kwankyu:

While fixing things, I could not resist from doing more than I intended. Sorry...

It seemed that TeXFile doesn't need to be a subclass of JoinFeature.

Instead, TeXFile now simply checks latex presence first.

Note that #34185 has a dependency on this ticket and I think this would break things there. More precisely, the method joined_features defined in class Feature will not work correctly for TeX files. So at least you should find another solution for that.

[kwankyu]

comment:24

Replying to @soehms:

Note that #34185 has a dependency on this ticket and I think this would break things there.

Probably.

More precisely, the method joined_features defined in class Feature will not work correctly for TeX files. So at least you should find another solution for that.

The purpose of this ticket as in description (but not as in old title) is to fix a bug in TeXFile feature, which did not check latex system in place before it check the presence of a TeX file (such as a latex package). This has nothing to do with #34185.

Hence I think that any changes to TeXFile feature related with #34185 should be made in #34185 itself.

Moreover, as far as I understand #34185, the goal of #34185 is not clear to me and I doubt if the goal is reasonable. So the present ticket is better to be independent from #34185.

[kwankyu]

comment:25

By the way, I am positive with this ticket.

[kwankyu]

Reviewer: Kwankyu Lee

[soehms]

comment:27

Replying to @kwankyu:

By the way, I am positive with this ticket.

I will not be able to look at your additional changes before I will be back home again.

At the moment I am still not convinced to remove the inheritance of JoinFeature (independently of #34185), since as fas as I understand this is the conceptual way to indicate dependencies between features.

[kwankyu]

comment:28

Replying to @soehms:

Replying to @kwankyu:

By the way, I am positive with this ticket.

I will not be able to look at your additional changes before I will be back home again.

No hurry. Enjoy vacation!

At the moment I am still not convinced to remove the inheritance of JoinFeature (independently of #34185), since as fas as I understand this is the conceptual way to indicate dependencies between features.

I think JoinFeature is not relevant for the bug. A joined feature is conceptually a new feature "A and B" combining A and B. TeXFile is a feature A that requires feature B. Conceptually it is "A => B".

[soehms]

Changed branch from u/klee/join_feature_texfile_with_pdflatex_34282 to u/soehms/join_feature_texfile_with_pdflatex_34282

[soehms]

comment:30

Replying to Kwankyu Lee:

I think JoinFeature is not relevant for the bug. A joined feature is conceptually a new feature "A and B" combining A and B. TeXFile is a feature A that requires feature B. Conceptually it is "A => B".

Indeed, according to the examples given in the docstring of the class you are right. But it is at least misleading that most examples of joined features consist of just one:

sage: from sage.features.all import all_features
sage: from sage.features.join_feature import JoinFeature
sage: J = [F for F in all_features() if isinstance(F, JoinFeature)]
sage: len([F for F in J if len(F._features) == 1])
16
sage: len([F for F in J if len(F._features) > 1])
7

In these cases the instance of JoinFeature usually refers to a SPKG (for example meataxe) which is combined with a Python module (in the example meataxe this is sage.matrix.matrix_gfpn_dense) implementing the integration of the new functionality into Sage. Since the Python module requires the SPKG the construction describes a dependency here, as well. But I confess that it is reverse on how I used it.

Finally, I agree with your changes, but I observed two little things that I have corrected. If you agree and the patchbot comes green again, you may set to positive review.

[soehms]

Changed commit from a31205a to a77ff0c

[soehms]

Changed reviewer from Kwankyu Lee to Kwankyu Lee, Sebastian Oehms

[kwankyu]

comment:32

Replying to Sebastian Oehms:

In these cases the instance of JoinFeature usually refers to a SPKG (for example meataxe) which is combined with a Python module (in the example meataxe this is sage.matrix.matrix_gfpn_dense) implementing the integration of the new functionality into Sage. Since the Python module requires the SPKG the construction describes a dependency here, as well. But I confess that it is reverse on how I used it.

It seems JoinFeature is also used as unary. In your example, the feature Meataxe is a JoinFeature of the Python module sage.matrix.matrix_gfpn_dense only. Any feature is provided by an SPKG. In this example, the presence of the Python module implies the presence of the SPKG meataxe. Hence I think the feature Meataxe uses the unary JoinFeature to check the presence of the SPKG meataxe. In general, unary JoinFeature seems to be used to check the presence of an SPKG.

In our case, we don't need the trick since we can check the presence of the latex feature.

Finally, I agree with your changes, but I observed two little things that I have corrected. If you agree and the patchbot comes green again, you may set to positive review.

Thanks. I am positive too!

[soehms]

comment:33

Replying to Kwankyu Lee:

Replying to Sebastian Oehms:
It seems JoinFeature is also used as unary. In your example, the feature Meataxe is a JoinFeature of the Python module sage.matrix.matrix_gfpn_dense only. Any feature is provided by an SPKG. In this example, the presence of the Python module implies the presence of the SPKG meataxe. Hence I think the feature Meataxe uses the unary JoinFeature to check the presence of the SPKG meataxe. In general, unary JoinFeature seems to be used to check the presence of an SPKG.

Exactly! I think JoinFeature is missing some documentation to make these things clear. Shall I open a ticket?

Thanks. I am positive too!

Thanks, as well!

[mkoeppe]

comment:34

Replying to Kwankyu Lee:

It seems JoinFeature is also used as unary. In your example, the feature Meataxe is a JoinFeature of the Python module sage.matrix.matrix_gfpn_dense only. Any feature is provided by an SPKG. In this example, the presence of the Python module implies the presence of the SPKG meataxe. Hence I think the feature Meataxe uses the unary JoinFeature to check the presence of the SPKG meataxe. In general, unary JoinFeature seems to be used to check the presence of an SPKG.

The name of a feature gives the # optional tag. I have used unary JoinFeatures to equip features such as PythonModule features with a tag name that differs from the systematic tag name.

[kwankyu]

comment:35

Replying to Sebastian Oehms:

I think JoinFeature is missing some documentation to make these things clear. Shall I open a ticket?

That would be nice. Matthias explains clearly how unary JoinFeature can be used.

[soehms]

comment:36

Replying to Kwankyu Lee:

Replying to Sebastian Oehms:

I think JoinFeature is missing some documentation to make these things clear. Shall I open a ticket?

That would be nice. Matthias explains clearly how unary JoinFeature can be used.

This is now #34508!

[vbraun]

Changed branch from u/soehms/join_feature_texfile_with_pdflatex_34282 to a77ff0c