Opened 3 years ago

Closed 17 months ago

Last modified 16 months ago

#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) Commit:
Dependencies: Stopgaps:

Description


Change History (159)

comment:1 Changed 3 years ago by saraedum

  • Branch set to u/saraedum/replace_is_package_installed_with_features

comment:2 follow-up: Changed 3 years ago by fbissey

  • Commit set to 7c1ca312c14f1b777563607d919a245e1d48e557

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:

3233147Add Features to check the environment at runtime
dfd97f8Check for grape with GapPackage instead of is_package_installed()
7c1ca31Check for a working CSDP binary instead of is_package_installed()

comment:3 Changed 3 years ago by nthiery

  • Branch changed from u/saraedum/replace_is_package_installed_with_features to u/nthiery/replace_is_package_installed_with_features

comment:4 Changed 3 years ago by git

  • Commit changed from 7c1ca312c14f1b777563607d919a245e1d48e557 to 97d47afe5405f899e63e655f2aedf409ad35e863

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

97d47af20382: missing docstring

comment:5 Changed 3 years ago by git

  • Commit changed from 97d47afe5405f899e63e655f2aedf409ad35e863 to 8b6fcc990f756d34e16c0439815ffbc4155e47af

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

8b6fcc920382: desirable error message + typo

comment:6 follow-up: Changed 3 years ago by fbissey

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 3 years ago by saraedum

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 3 years ago by fbissey

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: Changed 3 years ago by 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/game_theory/normal_form_game.py + sage/geometry/polyhedron/base.py look for the lrs executable
  • sage/graphs/generators/classical_geometries.py already covered in this ticket
  • sage/graphs/generic_graph.py I think my stuff should stay. It is mostly straight try...expect whether or not a python module is available.
  • sage/graphs/graph_generators.py 3 different executables to detect
  • sage/graphs/lovasz_theta.py covered here
  • sage/groups/generic.py possibly detect if gap package tomlib is present
  • sage/groups/perm_gps/permgroup.py again dodgy database_gap detection in one part. The part using the gap hap 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 of import no one uses.
  • sage/rings/polynomial/multi_polynomial_sequence.py finding one executable

comment:10 Changed 3 years ago by saraedum

  • Branch changed from u/nthiery/replace_is_package_installed_with_features to u/saraedum/replace_is_package_installed_with_features

comment:11 Changed 3 years ago by jhpalmieri

  • Commit changed from 8b6fcc990f756d34e16c0439815ffbc4155e47af to 1e275a8db635e5c9a0f63f9e37acc245616e94dd

Is there overlap with #20182? Should #20182 be rewritten to use this?


New commits:

1e275a8Merge branch 'develop' into t/20382/replace_is_package_installed_with_features

comment:12 follow-up: Changed 3 years ago by nthiery

  • Authors set to Julian Rüth
  • 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

Last edited 3 years ago by nthiery (previous) (diff)

comment:13 Changed 3 years ago by git

  • Commit changed from 1e275a8db635e5c9a0f63f9e37acc245616e94dd to 1bba301b7255d4725a0cb0e758ada93025b3e53b

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

340d36eImprove reporting of Features
0fc00d9grape is in the gap_packages SPKG
aa12d10Fixed typos in Features
76aedb4check for GAP package "hap" through a Feature test
2e451b420382: Proofreading
97d47af20382: missing docstring
8b6fcc920382: desirable error message + typo
1bba301Merge 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 3 years ago by saraedum

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: Changed 3 years ago by 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.

comment:16 follow-ups: Changed 3 years ago by vdelecroix

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 is True then we mostly don't care about explanation and if it is False we likely want an error (with an explicit error message). Similarly, I would get rid of FeatureNotPresentError. Instead, I would add an error and a message arguments in the require method (with of course some reasonable default which would mimic the current behavior using the standard RuntimeError).
  • Are you sure CSDP is better inside the file lovasz_theta.py? Do we want to spread the Features 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 having CSDP has a class when CSDP is the name of the program. It would be fine if all of them belong to a fixed module.

comment:17 follow-ups: Changed 3 years ago by vdelecroix

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 3 years ago by git

  • Commit changed from 1bba301b7255d4725a0cb0e758ada93025b3e53b to 2a594ee16f72f7bebd06f82d0fdadfe8a526f178

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

