Improve is_functional of lrs feature

[seblabbe]

From #33101 comment:3 :

"This was after a distclean. I had not explicitly installed lrslib, it was on the system. If I understand the config.log correctly, the system lrslib was not accepted by Sage. Yet, ptestlong runs the optional lrslib tests."

## ------------------------------------------------------- ##
## Checking whether SageMath should install SPKG lrslib... ##
## ------------------------------------------------------- ##
configure:30190: checking whether any of gmp flint is installed as or will be installed as SPKG
configure:30194: result: yes; install lrslib as well
configure:30296: no suitable system package found for SPKG lrslib

---

In this ticket, we add to the is_functional method a check corresponding to the check made in build/pkgs/lrslib/spkg-configure.m4 so that if lrslib is not picked up by sagemath at configuration/build time, then it is not picked up by sagemath at runtime or during doctests.

CC: @mwageringel @orlitzky

Component: packages: optional

Author: Sébastien Labbé, Markus Wageringel

Branch: 16e1b24

Reviewer: Matthias Koeppe

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

[seblabbe]

Commit: 5ecec1d

[seblabbe]

New commits:

0342538	33101: adding a second check in lrs feature is_functional
5ecec1d	33101: simplification of check #1 in is_functional method of lrs feature

[seblabbe]

Branch: u/slabbe/33167

[sagetrac-git]

Changed commit from 5ecec1d to f1d1269

[sagetrac-git]

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

32edecb	33167: adding a second check in lrs feature is_functional
f1d1269	33167: simplification of check #1 in is_functional method of lrs feature

[seblabbe]

comment:3

Forced push to rename the commit message to mention #33167 instead of #33101.

[seblabbe]

comment:4

I wonder if we should not check both lrs and lrsnash executables in the same is_functional method. Maybe a JoinFeature would be better here, as it is done for instance for graphviz. What does the reviewer think?

[seblabbe]

comment:5

(this comment is copied from #33101 comment:19 where I originally posted the branch)

For now, I just imported the test made in the m4 file into the is_functional method. On my machine everything works, so I can not test it properly. If I made stuff correctly, the following should now return False for affected users like gh-mwageringel :

sage: from sage.features.lrs import Lrs
sage: Lrs().is_present()
FeatureTestResult('lrslib', True)
sage: Lrs().is_functional()
FeatureTestResult('lrslib', True)

[seblabbe]

Author: Sébastien Labbé

[orlitzky]

comment:9

Why do we need a feature for lrs at all when we have an optional package for it? If we simply remove the feature, the spkg-configure check will handle this, won't it?

[orlitzky]

comment:10

Replying to @orlitzky:

Why do we need a feature for lrs at all when we have an optional package for it? If we simply remove the feature, the spkg-configure check will handle this, won't it?

Nevermind, I found #30746.

[mwageringel]

comment:11

Replying to @seblabbe:

For now, I just imported the test made in the m4 file into the is_functional method. On my machine everything works, so I can not test it properly. If I made stuff correctly, the following should now return False for affected users like gh-mwageringel :

This is not the case, I am afraid. I get the same output:

sage: from sage.features.lrs import Lrs
sage: Lrs().is_present()
FeatureTestResult('lrslib', True)
sage: Lrs().is_functional()
FeatureTestResult('lrslib', True)

My system lrslib seems to be mostly functional, as all the doctests apart from #33101 pass. But it is rejected by the .m4 file because of the dependency on flint – my Sage builds its own flint, so would also build its own lrslib.

[orlitzky]

comment:12

Replying to @mwageringel:

My system lrslib seems to be mostly functional, as all the doctests apart from #33101 pass. But it is rejected by the .m4 file because of the dependency on flint – my Sage builds its own flint, so would also build its own lrslib.

This is probably a mistake in the spkg-configure.m4 for lrslib. If all we do is run the executables from lrslib, we don't care if they're linked to a mismatched version of flint/gmp.

[mwageringel]

Changed commit from f1d1269 to bb047c7

[mwageringel]

Changed branch from u/slabbe/33167 to u/gh-mwageringel/33167

[mwageringel]

Changed author from Sébastien Labbé to Sébastien Labbé, Markus Wageringel

[mwageringel]

comment:13

Replying to @orlitzky:

Replying to @mwageringel:
This is probably a mistake in the spkg-configure.m4 for lrslib. If all we do is run the executables from lrslib, we don't care if they're linked to a mismatched version of flint/gmp.

Yes, this seems to be the case. I have removed the check for gmp/flint from the .m4 file. Now it works as I would expect – this is the relevant output of configure:

Checking whether SageMath should install SPKG lrslib...
checking for lrsnash... lrsnash
checking whether lrsnash can handle the new input format... yes
configure: will use system package and not install SPKG lrslib

This is the first time I have edited an .m4 file, so I hope I did not break anything.

Besides that, I have removed a print call from the is_functional check which was probably not intended in the first place.

---

New commits:

28abf29	33167: avoid printing of output
bb047c7	33167: remove check for gmp/flint in lrslib's m4 file

[orlitzky]

comment:15

Replying to @mwageringel:

This is the first time I have edited an .m4 file, so I hope I did not break anything.

It looks like you deleted the DEPCHECK line and its matching ]), and then un-indented the stuff between? That should be right.

