Replace is_package_installed with Features
saraedum opened this issue · 194 comments
Indeed same kind of work. Much more sophisticated on your part I must say. Do I understand well that you need an executable class for any external binaries that you want to call?
It could also solve the issue that I was thinking about executables and other files requirement. You should be able to find and use something already installed in a standard location. theta being already present in /usr/local/bin instead of $SAGE_LOCAL/bin (which resolves to /usr/bin in sage-on-gentoo) or anywhere in the PATH. Similarly you can install gap packages in ~/.gap/pkg and there is no reasons why you shouldn't be able to use those.
New commits:
3233147 | Add Features to check the environment at runtime |
dfd97f8 | Check for grape with GapPackage instead of is_package_installed() |
7c1ca31 | Check for a working CSDP binary instead of is_package_installed() |
Branch pushed to git repo; I updated commit sha1. New commits:
97d47af | 20382: missing docstring |
Branch pushed to git repo; I updated commit sha1. New commits:
8b6fcc9 | 20382: desirable error message + typo |
Handling of database_gap will be delicate. The spkg includes bits removed stuff from gap that are expected by default (no need to load if present) and one "real" gap package that one would need to load (tomlib).
That may require special consideration.
Thanks for the pointer about database_gap. I should have a complete proposal up on the other ticket in the next 24 hours.
I think this ticket should replace the other one unless you have a plan. I am making a list of stuff that needs to come from the other ticket and stuff that should use your feature detection. I guess we could split it along those lines.
Going through the files that I have changed in #20377 which this ticket should supersede:
sage/databases/cremona.pywe need to check the real presence of a file in a specific location in this case. Adding something to feature may be overkill for that particular case.sage/game_theory/normal_form_game.py+sage/geometry/polyhedron/base.pylook for thelrsexecutablesage/graphs/generators/classical_geometries.pyalready covered in this ticketsage/graphs/generic_graph.pyI think my stuff should stay. It is mostly straighttry...expectwhether or not a python module is available.sage/graphs/graph_generators.py3 different executables to detectsage/graphs/lovasz_theta.pycovered heresage/groups/generic.pypossibly detect if gap package tomlib is presentsage/groups/perm_gps/permgroup.pyagain dodgy database_gap detection in one part. The part using the gaphappackage would need a nice rework not just migration. I'd like to pick the mind of the person who wrote this.sage/misc/all.pymy stuff should stay. It is removal ofimportno one uses.sage/rings/polynomial/multi_polynomial_sequence.pyfinding one executable
Reviewer: Nicolas M. Thiéry, ...
Author: Julian Rüth
Hi Julian,
Just some rambling, thinking about use cases for this ticket or some follow up:
While discussing with the Debian people, it was stressed out that we
will eventually want to be able to support simultaneously a stable
version of software X (as provided by distributions), and a devel
version of it (e.g. for our researchers that need the very latest
features). Therefore we probably will want to have code that could
look like:
def latest_feature():
"""
...
EXAMPLES::
sage: X = ...
sage: X.latest_feature() # optional gap >= 3.5
"""
GAP(min_version=3.5).require() # or some better syntax
...
return gap.latest_feature()
To support this use case we would need:
-
support for version checking in the Feature code (probably in a follow up ticket).
Probably not much can be written generically, except possibly for Executables,
trying to pass the --version and scanning the output for something that looks
like a version number -
some updates to the doctesting framework to support version
annotations (definitely in another ticket) -
mapping feature names appearing in the #optional annotations to actual Feature's
(it's going in this direction in #20182).
Not directly related to this ticket, but building on the discussion we had while walking in Cernay, we may want to be able to write the annotation as::
def latest_feature():
"""
...
.. REQUIRES:: gap >= 3.5
EXAMPLES::
sage: X = ...
sage: X.latest_feature()
"""
Cheers,
Nicolas
Branch pushed to git repo; I updated commit sha1. New commits:
340d36e | Improve reporting of Features |
0fc00d9 | grape is in the gap_packages SPKG |
aa12d10 | Fixed typos in Features |
76aedb4 | check for GAP package "hap" through a Feature test |
2e451b4 | 20382: Proofreading |
97d47af | 20382: missing docstring |
8b6fcc9 | 20382: desirable error message + typo |
1bba301 | Merge branch 'u/nthiery/replace_is_package_installed_with_features' of git://trac.sagemath.org/sage into t/20382/replace_is_package_installed_with_features |
nthiery: About version checks. I think it would be easy to add this in the current setup. However, I would recommend to follow the philosophy of autoconf here. I.e., we should not check for a version because you need to keep track which version range supportes the feature you are looking for (and often things are heavily patched so the version might be misleading.) Instead, we should check for the actual feature/interface required. It is a bit more work of course but usually better and much easier to maintain.
Yep there is value in the autoconf approach. On the other hand, since this is mostly about doctests at this point, a version information is more useful for the user: "oh, I need to upgrade to 3.5" is simpler than "oh, I need to upgrade to a recent enough version that supports method xxx on object yyy". We could do both: make a feature check, and provide a version number for user reporting; but that's more work. Another advantage of version checking is that you just need to fetch the version once, and don't need to relaunch, e.g a GAP command, each time you want to check another feature.
I like more the centralized approach in this ticket rather than the one in #20377 in which each feature needs a special care. Though many things looks very complicated and the related #20182 is much more readable (though using inheritance from this ticket would save some efforts). Moreover #20182 implements some database of available features which is not discussed here. The latter has the advantage of minimizing the requests time after the first call. With the architecture proposed here, each check involves a Feature class creation and some associated test...
Some more specific remarks:
-
Do we really care about the class
FeatureTestResult? If the result isTruethen we mostly don't care about explanation and if it isFalsewe likely want an error (with an explicit error message). Similarly, I would get rid ofFeatureNotPresentError. Instead, I would add anerrorand amessagearguments in therequiremethod (with of course some reasonable default which would mimic the current behavior using the standardRuntimeError). -
Are you sure
CSDPis better inside the filelovasz_theta.py? Do we want to spread theFeaturesall around the modules or should they be centralized? -
Finally class names might be confusing.
GapPackageis not a handle on a gap package nor something describing what is inside a given package (what I would guess from the name). Similarly, with havingCSDPhas a class whenCSDPis the name of the program. It would be fine if all of them belong to a fixed module.
And last but not the least: what about having the "packages" part directly attached to executable?
sage.interfaces.gap.gap.require_package('grape')
instead of
sage.misc.features.GapPackage('grape').require()
Replying to @videlec:
And last but not the least: what about having the "packages" part directly attached to executable?
Sure. A followup ticket could define gap.require_package as a call out
to GapPackage().require.
Replying to @videlec:
Moreover #20182 implements some database of available features which
is not discussed here. The latter has the advantage of minimizing the
requests time after the first call. With the architecture proposed here,
each check involves aFeatureclass creation and some associated test...
The docstring tells you that you might want to cache the result if you think it
is a performance problem. Most of the time you are going to call out to a
binary anyway so I do not see the point of caching it as a default. "Premature
optimization is the root of all evil…"
We could centralize the checks. I do not really have an opinion on that.
- Do we really care about the class
FeatureTestResult? If the result is
Truethen we mostly don't care about explanation and if it isFalsewe
likely want an error (with an explicit error message).
That's what is_present vs require is about. There are places where the default algorithm is chosen depending on the
presence of some feature.
Similarly, I would
get rid ofFeatureNotPresentError. Instead, I would add anerrorand a
messagearguments in therequiremethod (with of course some
reasonable default which would mimic the current behavior using the
standardRuntimeError).
So you have to catch whatever RuntimeError instead of being able to catch
something specific? I do not see the advantage of that. Also, if I understand
your proposal, each caller has to cook up their own error message. I think it
is nice to have a central place where these error messages and resolutions
live.
Finally, with your approach you do not get the granularity between abc is not on the path vs abc is there but does not work. That's something that some
people requested.
- Are you sure
CSDPis better inside the filelovasz_theta.py? Do we
want to spread theFeaturesall around the modules or should they be
centralized?
See above.
- Finally class names might be confusing.
GapPackageis not a handle on
a gap package nor something describing what is inside a given package
(what I would guess from the name).
GapPackage lives in sage.misc.feature. So it should be clear that this is not
the actual package. We can suffix everything with Feature if that is preferred.
I prefer short class names if possible.
Similarly, with having
CSDPhas a
class whenCSDPis the name of the program. It would be fine if all of
them belong to a fixed module.
See above. (Sorry if I misunderstood you. It seems you are raising the
centralization vs localization issue several times in your comment.)
Replying to @nthiery:
Yep there is value in the autoconf approach. On the other hand, since this is mostly about doctests at this point, a version information is more useful for the user: "oh, I need to upgrade to 3.5" is simpler than "oh, I need to upgrade to a recent enough version that supports method xxx on object yyy". We could do both: make a feature check, and provide a version number for user reporting; but that's more work. Another advantage of version checking is that you just need to fetch the version once, and don't need to relaunch, e.g a GAP command, each time you want to check another feature.
Sure. Both options are valid. They are not in the scope of this ticket I
think. In either case, such "features" can easily be added to Feature.
Work Issues: reorder classes in sage.misc.feature
If I do ls in sage/misc directory and I see the file feature.py listed, I would never be able to guess its content. Can the filename be more verbose maybe package_feature.py?
Replying to @seblabbe:
If I do
lsinsage/miscdirectory and I see the filefeature.pylisted, I would never be able to guess its content. Can the filename be more verbose maybepackage_feature.py?
Would not make sense. It is not only about packages but also system softwares and libraries on the system. However, it could contains the words dependencies or requirement. I am not sure what would be the clearest.
Maybe something like feature_test? This seems to be consistent with autoconf's vocabulary.
I like feature_test. I will rename it to that.
Hello,
This code is mostly Sage agnostic right? Would it be possible to move this code out of Sage source code? It would better be in sage/src/sage_setup/ or whatever appropriate (even possibly an external python module). We might want to use it outside of Sage or before Sage is built to track possible dependencies.
As far as I am concerned sage_setup is only used at install time and shouldn't be used at runtime. But spinning it as a separate python package sounds like an interesting idea. Other packages could probably benefits from those mechanism.
I already thought about moving parts of this out of the source tree. I have not really reached a conclusion yet. It certainly needs to be used during build time. Currently, we have sage_setup/optional_extension.py which calls sage.misc.package.is_package_installed. So, at least in a first version, I would certainly mimic this behaviour and check for a feature (in this case, whether Cython can build against a library) at build time.
Replying to @videlec:
This code is mostly Sage agnostic right?
The idea is Sage agnostic. The features it checks for are of course relatively Sage specific.
Would it be possible to move this code out of Sage source code? It would better be in
sage/src/sage_setup/or whatever appropriate (even possibly an external python module). We might want to use it outside of Sage or before Sage is built to track possible dependencies.
I am not sure about moving it to sage_setup. We need this at runtime. Can we (and should we?) access things in sage_setup at runtime?
It is an interesting idea to make it a separate package. There seems to be no python package around which does anything similar. I would be happy to pursue such a path but would like to postpone this to a followup ticket.
Branch pushed to git repo; I updated commit sha1. New commits:
ef9cf7d | Merge branch 'develop' into t/20382/replace_is_package_installed_with_features |
Branch pushed to git repo; I updated commit sha1. New commits:
095b402 | fixed incorrect import statement |
670a3e2 | Merge branch 'u/saraedum/replace_is_package_installed_with_features' of git://trac.sagemath.org/sage into t/20382/replace_is_package_installed_with_features |
6f5dc21 | Completed implementation of CremonaDatabase feature |
074a318 | Implement SmallGroupsLibrary feature |
Branch pushed to git repo; I updated commit sha1. New commits:
6d508a1 | Merge branch 'develop' of https://github.com/sagemath/sage into t/20382/replace_is_package_installed_with_features |
Branch pushed to git repo; I updated commit sha1. New commits:
10c1a47 | Asking whether a package is installed is not deprecated |
680c19a | removed unused import |
bb47ef0 | Replace is_installed with SmallGroupsLibrary.require |
3acffa7 | A note on is_package_installed versus features |
b6c4797 | Optional sage modules and shared libraries as features |
This seems to be essentially complete now. Some docstrings are still missing and I also need to do some testing but I replaced all occurrences of is_package_installed if the code is just checking whether an optional package is installed.
Any feedback is already very welcome until I find the time to polish all the docstrings.
That's really cool. I will give it a good look. I may even try it in sage-on-gentoo.
I was actually a little confused that is_package_installed gets used in so few places. (Or maybe search_src is lying about it.)
Looks good to me. There is one last thing that I think should be done, but I wouldn't hold the ticket for it, is remove
from .package import (install_package,
installed_packages, is_package_installed,
standard_packages, optional_packages, experimental_packages,
upgrade, package_versions)
from sage/misc/all.py we don't really want those to be directly exposed in my opinion - and they are/were always called explicitly in sage itself.
My wish above may be best served by different ticket. Can you put this one in "need review".
Checking availability of a Python module as you did has consequences
sage: def is_present(module):
....: import importlib
....: try:
....: importlib.import_module(module)
....: return True
....: except ImportError:
....: return False
sage: print len(sys.modules), 'numpy' in sys.modules
2625 False
sage: is_present('numpy')
True
sage: print len(sys.modules), 'numpy' in sys.modules
2710 True
This would better be documented.
I would be very happy to use this ticket to solve #21135 where I need to set the octave parameters for its Sage interface depending on its version... hence I am asking whether it would be possible to have a version method on Features (or at least on Executable)? For executable it is reasonable to just have an attribute _version_cmd_option (that would generally be -v or --version or --dumpversion) with a default implementation that runs the command with the option _version_cmd_option. What do you think?
Note also that for interfaces we allow remote programs with possibly subtle commands (see #20388 and the file sage/interfaces/expect.py). For example, I can access magma and get it work within sage by doing
$ SAGE_MAGMA_COMMAND="module load magma/2.11.13; magma"
$ SAGE_MAGMA_SERVER="plafrim"
$ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER
$ sage -q
sage: magma('1+1')
2
Does a magma feature should take care of this?
Replying to @videlec:
I would be very happy to use this ticket to solve #21135 where I need to set the octave parameters for its Sage interface depending on its version...
It would be great to see somebody use this interface already :)
I have completing this Ticket high on my priority list. I hope I find the time to do this soon.
hence I am asking whether it would be possible to have a
versionmethod onFeatures(or at least onExecutable)? For executable it is reasonable to just have an attribute_version_cmd_option(that would generally be-vor--versionor--dumpversion) with a default implementation that runs the command with the option_version_cmd_option. What do you think?
Sometimes version checking is a good thing to do but I generally follow the autoconf tradition here that sees version numbers as something very unreliable. I think you should check whether the executable provides the interface that you need.
So, I would be reluctant to provide a general interface for version checks. I should certainly add a comment about this somewhere. Anyway, you can extend any Feature. To check for a version you might want to roughly do something like what I did for StaticFile.absolute_path.
Replying to @videlec:
Note also that for interfaces we allow remote programs with possibly subtle commands (see #20388 and the file
sage/interfaces/expect.py). For example, I can access magma and get it work within sage by doing$ SAGE_MAGMA_COMMAND="module load magma/2.11.13; magma" $ SAGE_MAGMA_SERVER="plafrim" $ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER $ sage -q sage: magma('1+1') 2Does a magma feature should take care of this?
Sorry, but I am not sure whether I understand your question. Do you mean whether a Magma Feature should look at SAGE_MAGMA_COMMAND and SAGE_MAGMA_SERVER? Yes, absolutely.
Replying to @saraedum:
Replying to @videlec:
Note also that for interfaces we allow remote programs with possibly subtle commands (see #20388 and the file
sage/interfaces/expect.py). For example, I can access magma and get it work within sage by doing$ SAGE_MAGMA_COMMAND="module load magma/2.11.13; magma" $ SAGE_MAGMA_SERVER="plafrim" $ export SAGE_MAGMA_COMMAND SAGE_MAGMA_SERVER $ sage -q sage: magma('1+1') 2Does a magma feature should take care of this?
Sorry, but I am not sure whether I understand your question. Do you mean whether a
MagmaFeature should look atSAGE_MAGMA_COMMANDandSAGE_MAGMA_SERVER? Yes, absolutely.
Yes. That was mostly the question ;-) There are some interferences between the branch provided here and how software are detected (and configured) in sage/interfaces/. This should be done in one place and I guess features is the right one.
Replying to @saraedum:
Replying to @videlec:
[...]
Sometimes version checking is a good thing to do but I generally follow the autoconf tradition here that sees version numbers as something very unreliable. I think you should check whether the executable provides the interface that you need.
Argh. I do not see how I can check that octave does not launch the GUI... I can check whether octave --no-gui is available. If octave does not exist I got a OSError and if it does not accept --no-gui I get a 1 return code. Is that what I should do? Does that should be part of an Octave feature?
Branch pushed to git repo; I updated commit sha1. New commits:
2a67f26 | Trivial doctest fixes in sage.misc.feature |
Branch pushed to git repo; I updated commit sha1. New commits:
948bb45 | Fill in missing doctests for sage.misc.feature |
Branch pushed to git repo; I updated commit sha1. New commits:
e824e0d | Fixed doctests in sage.misc.feature |
Branch pushed to git repo; I updated commit sha1. New commits:
7cf972c | Merge branch 'develop' of https://github.com/sagemath/sage into t/20382/replace_is_package_installed_with_features |
Replying to @kiwifb:
[...] Do I understand well that you need an executable class for any external binaries that you want to call?
Its up to you. You can create a new class and make it easier for others to use or you could just write Executable(...).require() to check for an executable in one place.
Replying to @kiwifb:
Handling of
database_gapwill be delicate. The spkg includes bits removed stuff fromgapthat are expected by default (no need to load if present) and one "real" gap package that one would need to load (tomlib).
I tried to check for the package that was actually needed whenever the old code queried the gap_packages SPKG. I am not sure I fully understand your comment here. Could you point me to an example in the code where this is a problem?
Replying to @kiwifb:
Going through the files that I have changed in #20377 which this ticket should supersede:
sage/databases/cremona.pywe need to check the real presence of a file in a specific location in this case. Adding something to feature may be overkill for that particular case.
sage/databases/cremona.py loads the cremona DB from a very specific place, namely SAGE_SHARE. This is certainly not ideal. But making that code more flexible should imho go into a followup ticket. Should I create one?
sage/game_theory/normal_form_game.py+sage/geometry/polyhedron/base.pylook for thelrsexecutable
Addressed with the Lrs feature.
sage/graphs/generators/classical_geometries.pyalready covered in this ticketsage/graphs/generic_graph.pyI think my stuff should stay. It is mostly straighttry...expectwhether or not a python module is available.
I am checking this now with a PythonModule feature.
sage/graphs/graph_generators.py3 different executables to detect
I am checking for these executables (and run them on trivial inputs.)
sage/graphs/lovasz_theta.pycovered heresage/groups/generic.pypossibly detect if gap package tomlib is present
I am checking for the presence of the small groups library. Is that the same somehow?
sage/groups/perm_gps/permgroup.pyagain dodgy database_gap detection in one part. The part using the gaphappackage would need a nice rework not just migration. I'd like to pick the mind of the person who wrote this.
I am checking for the hap package here. Is there anything else that should happen?
sage/misc/all.pymy stuff should stay. It is removal ofimportno one uses.
Sure. But I am not addressing this here.
sage/rings/polynomial/multi_polynomial_sequence.pyfinding one executable
Actually you need to check for the presence of the fes library. Not only that but also whether the sage interface to that library has been built (which only happens if sage can find the library, i.e., SPKG at build time.)
Replying to @nthiery:
Yep there is value in the autoconf approach. On the other hand, since this is mostly about doctests at this point, a version information is more useful for the user: "oh, I need to upgrade to 3.5" is simpler than "oh, I need to upgrade to a recent enough version that supports method xxx on object yyy". We could do both: make a feature check, and provide a version number for user reporting; but that's more work.
That's the approach I would like to follow. Currently there is no code like this in here because I am just replacing is_package_installed which also did not do any version checks. But, yes, people should check for a certain feature of a package and then the reason of a test failure should say "Your X does not support Y." and the resolution should say "You need at least version Z of X. Try sage -i X to fix this." or something along these lines.
Branch pushed to git repo; I updated commit sha1. New commits:
7d38138 | Renamed sage.misc.feature to sage.misc.feature_test |
Replying to @videlec:
Checking availability of a Python module as you did has consequences
sage: def is_present(module): ....: import importlib ....: try: ....: importlib.import_module(module) ....: return True ....: except ImportError: ....: return False sage: print len(sys.modules), 'numpy' in sys.modules 2625 False sage: is_present('numpy') True sage: print len(sys.modules), 'numpy' in sys.modules 2710 TrueThis would better be documented.
You are right. I documented that.
Replying to @videlec:
Replying to @saraedum:
Replying to @videlec:
[...]
Sometimes version checking is a good thing to do but I generally follow the autoconf tradition here that sees version numbers as something very unreliable. I think you should check whether the executable provides the interface that you need.
Argh. I do not see how I can check that
octavedoes not launch the GUI... I can check whetheroctave --no-guiis available. If octave does not exist I got aOSErrorand if it does not accept--no-guiI get a1return code. Is that what I should do? Does that should be part of anOctavefeature?
I have a feeling that I already answered this one. But apparently I have not.
I would write an Octave feature that checks whether there is an octave binary and then checks whether it accepts --no-gui. Sort of like StaticFile has an absolute_path() that tells you something about how this feature works, Octave could expose information about which parameters are needed to actually invoke it so it reads from stdin.
I hope I addressed all comments on this ticket. I did not have a chance to run all doctests yet (my sage 7.4 is still building) but essentially this is ready for review now.
Changed work issues from reorder classes in sage.misc.feature to none
New commits:
0529637 | Reordered classes in sage.misc.feature_test |
6f2d596 | Fixed doctests which failed because of the feature -> feature_test renaming |
edd4f6e | add missing sig_off to doctest |
New commits:
0529637 | Reordered classes in sage.misc.feature_test |
6f2d596 | Fixed doctests which failed because of the feature -> feature_test renaming |
edd4f6e | add missing sig_off to doctest |
Branch pushed to git repo; I updated commit sha1. New commits:
2111eb3 | Move Bliss feature test to feature_test.py |
Since is_package_installed is very sage (the distribution) specific, it causes trouble for package managers which would like to refer to check whether a package is available on the local system instead of whether it has been installed with sage -i (or similar.)
This replaces all occurrences of is_package_installed (which are not part of sage the distribution) with a "feature" check which is performed much in the spirit of autoconf: check for the presence of the features needed, not for version numbers.
These checks are certainly not perfect yet and will need to evolve over time.
Needs to be merged with Sage 7.4.beta2.
For OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.
And once you do this, you can use the library check in src/sage_setup/optional_extension.py to decide which optional extensions to build.
The changes to src/sage/misc/cython.py might make sense but do not belong to this ticket.
I must say to me in sage-on-gentoo the fact that the current OptionalModule only require presence to build the extension is problematic but I don't expect a solution to that particular aspect in the near future.
In effect the fact that you only check for presence/availability as a signal for building is introducing automagic dependencies.
I currently have a variable mechanism to signal that the dependency should be present and I want to build. A setup.cfg could be a viable solution. I realise that it would be a burden in the current sage model in the short term.
Replying to @kiwifb:
I must say to me in sage-on-gentoo the fact that the current
OptionalModuleonly require presence to build the extension is problematic but I don't expect a solution to that particular aspect in the near future.
Ideally, Sage should have something like Gentoo's USE flags for optional dependencies...
Branch pushed to git repo; I updated commit sha1. New commits:
a427d75 | Merge remote-tracking branch 'origin/develop' into t/20382/replace_is_package_installed_with_features |
Replying to @jdemeyer:
For
OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.
Ok. I am splitting this up.
And once you do this, you can use the library check in
src/sage_setup/optional_extension.pyto decide which optional extensions to build.
That would be a followup ticket I believe.
Replying to @jdemeyer:
The changes to
src/sage/misc/cython.pymight make sense but do not belong to this ticket.
I'd prefer to sneak this change in here. The shared library test code in here actually tests that this does the right thing and I'd prefer not to write some constructed doctest for something like this.
Branch pushed to git repo; I updated commit sha1. New commits:
2086217 | Isolate library checks from the OptionalModule implementation code |
Sorry, but it will need rebasing against #21289 which is already merged for the next beta.
Replying to @saraedum:
Replying to @jdemeyer:
For
OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.Ok. I am splitting this up.
Well, I meant really splitting those it up: get rid of OptionalModule altogether and replace it by Module. And remove the dependencies: if the dependencies are not satisfied, the module will not import. There is no need for an additional check.
And once you do this, you can use the library check in
src/sage_setup/optional_extension.pyto decide which optional extensions to build.That would be a followup ticket I believe.
Fair enough.
Replying to @jdemeyer:
Replying to @saraedum:
Replying to @jdemeyer:
For
OptionalModule, you should separate the library check from the Python module check. They really are independent things. It just happens that there is essentially a one-to-one correspondence in Sage, but that doesn't have to be the case.Ok. I am splitting this up.
Well, I meant really splitting those it up: get rid of
OptionalModulealtogether and replace it byModule. And remove the dependencies: if the dependencies are not satisfied, the module will not import. There is no need for an additional check.
Sure, the check is not necessary. But I would like to tell people what is the problem here. In that particular case, the module also would not import but that's more obscure than telling people that the required library is missing. More generally, an OptionalModule checks whether the module imports and is functional. Running some tests to make sure that the underlying library is working properly is a reasonable thing to do imho.