7b6df60Buckygen feature
0529a07ignore stderr output of theta
baf2eedBenzene feature test
2b2bbf9Added Feature for plantri
30cacedAdded features for bliss and igraph
3a2f8d2is_package_installed() is deprecated
2a594eeFeature for lrslib

comment:19 in reply to: ↑ 17 Changed 3 years ago by saraedum

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 3 years ago by saraedum

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 is True then we mostly don't care about explanation and if it is False 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 an error and a message arguments in the require method (with of course some reasonable default which would mimic the current behavior using the standard RuntimeError).

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 file lovasz_theta.py? Do we want to spread the Features 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 when CSDP 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.)

Last edited 3 years ago by saraedum (previous) (diff)

comment:21 in reply to: ↑ 15 Changed 3 years ago by saraedum

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 3 years ago by git

  • Commit changed from 2a594ee16f72f7bebd06f82d0fdadfe8a526f178 to 2007e1822e0684078323677211c3a893d5af414e

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

90be504Centralize feature tests
da5e893More checks for Lrs
a9cbda3Move generic resolution information from Executable to Feature
2007e18Add Feature CremonaDatabase

comment:23 Changed 3 years ago by saraedum

  • Work issues set to reorder classes in sage.misc.feature

comment:24 follow-up: Changed 3 years ago by slabbe

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 3 years ago by vdelecroix

Replying to slabbe:

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?

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 3 years ago by nthiery

Maybe something like feature_test? This seems to be consistent with autoconf's vocabulary.

comment:27 Changed 3 years ago by saraedum

I like feature_test. I will rename it to that.

comment:28 follow-up: Changed 3 years ago by vdelecroix

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 3 years ago by fbissey

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 3 years ago by saraedum

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 3 years ago by git

  • Commit changed from 2007e1822e0684078323677211c3a893d5af414e to ef9cf7d1eacb9195ab41611c2f05e634a1bf3d03

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

ef9cf7dMerge branch 'develop' into t/20382/replace_is_package_installed_with_features

comment:32 Changed 3 years ago by git

  • Commit changed from ef9cf7d1eacb9195ab41611c2f05e634a1bf3d03 to 074a31801df66eb96116e3f59ab6b2d0e42bd951

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

095b402fixed incorrect import statement
670a3e2Merge branch 'u/saraedum/replace_is_package_installed_with_features' of git://trac.sagemath.org/sage into t/20382/replace_is_package_installed_with_features
6f5dc21Completed implementation of CremonaDatabase feature
074a318Implement SmallGroupsLibrary feature

comment:33 Changed 3 years ago by git

  • Commit changed from 074a31801df66eb96116e3f59ab6b2d0e42bd951 to 6d508a11632ef2f8931c3f8b2ad709e6dffe5446

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

6d508a1Merge branch 'develop' of https://github.com/sagemath/sage into t/20382/replace_is_package_installed_with_features

comment:34 Changed 3 years ago by git

  • Commit changed from 6d508a11632ef2f8931c3f8b2ad709e6dffe5446 to b6c4797ace44ecb6b85ec1d17e53258970866e7d

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

10c1a47Asking whether a package is installed is not deprecated
680c19aremoved unused import
bb47ef0Replace is_installed with SmallGroupsLibrary.require
3acffa7A note on is_package_installed versus features
b6c4797Optional sage modules and shared libraries as features

comment:35 Changed 3 years ago by saraedum

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 3 years ago by fbissey

That's really cool. I will give it a good look. I may even try it in sage-on-gentoo.

comment:37 Changed 3 years ago by saraedum

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 3 years ago by fbissey

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 3 years ago by fbissey

My wish above may be best served by different ticket. Can you put this one in "need review".

comment:40 follow-up: Changed 3 years ago by 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 True

This would better be documented.

comment:41 follow-up: Changed 3 years ago by 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... 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: Changed 3 years ago by 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')
2

Does a magma feature should take care of this?

comment:43 in reply to: ↑ 41 ; follow-up: Changed 3 years ago by saraedum

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 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?

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: Changed 3 years ago by 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')
2

Does 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 3 years ago by vdelecroix

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')
2

Does 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.

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: Changed 3 years ago by 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 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 3 years ago by git

  • Commit changed from b6c4797ace44ecb6b85ec1d17e53258970866e7d to 2a67f26dbe72f17af0500772b8370013d89f44d2

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

2a67f26Trivial doctest fixes in sage.misc.feature

