#20382 closed enhancement (fixed)
Replace is_package_installed with Features
Reported by: | saraedum | Owned by: | jdemeyer |
---|---|---|---|
Priority: | major | Milestone: | sage-8.3 |
Component: | build | Keywords: | days85 |
Cc: | mkoeppe, jdemeyer, leif, embray, defeo, isuruf, slelievre | Merged in: | |
Authors: | Julian Rüth, Jeroen Demeyer | Reviewers: | Nicolas M. Thiéry, François Bissey, Julian Rüth |
Report Upstream: | N/A | Work issues: | |
Branch: | ad6dcc6 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
Change History (159)
comment:1 Changed 6 years ago by
- Branch set to u/saraedum/replace_is_package_installed_with_features
comment:2 follow-up: ↓ 51 Changed 6 years ago by
- Commit set to 7c1ca312c14f1b777563607d919a245e1d48e557
comment:3 Changed 6 years ago by
- Branch changed from u/saraedum/replace_is_package_installed_with_features to u/nthiery/replace_is_package_installed_with_features
comment:4 Changed 6 years ago by
- Commit changed from 7c1ca312c14f1b777563607d919a245e1d48e557 to 97d47afe5405f899e63e655f2aedf409ad35e863
Branch pushed to git repo; I updated commit sha1. New commits:
97d47af | 20382: missing docstring
|
comment:5 Changed 6 years ago by
- Commit changed from 97d47afe5405f899e63e655f2aedf409ad35e863 to 8b6fcc990f756d34e16c0439815ffbc4155e47af
Branch pushed to git repo; I updated commit sha1. New commits:
8b6fcc9 | 20382: desirable error message + typo
|
comment:6 follow-up: ↓ 52 Changed 6 years ago by
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.
comment:7 Changed 6 years ago by
Thanks for the pointer about database_gap. I should have a complete proposal up on the other ticket in the next 24 hours.
comment:8 Changed 6 years ago by
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.
comment:9 follow-up: ↓ 53 Changed 6 years ago by
Going through the files that I have changed in #20377 which this ticket should supersede:
sage/databases/cremona.py
we 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.py
look for thelrs
executablesage/graphs/generators/classical_geometries.py
already covered in this ticketsage/graphs/generic_graph.py
I think my stuff should stay. It is mostly straighttry...expect
whether or not a python module is available.sage/graphs/graph_generators.py
3 different executables to detectsage/graphs/lovasz_theta.py
covered heresage/groups/generic.py
possibly detect if gap package tomlib is presentsage/groups/perm_gps/permgroup.py
again dodgy database_gap detection in one part. The part using the gaphap
package would need a nice rework not just migration. I'd like to pick the mind of the person who wrote this.sage/misc/all.py
my stuff should stay. It is removal ofimport
no one uses.sage/rings/polynomial/multi_polynomial_sequence.py
finding one executable
comment:10 Changed 6 years ago by
- Branch changed from u/nthiery/replace_is_package_installed_with_features to u/saraedum/replace_is_package_installed_with_features
comment:11 Changed 6 years ago by
- Commit changed from 8b6fcc990f756d34e16c0439815ffbc4155e47af to 1e275a8db635e5c9a0f63f9e37acc245616e94dd
comment:12 follow-up: ↓ 14 Changed 6 years ago by
- Reviewers set to Nicolas M. Thiéry, ...
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
comment:13 Changed 6 years ago by
- Commit changed from 1e275a8db635e5c9a0f63f9e37acc245616e94dd to 1bba301b7255d4725a0cb0e758ada93025b3e53b
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
|
comment:14 in reply to: ↑ 12 Changed 6 years ago by
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.
comment:15 follow-ups: ↓ 21 ↓ 54 Changed 6 years ago by
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.
comment:16 follow-ups: ↓ 20 ↓ 55 Changed 6 years ago by
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 isTrue
then we mostly don't care about explanation and if it isFalse
we likely want an error (with an explicit error message). Similarly, I would get rid ofFeatureNotPresentError
. Instead, I would add anerror
and amessage
arguments in therequire
method (with of course some reasonable default which would mimic the current behavior using the standardRuntimeError
).
- Are you sure
CSDP
is better inside the filelovasz_theta.py
? Do we want to spread theFeatures
all around the modules or should they be centralized?
- Finally class names might be confusing.
GapPackage
is 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 havingCSDP
has a class whenCSDP
is the name of the program. It would be fine if all of them belong to a fixed module.
comment:17 follow-ups: ↓ 19 ↓ 56 Changed 6 years ago by
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()
comment:18 Changed 6 years ago by
- Commit changed from 1bba301b7255d4725a0cb0e758ada93025b3e53b to 2a594ee16f72f7bebd06f82d0fdadfe8a526f178
Branch pushed to git repo; I updated commit sha1. New commits:
7b6df60 | Buckygen feature
|
0529a07 | ignore stderr output of theta
|
baf2eed | Benzene feature test
|
2b2bbf9 | Added Feature for plantri
|
30caced | Added features for bliss and igraph
|
3a2f8d2 | is_package_installed() is deprecated
|
2a594ee | Feature for lrslib
|
comment:19 in reply to: ↑ 17 Changed 6 years ago by
Replying to vdelecroix:
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
.
comment:20 in reply to: ↑ 16 Changed 6 years ago by
Replying to vdelecroix:
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...
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 isTrue
then we mostly don't care about explanation and if it isFalse
we 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 of
FeatureNotPresentError
. Instead, I would add anerror
and amessage
arguments in therequire
method (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
CSDP
is better inside the filelovasz_theta.py
? Do we want to spread theFeatures
all around the modules or should they be centralized?
See above.
- Finally class names might be confusing.
GapPackage
is 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
CSDP
has a class whenCSDP
is 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.)
comment:21 in reply to: ↑ 15 Changed 6 years ago by
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.
comment:22 Changed 6 years ago by
- Commit changed from 2a594ee16f72f7bebd06f82d0fdadfe8a526f178 to 2007e1822e0684078323677211c3a893d5af414e
comment:23 Changed 6 years ago by
- Work issues set to reorder classes in sage.misc.feature
comment:24 follow-up: ↓ 25 Changed 6 years ago by
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
?
comment:25 in reply to: ↑ 24 Changed 6 years ago by
Replying to slabbe:
If I do
ls
insage/misc
directory and I see the filefeature.py
listed, 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.
comment:26 Changed 6 years ago by
Maybe something like feature_test
? This seems to be consistent with autoconf's vocabulary.
comment:27 Changed 6 years ago by
I like feature_test
. I will rename it to that.
comment:28 follow-up: ↓ 30 Changed 6 years ago by
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.
comment:29 Changed 6 years ago by
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.
comment:30 in reply to: ↑ 28 Changed 6 years ago by
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 vdelecroix:
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.
comment:31 Changed 6 years ago by
- Commit changed from 2007e1822e0684078323677211c3a893d5af414e to ef9cf7d1eacb9195ab41611c2f05e634a1bf3d03
Branch pushed to git repo; I updated commit sha1. New commits:
ef9cf7d | Merge branch 'develop' into t/20382/replace_is_package_installed_with_features
|
comment:32 Changed 6 years ago by
- Commit changed from ef9cf7d1eacb9195ab41611c2f05e634a1bf3d03 to 074a31801df66eb96116e3f59ab6b2d0e42bd951
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
|
comment:33 Changed 6 years ago by
- Commit changed from 074a31801df66eb96116e3f59ab6b2d0e42bd951 to 6d508a11632ef2f8931c3f8b2ad709e6dffe5446
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
|
comment:34 Changed 6 years ago by
- Commit changed from 6d508a11632ef2f8931c3f8b2ad709e6dffe5446 to b6c4797ace44ecb6b85ec1d17e53258970866e7d
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
|
comment:35 Changed 6 years ago by
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.
comment:36 Changed 6 years ago by
That's really cool. I will give it a good look. I may even try it in sage-on-gentoo.
comment:37 Changed 6 years ago by
I was actually a little confused that is_package_installed
gets used in so few places. (Or maybe search_src
is lying about it.)
comment:38 Changed 6 years ago by
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.
comment:39 Changed 6 years ago by
My wish above may be best served by different ticket. Can you put this one in "need review".
comment:40 follow-up: ↓ 58 Changed 6 years ago by
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.
comment:41 follow-up: ↓ 43 Changed 6 years ago by
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?
comment:42 follow-up: ↓ 44 Changed 6 years ago by
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?
comment:43 in reply to: ↑ 41 ; follow-up: ↓ 46 Changed 6 years ago by
Replying to vdelecroix:
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
version
method onFeatures
(or at least onExecutable
)? 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?
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
.
comment:44 in reply to: ↑ 42 ; follow-up: ↓ 45 Changed 6 years ago by
Replying to vdelecroix:
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.
comment:45 in reply to: ↑ 44 Changed 6 years ago by
Replying to saraedum:
Replying to vdelecroix:
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 atSAGE_MAGMA_COMMAND
andSAGE_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.
comment:46 in reply to: ↑ 43 ; follow-up: ↓ 59 Changed 6 years ago by
Replying to saraedum:
Replying to vdelecroix:
[...]
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?
comment:47 Changed 6 years ago by
- Commit changed from b6c4797ace44ecb6b85ec1d17e53258970866e7d to 2a67f26dbe72f17af0500772b8370013d89f44d2
Branch pushed to git repo; I updated commit sha1. New commits:
2a67f26 | Trivial doctest fixes in sage.misc.feature
|
comment:48 Changed 6 years ago by
- Commit changed from 2a67f26dbe72f17af0500772b8370013d89f44d2 to 948bb459cccead366a9c98b93c7743c88583c78f
Branch pushed to git repo; I updated commit sha1. New commits:
948bb45 | Fill in missing doctests for sage.misc.feature
|
comment:49 Changed 6 years ago by
- Commit changed from 948bb459cccead366a9c98b93c7743c88583c78f to e824e0d72c3e8195c8e23582192e55e6acf4907a
Branch pushed to git repo; I updated commit sha1. New commits:
e824e0d | Fixed doctests in sage.misc.feature
|
comment:50 Changed 6 years ago by
- Commit changed from e824e0d72c3e8195c8e23582192e55e6acf4907a to 7cf972c3f7a78e4c064e0174ae7ae1bf1a6e003e
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
|
comment:51 in reply to: ↑ 2 Changed 6 years ago by
Replying to fbissey:
[...] 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.
comment:52 in reply to: ↑ 6 Changed 6 years ago by
Replying to fbissey:
Handling of
database_gap
will be delicate. The spkg includes bits removed stuff fromgap
that 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?
comment:53 in reply to: ↑ 9 Changed 6 years ago by
Replying to fbissey:
Going through the files that I have changed in #20377 which this ticket should supersede:
sage/databases/cremona.py
we 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.py
look for thelrs
executable
Addressed with the Lrs
feature.
sage/graphs/generators/classical_geometries.py
already covered in this ticketsage/graphs/generic_graph.py
I think my stuff should stay. It is mostly straighttry...expect
whether or not a python module is available.
I am checking this now with a PythonModule
feature.
sage/graphs/graph_generators.py
3 different executables to detect
I am checking for these executables (and run them on trivial inputs.)
sage/graphs/lovasz_theta.py
covered heresage/groups/generic.py
possibly 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.py
again dodgy database_gap detection in one part. The part using the gaphap
package 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.py
my stuff should stay. It is removal ofimport
no one uses.
Sure. But I am not addressing this here.
sage/rings/polynomial/multi_polynomial_sequence.py
finding 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.)
comment:54 in reply to: ↑ 15 Changed 6 years ago by
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.
comment:55 in reply to: ↑ 16 Changed 6 years ago by
comment:56 in reply to: ↑ 17 Changed 6 years ago by
comment:57 Changed 6 years ago by
- Commit changed from 7cf972c3f7a78e4c064e0174ae7ae1bf1a6e003e to 7d381385007870e353d1d63e4f995162e3c7285a
Branch pushed to git repo; I updated commit sha1. New commits:
7d38138 | Renamed sage.misc.feature to sage.misc.feature_test
|
comment:58 in reply to: ↑ 40 Changed 6 years ago by
Replying to vdelecroix:
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.
comment:59 in reply to: ↑ 46 Changed 6 years ago by
Replying to vdelecroix:
Replying to saraedum:
Replying to vdelecroix:
[...]
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 whetheroctave --no-gui
is available. If octave does not exist I got aOSError
and if it does not accept--no-gui
I get a1
return code. Is that what I should do? Does that should be part of anOctave
feature?
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.
comment:60 Changed 6 years ago by
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.
comment:61 Changed 6 years ago by
- Work issues reorder classes in sage.misc.feature deleted
comment:62 Changed 6 years ago by
- Commit changed from 7d381385007870e353d1d63e4f995162e3c7285a to edd4f6ed42b58f9b8b88e6349b0439ea7fbc3447
comment:63 Changed 6 years ago by
- Status changed from new to needs_review
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
|
comment:64 Changed 6 years ago by
- Cc mkoeppe jdemeyer added
- Milestone changed from sage-7.2 to sage-7.4
comment:65 Changed 6 years ago by
- Commit changed from edd4f6ed42b58f9b8b88e6349b0439ea7fbc3447 to 2111eb35d8a7ca8a13ee477ee0839d98c24b4fa2
Branch pushed to git repo; I updated commit sha1. New commits:
2111eb3 | Move Bliss feature test to feature_test.py
|
comment:66 Changed 6 years ago by
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.
comment:67 Changed 6 years ago by
- Commit changed from 2111eb35d8a7ca8a13ee477ee0839d98c24b4fa2 to 44e9a5f46a85a3c9c5835a66c65f38edc4483b0c
comment:68 Changed 6 years ago by
- Status changed from needs_review to needs_work
Needs to be merged with Sage 7.4.beta2.
comment:69 follow-up: ↓ 75 Changed 6 years ago by
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.
comment:70 follow-up: ↓ 76 Changed 6 years ago by
The changes to src/sage/misc/cython.py
might make sense but do not belong to this ticket.
comment:71 follow-up: ↓ 72 Changed 6 years ago by
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.
comment:72 in reply to: ↑ 71 Changed 6 years ago by
Replying to fbissey:
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.
Ideally, Sage should have something like Gentoo's USE
flags for optional dependencies...
comment:73 Changed 6 years ago by
- Cc leif added
comment:74 Changed 6 years ago by
- Commit changed from 44e9a5f46a85a3c9c5835a66c65f38edc4483b0c to a427d751fb1531306c8e35fe8cdfc7f755b8f243
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
|
comment:75 in reply to: ↑ 69 ; follow-up: ↓ 80 Changed 6 years ago by
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.py
to decide which optional extensions to build.
That would be a followup ticket I believe.
comment:76 in reply to: ↑ 70 Changed 6 years ago by
Replying to jdemeyer:
The changes to
src/sage/misc/cython.py
might 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.
comment:77 Changed 6 years ago by
- Commit changed from a427d751fb1531306c8e35fe8cdfc7f755b8f243 to 20862171588e5dbcd0d08e77cf8b0ab8cc718581
Branch pushed to git repo; I updated commit sha1. New commits:
2086217 | Isolate library checks from the OptionalModule implementation code
|
comment:78 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:79 Changed 6 years ago by
- Dependencies set to #21289
- Status changed from needs_review to needs_work
Sorry, but it will need rebasing against #21289 which is already merged for the next beta.
comment:80 in reply to: ↑ 75 ; follow-up: ↓ 81 Changed 6 years ago by
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.py
to decide which optional extensions to build.That would be a followup ticket I believe.
Fair enough.
comment:81 in reply to: ↑ 80 ; follow-up: ↓ 82 Changed 6 years ago by
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
OptionalModule
altogether 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.
comment:82 in reply to: ↑ 81 ; follow-up: ↓ 83 Changed 6 years ago by
Replying to saraedum:
But I would like to tell people what is the problem here.
The problem is that they need to install the optional Sage package. That's what you should tell.
In that particular case, the module also would not import but that's more obscure than telling people that the required library is missing.
But the OptionalModule
machine checks whether the optional Sage package is installed. Having the library available from the OS would not help.
Running some tests to make sure that the underlying library is working properly is a reasonable thing to do imho.
I don't like to put the same information in different places. That way, you will eventually end up with inconsistencies. The information that the module needs the library should be in exactly one place, namely the build system which decides when the module is compiled.
comment:83 in reply to: ↑ 82 ; follow-up: ↓ 84 Changed 6 years ago by
Replying to jdemeyer:
Replying to saraedum:
But I would like to tell people what is the problem here.
The problem is that they need to install the optional Sage package. That's what you should tell.
In that particular case, the module also would not import but that's more obscure than telling people that the required library is missing.
But the
OptionalModule
machine checks whether the optional Sage package is installed. Having the library available from the OS would not help.
Indeed. The current build system assumes that if the package is installed it is because I explicitly want it in sage. It is a long standing convention which from where I sit on the outside is called automagic detection. The reality is that it belongs to configuration space (with availability checking - autoconf style). Sure it is possibly more risky to use an outside package but if it satisfy the availability checking, why not?
After all, at least some of us want to push sage down that way.
Also note that for executables Features
only cares about availability in the path, from sage or not (as it should as far as I am concerned). So not caring whether it is from sage or not is in spirit.
I'll admit to have a bias since I want most of sage/misc/package.py
to be gone with a top priority on is_package_installed
in particular.
comment:84 in reply to: ↑ 83 Changed 6 years ago by
Replying to fbissey:
The reality is that it belongs to configuration space (with availability checking - autoconf style). Sure it is possibly more risky to use an outside package but if it satisfy the availability checking, why not?
I'm not disagreeing with that. However, it is besides the point. This ticket should implement the current reality, not some future plan.
comment:85 follow-up: ↓ 88 Changed 6 years ago by
I don't think feature
can replace the current reality of what happens in OptionalExtension
. It should be strictly future work. I am OK for it to be left as is until we decide it is time to move.
comment:86 Changed 6 years ago by
I guess if building and possibly documentation are the only place left using stuff from package.py
it could be moved to sage_setup
where things not needed at run-time belongs in my opinion.
comment:87 Changed 6 years ago by
documentation -> doctesting
comment:88 in reply to: ↑ 85 ; follow-up: ↓ 90 Changed 6 years ago by
I think I have not made my point clear in 82
What I wanted to say: you should remove the check for libraries from the OptionalModule
features. It is currently (whether you like it or not) the job of OptionalExtension
in sage_setup
to decide whether or not to build the Python module. I don't see the point of the redundant check in OptionalModule
. It needlessly complicates things and it is actually wrong (OptionalModule
requires a Sage package, not a system package).
comment:89 Changed 6 years ago by
- Branch changed from u/saraedum/replace_is_package_installed_with_features to u/fbissey/features
- Commit changed from 20862171588e5dbcd0d08e77cf8b0ab8cc718581 to e23341dacc43e5bc691dd7585d639ce8ef9a0cf7
Rebasing on current develop. Someone else should check that I did everything right. One worry when building in parallel building of documentation kept on failing on :module:
in package.py
. When I built serial it just worked.
New commits:
e23341d | Merge branch 'develop' into features
|
comment:90 in reply to: ↑ 88 ; follow-up: ↓ 92 Changed 6 years ago by
Replying to jdemeyer:
I think I have not made my point clear in 82
What I wanted to say: you should remove the check for libraries from the
OptionalModule
features. It is currently (whether you like it or not) the job ofOptionalExtension
insage_setup
to decide whether or not to build the Python module. I don't see the point of the redundant check inOptionalModule
. It needlessly complicates things and it is actually wrong (OptionalModule
requires a Sage package, not a system package).
I don't understand. I meant to write this so it works for a sage that has not been installed through sage-the-distribution. So, for the module to work, I have to check that it has been compiled and that the library it uses actually works. For sage-the-distribution the second part is redundant for other distributions it may not be.
comment:91 Changed 6 years ago by
- Cc embray defeo added
comment:92 in reply to: ↑ 90 Changed 6 years ago by
Replying to saraedum:
So, for the module to work, I have to check that it has been compiled and that the library it uses actually works.
No, you don't need to check anything. The module is linked against the library. If the library didn't work, the module probably wouldn't even compile and it certainly wouldn't be importable. If you can import the module, then surely the library works.
comment:93 Changed 6 years ago by
- Work issues set to does not apply
comment:94 Changed 6 years ago by
Something I don't quite get about this: It seems to be purely for runtime feature checks, which is fine and a good thing to have. But is there also any intention or thought put into making this usable for build-time feature checks when building Sage? Is there any way this could be used to check the system for features during ./configure
(i.e. as a replacement for a m4 scripts to do the same)?
comment:95 Changed 6 years ago by
When we originally discussed this during Sage days the idea was to eventually use it for both, build and compile time. For that it would have to be moved out of the sage library probably. So yes, we thought about this as a possible second step.
comment:96 Changed 6 years ago by
Thanks for clarifying that. Yes, for that reason alone it would make sense to have as a stand-alone package of sorts.
Is there anything I can do to help move this forward? I think getting this to a state where it can also detect already installed dependencies will help massively speed up development time in some cases.
comment:97 follow-up: ↓ 98 Changed 6 years ago by
I am quite happy to have it for run-time only in the first instance. One issue for building is that we are not quite ready to use non sage installed optional package at this stage (i.e. finding optional package not installed by sage).
Fixing runtime would remove a burden from sage-on-distro, especially the bits depending on is_package_installed
that we don't want to use in a distro. Build time can be fixed if the distro decides to support a particular option. But ideally we would want something more fine grained. Ideally it would tie up with a proper configuration phase.
comment:98 in reply to: ↑ 97 Changed 6 years ago by
Replying to fbissey:
I am quite happy to have it for run-time only in the first instance. One issue for building is that we are not quite ready to use non sage installed optional package at this stage (i.e. finding optional package not installed by sage).
I'm not sure what you mean exactly by this. Why should optional packages be a problem as long as this can be useful for non-optional packages?
Fixing runtime would remove a burden from sage-on-distro, especially the bits depending on
is_package_installed
that we don't want to use in a distro. Build time can be fixed if the distro decides to support a particular option. But ideally we would want something more fine grained. Ideally it would tie up with a proper configuration phase.
Yes, that work is going on in #21566 and other associated tickets.
To be clear, I think we're in agreement that this ticket should not handle any tasks directly related to enabling build-time detection of dependencies. I agree that's a separate issue. My point is that we should keep that use-case in mind for this ticket insofar as the checks its provides don't depend on having a fully working Sage install already, and that it exists outside the main Sage package.
In other words, this ticket could remain focused on fixing the is_package_installed
issue, but just keep in mind the other use case that this code could help resolve.
comment:99 Changed 5 years ago by
- Branch changed from u/fbissey/features to u/saraedum/features
comment:100 follow-up: ↓ 111 Changed 5 years ago by
- Branch changed from u/saraedum/features to u/fbissey/features
After some discussion with jdemeyer, I'll remove the current checks OptionalModule?. I'll leave the possibility for such checks in though.
comment:101 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:102 Changed 5 years ago by
- Branch changed from u/fbissey/features to u/saraedum/features
comment:103 Changed 5 years ago by
- Commit changed from e23341dacc43e5bc691dd7585d639ce8ef9a0cf7 to c809debf750e7ee2fb3d2792f4ff73ab5626120e
- Reviewers changed from Nicolas M. Thiéry, ... to Nicolas M. Thiéry
- Work issues does not apply deleted
comment:104 Changed 5 years ago by
- Keywords days85 added
comment:105 Changed 5 years ago by
- Dependencies #21289 deleted
comment:106 Changed 5 years ago by
Shouldn't all feature checks be cached?
comment:107 Changed 5 years ago by
Copy-paste typo:
class Bliss(OptionalModule): r""" A :class:`Feature` which describes whether the ;module:`sage.graphs.bliss` module has been enabled for this build of Sage and is functional. .. NOTE:: ------> This module does not depend on :class:`LibFESLibrary`. If the module imports we just assume that the library is working.
comment:108 Changed 5 years ago by
I don't like _test_code
: you define the attribute _test_code
, pass it through __init__
and then assign it as the test_code
attribute. Why so convoluted, you might as well assign test_code
directly.
comment:109 Changed 5 years ago by
Speaking of test code, wouldn't it be cleaner to move the test code snippets to separate files? Right now, I find it a bit distracting to have the Cython test code in between the Python code implementing the testing logic.
comment:110 Changed 5 years ago by
Typo: ;module:`sage.libs.fes`
comment:111 in reply to: ↑ 100 ; follow-up: ↓ 126 Changed 5 years ago by
Replying to saraedum:
After some discussion with jdemeyer, I'll remove the current checks OptionalModule?.
Given that OptionalModule
and Module
essentially do the same thing, can you just get rid of the OptionalModule
class completely?
comment:112 Changed 5 years ago by
I don't really like the names LibFES
and Bliss
. I would prefer something which clearly reminds that this is just about the module: I would call them FESModule
and BlissModule
.
comment:113 Changed 5 years ago by
Another suggestion: split this one big file src/sage/misc/feature_test.py
up. It would be good to separate the abstract base classes (like Executable
, SharedLibrary
,...) from the actual concrete tests.
Personally, I would even have one file for each package, for example src/sage/misc/feature_test/bliss.py
for BlissLibrary
and BlissModule
. In this case, you can define the Cython test code as module global, which would solve 109 too.
comment:114 Changed 5 years ago by
- Status changed from needs_review to needs_work
Except for the typos, we do not fully agree on whether the above should be changed or not. jdemeyer agreed to split up the module, introduce caching, and do the renamings himself.
comment:115 Changed 5 years ago by
- Owner changed from (none) to jdemeyer
comment:116 Changed 5 years ago by
- Dependencies set to #22113
comment:117 Changed 5 years ago by
This is a minor nit, but is there a reason this isn't using super().__init__
for calling the base-class __init__
s?
comment:118 Changed 5 years ago by
- Cc isuruf added
comment:119 Changed 5 years ago by
- Status changed from needs_work to needs_review
What should we do about this issue? I had put a lot of work into this and I would like to get this merged.
Jeroen: Would you agree to merge this as is now (after fixing patchbot errors)? Or can I expect a patch anytime soon?
comment:120 Changed 5 years ago by
I've in the meantime started moving in a different direction for build-time feature dependency checks, so to me at least my earlier questions about using this for that purpose are moot. But I'm still +1 on having it for runtime feature checks.
It looks like the patch does not apply so I think you'll have to set this back to needs_work. I'd be happy to help rebasing your patch if need be.
comment:121 Changed 5 years ago by
- Branch changed from u/saraedum/features to u/jdemeyer/features
comment:122 Changed 5 years ago by
- Commit changed from c809debf750e7ee2fb3d2792f4ff73ab5626120e to 3deea8282218b1549179972183d37b354da9d2e4
- Dependencies #22113 deleted
- Status changed from needs_review to needs_work
comment:123 Changed 5 years ago by
In some cases, bigger changes were made than just using this new Features
framework. I'll split those off as separate tickets.
comment:124 Changed 5 years ago by
- Commit changed from 3deea8282218b1549179972183d37b354da9d2e4 to c244878077536ad9bca90b09b8279447bbe2114d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c244878 | Add Features to check the environment at runtime
|
comment:125 Changed 5 years ago by
It seems that SharedLibrary
is not checking for libraries at all: it just tries to compile some Cython code. So it would be better to rename it as CythonFeature
or whatever better name I can come up with...
comment:126 in reply to: ↑ 111 Changed 5 years ago by
Replying to jdemeyer:
Given that
OptionalModule
andModule
essentially do the same thing, can you just get rid of theOptionalModule
class completely?
I will remove OptionalModule
and also rename Module
-> PythonModule
.
comment:127 Changed 5 years ago by
I just found a problem which is not easy to fix: there is no way as far as I can tell to silence compiler error in distutils. This means that feature checks will show messages like
/usr/lib/gcc/x86_64-pc-linux-gnu/4.9.4/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lxxxxxxxx collect2: error: ld returned 1 exit status
comment:128 Changed 5 years ago by
I could take a look at that. I feel like that's a problem I've solved once before, also when using distutils for feature checks (even if it means temporarily redirecting stdio or something).
comment:129 Changed 5 years ago by
- Commit changed from c244878077536ad9bca90b09b8279447bbe2114d to 91558bd79ce6acfd5afec7089d57f66f5d891950
comment:130 Changed 5 years ago by
- Commit changed from 91558bd79ce6acfd5afec7089d57f66f5d891950 to 92f9e79f10da8b1ef9e32f0db49aa185908cdb59
comment:131 Changed 5 years ago by
- Commit changed from 92f9e79f10da8b1ef9e32f0db49aa185908cdb59 to 35f6601ed41d1e1f767c46a26b4dea2cadf0fa1f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
35f6601 | Cleanup feature tests
|
comment:132 Changed 5 years ago by
- Status changed from needs_work to needs_review
I'm setting this to needs_review mainly for patchbot testing. It's probably going to break somewhere, but it should mostly work.
comment:133 Changed 5 years ago by
- Dependencies set to #24720
comment:134 Changed 5 years ago by
- Commit changed from 35f6601ed41d1e1f767c46a26b4dea2cadf0fa1f to 5e3ede550ab51759d2cdb3e0598eb34e7dab277a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5e3ede5 | Cleanup feature tests
|
comment:135 Changed 5 years ago by
- Commit changed from 5e3ede550ab51759d2cdb3e0598eb34e7dab277a to 56a05312a139ea6f77f727a3fc379b563b386f9a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
56a0531 | Cleanup feature tests
|
comment:136 Changed 5 years ago by
- Status changed from needs_review to needs_work
comment:137 Changed 5 years ago by
- Commit changed from 56a05312a139ea6f77f727a3fc379b563b386f9a to 69ad20017c57129525a4a6b34f0cdb4711021467
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
69ad200 | Cleanup feature tests
|
comment:138 Changed 5 years ago by
I now consider this finished, apart from #24720.
comment:139 Changed 5 years ago by
- Commit changed from 69ad20017c57129525a4a6b34f0cdb4711021467 to af22cc45791b95046ae5e50a7f989290257e224b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
af22cc4 | Cleanup feature tests
|
comment:140 Changed 5 years ago by
- Commit changed from af22cc45791b95046ae5e50a7f989290257e224b to 3696d1979dc71abb9a3206eb95f5af725a8b1c6f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3696d19 | Cleanup feature tests
|
comment:141 Changed 5 years ago by
- Commit changed from 3696d1979dc71abb9a3206eb95f5af725a8b1c6f to 32a8eab55a755a24faf4b3ba9bc8c796bbfa4fe6
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
32a8eab | Cleanup feature tests
|
comment:142 follow-up: ↓ 143 Changed 5 years ago by
It looks really good. There are a few files remaining to migrate to "features" not covered here but that's good for a first iteration and will prove that the concept work. One more file should be migrated in this ticket I think, databases/cremona.py
because it is the only file that would use features/databases.py
as it currently stands.
For info the following files have stuff to convert
- coding/linear_code.py
- coding/guava.py
- coding/databases.py
- graphs/generators/smallgraphs.py
- groups/abelian_gps/abelian_group.py
All of these are about some gap packages.
comment:143 in reply to: ↑ 142 Changed 5 years ago by
comment:144 Changed 4 years ago by
comment:145 Changed 4 years ago by
- Milestone changed from sage-7.4 to sage-8.2
- Reviewers changed from Nicolas M. Thiéry to Nicolas M. Thiéry, François Bissey
- Status changed from needs_work to positive_review
OK I think it is time to move this ticket further.
comment:146 Changed 4 years ago by
- Status changed from positive_review to needs_work
I never set this to needs_review.
comment:147 Changed 4 years ago by
There is one issue remaining: this shouldn't use cython_import()
but a direct cython()
call. For simplicity, I plan to do that once all dependencies are merged.
comment:148 Changed 4 years ago by
Ok, at https://trac.sagemath.org/ticket/20382#comment:138 you said you considered this done. So I assumed too much out of that. Works well as far as I am concerned.
comment:149 Changed 4 years ago by
I guess I meant more broadly "apart from issues with Cython".
comment:150 Changed 4 years ago by
It looks like the dependencies that you listed will be in the next beta - which could turn out to be a rc at this stage. Are there any more tickets that need to go in prior to this one in your opinion?
comment:151 Changed 4 years ago by
#24764 would be nice because it touches the same code.
comment:153 Changed 4 years ago by
- Dependencies #24720 deleted
comment:154 Changed 4 years ago by
- Commit changed from 32a8eab55a755a24faf4b3ba9bc8c796bbfa4fe6 to ad6dcc6fc50723b616ae32f23ea592c8a27da5ca
comment:155 follow-up: ↓ 159 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-pending
- Status changed from needs_work to needs_review
Setting milestone to sage-pending because I think that this ticket should not be merged in Sage 8.2. The reason is that this ticket has a lot of potential to break stuff because it involves optional packages and those are tested poorly. This would be an ideal ticket to merge in 8.3.beta0.
comment:156 Changed 4 years ago by
- Reviewers changed from Nicolas M. Thiéry, François Bissey to Nicolas M. Thiéry, François Bissey, Julian Rüth
- Status changed from needs_review to positive_review
comment:157 Changed 4 years ago by
- Milestone changed from sage-pending to sage-8.3
comment:158 Changed 4 years ago by
- Branch changed from u/jdemeyer/features to ad6dcc6fc50723b616ae32f23ea592c8a27da5ca
- Resolution set to fixed
- Status changed from positive_review to closed
comment:159 in reply to: ↑ 155 Changed 4 years ago by
- Commit ad6dcc6fc50723b616ae32f23ea592c8a27da5ca deleted
Replying to jdemeyer:
Setting milestone to sage-pending because I think that this ticket should not be merged in Sage 8.2. The reason is that this ticket has a lot of potential to break stuff because it involves optional packages and those are tested poorly. This would be an ideal ticket to merge in 8.3.beta0.
Indeed... help for fixing the consequences in: #25332
And one question: the LattE feature from #25400 needs to check for two executables. Should I introduce an abstract class feature "Executables" (whose constructor argument would be a list of Executable
classes)?
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 thePATH
. Similarly you can installgap
packages in~/.gap/pkg
and there is no reasons why you shouldn't be able to use those.New commits:
Add Features to check the environment at runtime
Check for grape with GapPackage instead of is_package_installed()
Check for a working CSDP binary instead of is_package_installed()