Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 31 additions & 8 deletions conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,37 @@ def _find(
pytest.skip("unable to import module %r" % self.path)
else:
if isinstance(exception, ModuleNotFoundError):
# Ignore some missing features/modules for now
# TODO: Remove this once all optional things are using Features
if exception.name in (
"valgrind",
"rpy2",
"sage.libs.coxeter3.coxeter",
"sagemath_giac",
):
# Ignore some missing features/modules for
# now. Many of these are using custom
# "sage.doctest" headers that only our doctest
# runner (i.e. not pytest) can understand.
#
# TODO: we should remove this once all
# optional things are using Features. It
# wouldn't be too hard to move the
# "sage.doctest" header into pytest (as
# explicit ignore lists based on feature
# tests), but that would require duplication
# for as long as `sage -t` is still used.
from sage.features.coxeter3 import Coxeter3
from sage.features.sagemath import (sage__libs__eclib,
sage__libs__giac)
from sage.features.standard import PythonModule

exc_list = ["valgrind"]
if not sage__libs__eclib().is_present():
exc_list.append("sage.libs.eclib")
if not PythonModule("rpy2").is_present():
exc_list.append("rpy2")
if not Coxeter3().is_present():
exc_list.append("sage.libs.coxeter3.coxeter")
if not sage__libs__giac().is_present():
exc_list.append("sagemath_giac")

# Ignore import errors, but only when the associated
# feature is actually disabled. Use startswith() so
# that sage.libs.foo matches all of sage.libs.foo.*
if any(exception.name.startswith(e) for e in exc_list):
pytest.skip(
f"unable to import module {self.path} due to missing feature {exception.name}"
)
Expand Down
1 change: 1 addition & 0 deletions src/doc/en/reference/spkg/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ Features
sage/features/mcqd
sage/features/meataxe
sage/features/mip_backends
sage/features/mwrank
sage/features/normaliz
sage/features/pandoc
sage/features/pdf2svg
Expand Down
33 changes: 33 additions & 0 deletions src/sage/features/mwrank.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
r"""
Feature for testing the presence of the ``mwrank`` program (part
of eclib)
"""

from . import Executable, FeatureTestResult


class Mwrank(Executable):
r"""
A :class:`~sage.features.Feature` describing the presence of
the ``mwrank`` program.
EXAMPLES::
sage: from sage.features.mwrank import Mwrank
sage: Mwrank().is_present() # needs mwrank
FeatureTestResult('mwrank', True)
"""
def __init__(self):
r"""
TESTS::
sage: from sage.features.mwrank import Mwrank
sage: isinstance(Mwrank(), Mwrank)
True
"""
Executable.__init__(self, 'mwrank', executable='mwrank',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to move this runtime check to a compile time meson check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. Yes, but we should either (a) have a plan for how to leave it detectable at runtime, since this is easy to install after sage is installed, or (b) decide as a group that we don't want to detect it at runtime, so that I don't get yelled at for breaking something without community input.

spkg='eclib', type='standard')


def all_features():
return [Mwrank()]
27 changes: 27 additions & 0 deletions src/sage/features/sagemath.py
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,33 @@ def __init__(self):
spkg='sagemath_ntl', type='standard')


class sage__libs__eclib(JoinFeature):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think conditionalizing on imports in sagelib is a good idea: when the import fails, all tests that would otherwise have notified us about the import problem are disabled. (This actually came up recently in the context of bliss, where it was not clear if the tests just 'succeed' because they were disabled.)

So I would prefer if you could directly conditionalize on eclib, using the detection with meson. So either write a eclib_is_present toggle in sage.config or via a new sage/features/eclib.py.in (which then has a constant 'presence' state).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the way to go, but I also think we should all think this through (in a GH discussion?) first. It would make more sense, especially for the features in meson.options, to do them all at once. There are at least three classes of feature that we need to think about:

  1. Executables (mwrank, but also e.g. ffmpeg)
  2. Internal optional cython modules (sage.libs.eclib)
  3. External cython modules (sagemath_giac)

For executables, I definitely want the ability to pass -Dffmpeg=disabled, and have the corresponding feature disabled. I don't want it detected at runtime and then failing because (say) animated gif support isn't there, and nobody copied that check to sage/features/ffmpeg.py. (I also simply don't want to waste time testing something I'm never going to use.) That -Dffmpeg=enabled should do the opposite is fairly straightforward. Where it gets tricky is that not everyone wants to recompile sage just to detect /usr/bin/ffmpeg. On binary distros that isn't an option, and people will probably complain if we take away the ability. We do have -Dffmpeg=auto, but that means something different, and we probably don't want to redefine standard meson terms. We could have a global option to override any "disabled" features at run-time. In any case, doing the detection at runtime (even optionally) does mean that we need to duplicate every configure-time check into the corresponding sage feature, so there are trade-offs to supporting this.

For internal cython modules, there's really no other way to obtain the module than to rebuild sage, so being able to detect them on-the-fly doesn't really help. Although I guess it's possible that some distro maintainer is building all of sage and then putting sage/libs/eclib into a separate tarball. It probably depends on how independent the module is. eclib is more tightly integrated than bliss, coxeter, and sirocco, for example.

External cython modules are the easiest case IMO. Right now sagemath_giac is the main example, but there's no reason (other than lack of time) we couldn't do the same thing with e.g. sagemath_bliss. What's nice about these is that it solves the runtime detection problem in a very simple way. If you want support for giac in sage, you install sagemath_giac. If you don't, you don't. We can detect it at runtime without side effects because, in this very special case, there's no other reason for you to have sagemath_giac installed. This is in contrast with (say) ffmpeg that I have installed for other reasons. tl;dr in this case runtime detection is fine. Being able to force-disable or force-enable the feature would be a bonus, but these aren't our main problem.

r"""
A :class:`sage.features.Feature` describing the presence of
:mod:`sage.libs.eclib`.

In addition to the modularization purposes that this tag serves,
it also provides attribution to the upstream project.

TESTS::

sage: from sage.features.sagemath import sage__libs__eclib
sage: sage__libs__eclib().is_present() # needs sage.libs.eclib
FeatureTestResult('sage.libs.eclib', True)
"""
def __init__(self):
r"""
TESTS::

sage: from sage.features.sagemath import sage__libs__eclib
sage: isinstance(sage__libs__eclib(), sage__libs__eclib)
True
"""
JoinFeature.__init__(self, 'sage.libs.eclib',
[PythonModule('sage.libs.eclib.mwrank')],
spkg='eclib', type='standard')


class sage__libs__giac(JoinFeature):
r"""
A :class:`sage.features.Feature` describing the presence of :mod:`sage.libs.giac`.
Expand Down
Loading