comment:48 Changed 3 years ago by git

  • Commit changed from 2a67f26dbe72f17af0500772b8370013d89f44d2 to 948bb459cccead366a9c98b93c7743c88583c78f

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

948bb45Fill in missing doctests for sage.misc.feature

comment:49 Changed 3 years ago by git

  • Commit changed from 948bb459cccead366a9c98b93c7743c88583c78f to e824e0d72c3e8195c8e23582192e55e6acf4907a

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

e824e0dFixed doctests in sage.misc.feature

comment:50 Changed 3 years ago by git

  • Commit changed from e824e0d72c3e8195c8e23582192e55e6acf4907a to 7cf972c3f7a78e4c064e0174ae7ae1bf1a6e003e

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

7cf972cMerge branch 'develop' of https://github.com/sagemath/sage into t/20382/replace_is_package_installed_with_features

comment:51 in reply to: ↑ 2 Changed 3 years ago by saraedum

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 3 years ago by saraedum

Replying to fbissey:

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).

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 3 years ago by saraedum

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 the lrs executable

Addressed with the Lrs feature.

  • sage/graphs/generators/classical_geometries.py already covered in this ticket
  • sage/graphs/generic_graph.py I think my stuff should stay. It is mostly straight try...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 here
  • sage/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 gap hap 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 of import 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 3 years ago by saraedum

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 3 years ago by saraedum

Last edited 3 years ago by saraedum (previous) (diff)

comment:56 in reply to: ↑ 17 Changed 3 years ago by saraedum

Last edited 3 years ago by saraedum (previous) (diff)

comment:57 Changed 3 years ago by git

  • Commit changed from 7cf972c3f7a78e4c064e0174ae7ae1bf1a6e003e to 7d381385007870e353d1d63e4f995162e3c7285a

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

7d38138Renamed sage.misc.feature to sage.misc.feature_test

comment:58 in reply to: ↑ 40 Changed 3 years ago by saraedum

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 True

This would better be documented.

You are right. I documented that.

comment:59 in reply to: ↑ 46 Changed 3 years ago by saraedum

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 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?

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 3 years ago by saraedum

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 3 years ago by saraedum

  • Work issues reorder classes in sage.misc.feature deleted

comment:62 Changed 3 years ago by git

  • Commit changed from 7d381385007870e353d1d63e4f995162e3c7285a to edd4f6ed42b58f9b8b88e6349b0439ea7fbc3447

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

0529637Reordered classes in sage.misc.feature_test
6f2d596Fixed doctests which failed because of the feature -> feature_test renaming
edd4f6eadd missing sig_off to doctest

comment:63 Changed 3 years ago by saraedum

  • Status changed from new to needs_review

New commits:

0529637Reordered classes in sage.misc.feature_test
6f2d596Fixed doctests which failed because of the feature -> feature_test renaming
edd4f6eadd missing sig_off to doctest

New commits:

0529637Reordered classes in sage.misc.feature_test
6f2d596Fixed doctests which failed because of the feature -> feature_test renaming
edd4f6eadd missing sig_off to doctest

comment:64 Changed 3 years ago by mkoeppe

  • Cc mkoeppe jdemeyer added
  • Milestone changed from sage-7.2 to sage-7.4

comment:65 Changed 3 years ago by git

  • Commit changed from edd4f6ed42b58f9b8b88e6349b0439ea7fbc3447 to 2111eb35d8a7ca8a13ee477ee0839d98c24b4fa2

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

2111eb3Move Bliss feature test to feature_test.py

comment:66 Changed 3 years ago by saraedum

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 3 years ago by git

  • Commit changed from 2111eb35d8a7ca8a13ee477ee0839d98c24b4fa2 to 44e9a5f46a85a3c9c5835a66c65f38edc4483b0c

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

c3857a6Bliss feature is an OptionalModule
44e9a5fFix suffix of cythonized files

comment:68 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Needs to be merged with Sage 7.4.beta2.

comment:69 follow-up: Changed 3 years ago by 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.

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: Changed 3 years ago by jdemeyer

The changes to src/sage/misc/cython.py might make sense but do not belong to this ticket.

comment:71 follow-up: Changed 3 years ago by 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. 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 3 years ago by jdemeyer

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 3 years ago by leif

  • Cc leif added

comment:74 Changed 3 years ago by git

  • Commit changed from 44e9a5f46a85a3c9c5835a66c65f38edc4483b0c to a427d751fb1531306c8e35fe8cdfc7f755b8f243

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