m4 and the ./configure script are all "stringly typed," so really the only way we have of testing is to run ./configure in every configuration and watch the output.

[mwageringel]

comment:16

Replying to @orlitzky:

It looks like you deleted the DEPCHECK line and its matching ]), and then un-indented the stuff between?

Yes, exactly.

m4 and the ./configure script are all "stringly typed," so really the only way we have of testing is to run ./configure in every configuration and watch the output.

For another data point, I have tested this by replacing the lrsnash binary by a non-functional one, which also leads to the intended result:

Checking whether SageMath should install SPKG lrslib...
checking for lrsnash... lrsnash
checking whether lrsnash can handle the new input format... no
configure: no suitable system package found for SPKG lrslib

[seblabbe]

comment:17

Since I saw the problem described in #33231, I wonder if the line

result = subprocess.run(command, capture_output=True, text=True)

should be put inside of a try to catch OSError type of failures?

[orlitzky]

comment:18

Replying to @seblabbe:

Since I saw the problem described in #33231, I wonder if the line

result = subprocess.run(command, capture_output=True, text=True)

should be put inside of a try to catch OSError type of failures?

Yeah, just about anything can go wrong. Another example:

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_functional()
...
PermissionError: [Errno 13] Permission denied: 'convert'

Or with a dangling symlink from graphicsmagick,

...
FileNotFoundError: [Errno 2] No such file or directory: 'convert'

These aren't normal scenarios, but if there's no way to disable the check, the test suite can't crash whenever e.g. someone installs a broken tool in /usr/local/bin.

[seblabbe]

Changed branch from u/gh-mwageringel/33167 to u/slabbe/33167

[seblabbe]

comment:19

I am now wondering if it should be a join feature instead.

---

New commits:

ffe23f9	33167: adding a second check in lrs feature is_functional
986ed53	33167: simplification of check #1 in is_functional method of lrs feature
ab57121	33167: avoid printing of output
0fe547b	33167: remove check for gmp/flint in lrslib's m4 file
f42ade0	33231: add try except clause to catch OSError during run

[seblabbe]

Changed commit from bb047c7 to f42ade0

[sagetrac-git]

Changed commit from f42ade0 to 1d38000

[sagetrac-git]

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

1d38000

33167: add try except clause to catch OSError during run

[seblabbe]

comment:21

I made an error in the commit message. I just force-pushed the last commit.

[slel]

comment:22

Set milestone to sage-9.6 after Sage 9.5 release.

[mkoeppe]

comment:23

Replying to @mwageringel:

Replying to @orlitzky:

Replying to @mwageringel:
This is probably a mistake in the spkg-configure.m4 for lrslib. If all we do is run the executables from lrslib, we don't care if they're linked to a mismatched version of flint/gmp.

Yes, this seems to be the case. I have removed the check for gmp/flint from the .m4 file.

I don't like this change. polymake does using lrslib as a library. This is why we need the DEPCHECK.

[orlitzky]

comment:24

Replying to @mkoeppe:

Yes, this seems to be the case. I have removed the check for gmp/flint from the .m4 file.

I don't like this change. polymake does using lrslib as a library. This is why we need the DEPCHECK.

Do we link against polymake?

[mkoeppe]

comment:25

Yes

[orlitzky]

comment:26

Ok, I guess put it back then. A comment would be nice too. We can't expect people to be aware of all second-order optional reverse dependencies when reading these files.

[mwageringel]

Changed branch from u/slabbe/33167 to u/gh-mwageringel/33167

[mwageringel]

comment:27

Replying to @orlitzky:

Ok, I guess put it back then. A comment would be nice too.

I have removed the respective commit from the branch and added a comment instead in the last commit.

---

New commits:

ed2a3d4	33167: add try except clause to catch OSError during run
16e1b24	33167: add comment about polymake's use of lrslib

[mwageringel]

Changed commit from 1d38000 to 16e1b24

[mkoeppe]

Reviewer: Matthias Koeppe

[mwageringel]

comment:30

Thanks.

[vbraun]

Changed branch from u/gh-mwageringel/33167 to 16e1b24

[mkoeppe]

comment:32

Follow-up: #33466

[mkoeppe]

Changed commit from 16e1b24 to none