a427d75Merge remote-tracking branch 'origin/develop' into t/20382/replace_is_package_installed_with_features

comment:75 in reply to: ↑ 69 ; follow-up: Changed 3 years ago by 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.

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 3 years ago by saraedum

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 3 years ago by git

  • Commit changed from a427d751fb1531306c8e35fe8cdfc7f755b8f243 to 20862171588e5dbcd0d08e77cf8b0ab8cc718581

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

2086217Isolate library checks from the OptionalModule implementation code

comment:78 Changed 3 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:79 Changed 3 years ago by fbissey

  • 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: Changed 3 years ago by 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 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.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:81 in reply to: ↑ 80 ; follow-up: Changed 3 years ago by saraedum

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 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.

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: Changed 3 years ago by 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.

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: Changed 3 years ago by fbissey

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 3 years ago by jdemeyer

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: Changed 3 years ago by fbissey

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 3 years ago by fbissey

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 3 years ago by fbissey

documentation -> doctesting

comment:88 in reply to: ↑ 85 ; follow-up: Changed 3 years ago by 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 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 3 years ago by fbissey

  • 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:

e23341dMerge branch 'develop' into features

comment:90 in reply to: ↑ 88 ; follow-up: Changed 3 years ago by saraedum

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 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).

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.

Last edited 3 years ago by saraedum (previous) (diff)

comment:91 Changed 3 years ago by defeo

  • Cc embray defeo added

comment:92 in reply to: ↑ 90 Changed 3 years ago by jdemeyer

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 3 years ago by saraedum

  • Work issues set to does not apply

comment:94 Changed 3 years ago by embray

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 3 years ago by saraedum

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 3 years ago by embray

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: Changed 3 years ago by 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).

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 3 years ago by embray

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 3 years ago by saraedum

  • Branch changed from u/fbissey/features to u/saraedum/features

comment:100 follow-up: Changed 3 years ago by saraedum

  • 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 3 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:102 Changed 3 years ago by saraedum

  • Branch changed from u/fbissey/features to u/saraedum/features

comment:103 Changed 3 years ago by saraedum

  • Commit changed from e23341dacc43e5bc691dd7585d639ce8ef9a0cf7 to c809debf750e7ee2fb3d2792f4ff73ab5626120e
  • Reviewers changed from Nicolas M. Thiéry, ... to Nicolas M. Thiéry
  • Work issues does not apply deleted

New commits:

a3947c4Merge branch 'develop' into t/20382/features
c809debRemove runtime dependencies on shared libraries

comment:104 Changed 3 years ago by saraedum

  • Keywords days85 added

comment:105 Changed 3 years ago by jdemeyer

  • Dependencies #21289 deleted

comment:106 Changed 3 years ago by jdemeyer

Shouldn't all feature checks be cached?

comment:107 Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

Typo: ;module:`sage.libs.fes`

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:111 in reply to: ↑ 100 ; follow-up: Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by saraedum

  • 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 3 years ago by saraedum

  • Owner changed from (none) to jdemeyer

comment:116 Changed 3 years ago by jdemeyer

  • Dependencies set to #22113

comment:117 Changed 2 years ago by embray

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 2 years ago by isuruf

  • Cc isuruf added

comment:119 Changed 20 months ago by saraedum

  • 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 20 months ago by embray

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 19 months ago by jdemeyer

  • Branch changed from u/saraedum/features to u/jdemeyer/features

comment:122 Changed 19 months ago by jdemeyer

  • Authors changed from Julian Rüth to Julian Rüth, Jeroen Demeyer
  • Commit changed from c809debf750e7ee2fb3d2792f4ff73ab5626120e to 3deea8282218b1549179972183d37b354da9d2e4
  • Dependencies #22113 deleted
  • Status changed from needs_review to needs_work

I'll clean this up.


New commits:

3deea82Add Features to check the environment at runtime

comment:123 Changed 19 months ago by jdemeyer

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 19 months ago by git

  • Commit changed from 3deea8282218b1549179972183d37b354da9d2e4 to c244878077536ad9bca90b09b8279447bbe2114d

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

c244878Add Features to check the environment at runtime

comment:125 Changed 19 months ago by jdemeyer

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 19 months ago by jdemeyer

Replying to jdemeyer:

Given that OptionalModule and Module essentially do the same thing, can you just get rid of the OptionalModule class completely?

I will remove OptionalModule and also rename Module -> PythonModule.

comment:127 Changed 19 months ago by jdemeyer

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 19 months ago by embray

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 19 months ago by git

  • Commit changed from c244878077536ad9bca90b09b8279447bbe2114d to 91558bd79ce6acfd5afec7089d57f66f5d891950

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

00cb88eAdd Features to check the environment at runtime
91558bdCleanup feature tests

comment:130 Changed 19 months ago by git

  • Commit changed from 91558bd79ce6acfd5afec7089d57f66f5d891950 to 92f9e79f10da8b1ef9e32f0db49aa185908cdb59

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

5d85c54Add Features to check the environment at runtime
92f9e79Cleanup feature tests

comment:131 Changed 19 months ago by git

  • Commit changed from 92f9e79f10da8b1ef9e32f0db49aa185908cdb59 to 35f6601ed41d1e1f767c46a26b4dea2cadf0fa1f

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

35f6601Cleanup feature tests

comment:132 Changed 19 months ago by jdemeyer

  • 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 19 months ago by jdemeyer

  • Dependencies set to #24720

comment:134 Changed 19 months ago by git

  • Commit changed from 35f6601ed41d1e1f767c46a26b4dea2cadf0fa1f to 5e3ede550ab51759d2cdb3e0598eb34e7dab277a

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

5e3ede5Cleanup feature tests

comment:135 Changed 19 months ago by git

  • Commit changed from 5e3ede550ab51759d2cdb3e0598eb34e7dab277a to 56a05312a139ea6f77f727a3fc379b563b386f9a

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

56a0531Cleanup feature tests

comment:136 Changed 19 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:137 Changed 19 months ago by git

  • Commit changed from 56a05312a139ea6f77f727a3fc379b563b386f9a to 69ad20017c57129525a4a6b34f0cdb4711021467

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

69ad200Cleanup feature tests

comment:138 Changed 19 months ago by jdemeyer

I now consider this finished, apart from #24720.

comment:139 Changed 19 months ago by git

  • Commit changed from 69ad20017c57129525a4a6b34f0cdb4711021467 to af22cc45791b95046ae5e50a7f989290257e224b

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

af22cc4Cleanup feature tests

comment:140 Changed 19 months ago by git

  • Commit changed from af22cc45791b95046ae5e50a7f989290257e224b to 3696d1979dc71abb9a3206eb95f5af725a8b1c6f

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

3696d19Cleanup feature tests

comment:141 Changed 19 months ago by git

  • Commit changed from 3696d1979dc71abb9a3206eb95f5af725a8b1c6f to 32a8eab55a755a24faf4b3ba9bc8c796bbfa4fe6

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

32a8eabCleanup feature tests

comment:142 follow-up: Changed 19 months ago by fbissey

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 19 months ago by jdemeyer

Replying to fbissey:

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.

I split that off as #24718.

comment:144 Changed 19 months ago by jdemeyer

If you care about this ticket, please review #24722, #24724, #24720 which all deal with improving sage.misc.cython to allow for silent operation.

comment:145 Changed 19 months ago by fbissey

  • 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 19 months ago by jdemeyer

  • Status changed from positive_review to needs_work

I never set this to needs_review.

comment:147 Changed 19 months ago by jdemeyer

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 19 months ago by fbissey

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 19 months ago by jdemeyer

I guess I meant more broadly "apart from issues with Cython".

comment:150 Changed 19 months ago by fbissey

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 19 months ago by jdemeyer

#24764 would be nice because it touches the same code.

comment:152 Changed 18 months ago by slelievre

  • Cc slelievre added

#24764 is in.

comment:153 Changed 18 months ago by jdemeyer

  • Dependencies #24720 deleted

comment:154 Changed 18 months ago by git

  • Commit changed from 32a8eab55a755a24faf4b3ba9bc8c796bbfa4fe6 to ad6dcc6fc50723b616ae32f23ea592c8a27da5ca

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

8e4c504Add Features to check the environment at runtime
ad6dcc6Cleanup feature tests

comment:155 follow-up: Changed 18 months ago by jdemeyer

  • 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 18 months ago by saraedum

  • 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 17 months ago by jdemeyer

  • Milestone changed from sage-pending to sage-8.3

comment:158 Changed 17 months ago by vbraun

  • 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 16 months ago by vdelecroix

  • 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)?

Last edited 16 months ago by vdelecroix (previous) (diff)
Note: See TracTickets for help on using tickets.