Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FriCAS spkg-configure and Feature #35838

Merged
merged 30 commits into from
Dec 6, 2023
Merged

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Jun 26, 2023

As FriCAS is only used as an executable, this is straightforward; quite a number of systems has Fricas 1.3.8, so this is useful, too.

The FriCAS pexpect interface now uses the new Executable feature to determine the absolute pathname of the fricas executable (unless executed remotely).
This is made possible by a simple refactor of the sage.interfaces.expect.Expect class: Computing the effective command line is no longer done in set_server_and_command (called by __init__); it is delayed until an interface is started and needs the command line.

Dependencies: The changes in sage.interfaces outside of .expect and .fricas are all from the following PR and do not need review.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 27, 2023

The doctests marked # optional - fricas won't be run unless you also do #33575

@dimpase
Copy link
Member Author

dimpase commented Jun 27, 2023 via email

@kiwifb
Copy link
Member

kiwifb commented Jun 27, 2023

I am not saying to make it ECL, I am saying that lips user on Gentoo make their own choices unless there is a technical reason (as in a breaking reason) to do so. If they are interested in speed they will make that choice themselves.

@dimpase
Copy link
Member Author

dimpase commented Jun 29, 2023

The doctests marked # optional - fricas won't be run unless you also do #33575

This looks quite heavy-handed. Why can't this be done at ./configure time? What's the magic done by building fricas spkg so that #33575 is not needed?

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 29, 2023

When you build the fricas spkg, there will be an installation record that is found by sage.misc.packages.

This is a Sage-the-distribution-specific mechanism that we want to get rid of altogether (see #35856), for the benefit of downstream packagers and also for the pip-installed setting. See also #35858

@dimpase
Copy link
Member Author

dimpase commented Jun 30, 2023

@dimpase
Copy link
Member Author

dimpase commented Jun 30, 2023

OK, feature seems to be implemented now, please have a look

@@ -0,0 +1,16 @@
SAGE_SPKG_CONFIGURE(
[fricas], [
AC_CACHE_CHECK([for FriCAS >= 1.3.8], [ac_cv_path_FRICAS], [
Copy link
Contributor

Choose a reason for hiding this comment

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

the indentation is a bit wild

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 30, 2023

Has using system fricas been tested on more than 1 machine?

@dimpase
Copy link
Member Author

dimpase commented Jun 30, 2023

Has using system fricas been tested on more than 1 machine?

on 2 different machines, 3 different configurations (using ECL as well as using SBCL as lisp compiler)

@mkoeppe
Copy link
Contributor

mkoeppe commented Jun 30, 2023

Build&Test fails with lots of errors originating from the feature thing

@dimpase
Copy link
Member Author

dimpase commented Jun 30, 2023 via email

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 1, 2023

Yes, in the test run on GH Actions there is no FriCAS because it's an optional package...

@dimpase
Copy link
Member Author

dimpase commented Jul 1, 2023

Build&Test fails with lots of errors originating from the feature thing

well, indeed - I suppose it's due to #33575 pointing to an outdated design, compared to e.g. what's in
src/sage/features/csdp.py

Another potential problem is is_FriCASElement - not sure what should come to replace it.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 1, 2023

Another potential problem is is_FriCASElement - not sure what should come to replace it.

sage.interfaces.abc

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 1, 2023

I'll take a look what needs to be fixed in the feature definition later today.

@dimpase
Copy link
Member Author

dimpase commented Jul 1, 2023

I'll take a look what needs to be fixed in the feature definition later today.

I'm actually working on this now. I'll post an update in an hour or two

@dimpase
Copy link
Member Author

dimpase commented Jul 1, 2023

here is a non-working update. I'm lost at what options are supposed to be there.

sage.features an unpleasantly fragile kludge. Is it supposed to test whether pexpect FriCAS interface works?
Whether fricas executable is there? Really, no idea, no docs...

...
FileNotFoundError: [Errno 2] No such file or directory: 'fricas -nosman'

----------------------------------------------------------------------
sage -t --random-seed=195934185104556495832583672009163086833 src/sage/features/fricas.py  # FileNotFoundError in doctesting framework

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 1, 2023

The documentation is being added in this PR, no?

src/sage/features/fricas.py Outdated Show resolved Hide resolved
@dimpase
Copy link
Member Author

dimpase commented Jul 2, 2023

Do we need _install_hints for FriCAS ? It's not like the other packages for which it's provided, one can install it from Sage.

@dimpase
Copy link
Member Author

dimpase commented Jul 2, 2023

If FriCAS is not available (I just remove the executable), I get doctest errors like

File "src/sage/interfaces/fricas.py", line 899, in sage.interfaces.fricas.FriCAS.__reduce__
Failed example:
    FriCAS().__reduce__()
Exception raised:
    Traceback (most recent call last):
      File "/mnt/opt/Sage/sage-dev/src/sage/doctest/forker.py", line 696, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/opt/Sage/sage-dev/src/sage/doctest/forker.py", line 1106, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.interfaces.fricas.FriCAS.__reduce__[0]>", line 1, in <module>
        FriCAS().__reduce__()
        ^^^^^^^^
      File "sage/misc/lazy_import.pyx", line 404, in sage.misc.lazy_import.LazyImport.__call__
        sage: is_prime(12) == my_isprime(12)
      File "sage/misc/lazy_import.pyx", line 225, in sage.misc.lazy_import.LazyImport.get_object
        return self._get_object()
      File "sage/misc/lazy_import.pyx", line 261, in sage.misc.lazy_import.LazyImport._get_object
        try:
      File "/mnt/opt/Sage/sage-dev/src/sage/interfaces/fricas.py", line 2134, in <module>
        fricas = FriCAS()
                 ^^^^^^^^
      File "/mnt/opt/Sage/sage-dev/src/sage/interfaces/fricas.py", line 293, in __init__
        FriCAS().require()
      File "/mnt/opt/Sage/sage-dev/src/sage/features/__init__.py", line 243, in require
        raise FeatureNotPresentError(self, presence.reason, presence.resolution)
    sage.features.FeatureNotPresentError: fricas is not available.
    Executable 'fricas' not found on PATH.
    To install fricas using the gentoo package manager, you can try to run:
    !sudo emerge sci-mathematics/fricas
    To install fricas using the Sage package manager, you can try to run:
      !sage -i fricas
    No equivalent system packages for pip are known to Sage.
    Further installation instructions might be available at https://fricas.github.io.

no idea what's wrong. Could it be due to ./configure --enable-fricas=yes ?

The branch seems to work (modulo a number of maths doctest errors) if FriCAS is available.

Copy link

Documentation preview for this PR (built with commit 870fa22; changes) is ready! 🎉

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 22, 2023

Usually not a good idea to do a rebase that includes the dependency PR.

@dimpase
Copy link
Member Author

dimpase commented Nov 22, 2023

All good, it seems.
Tested with system fricas built with sbcl, as well as fricas built with ECL (both on Gentoo - @orlitzky )
Also tests fine without fricas.

I can't officially review this, so, please...

@dimpase
Copy link
Member Author

dimpase commented Nov 25, 2023

@mantepse - this allows using system-wide FriCAS.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
    
As FriCAS is only used as an executable, this is straightforward; quite
a number of systems has Fricas 1.3.8, so this is useful, too.

The FriCAS pexpect interface now uses the new `Executable` feature to
determine the absolute pathname of the fricas executable (unless
executed remotely).
This is made possible by a simple refactor of the
`sage.interfaces.expect.Expect` class: Computing the effective command
line is no longer done in `set_server_and_command` (called by
`__init__`); it is delayed until an interface is started and needs the
command line.

- Fixes sagemath#35837
- Fixes sagemath#33575

Dependencies: The changes in `sage.interfaces` outside of `.expect` and
`.fricas` are all from the following PR and do not need review.

- Depends on sagemath#36656 (merged here)
    
URL: sagemath#35838
Reported by: Dima Pasechnik
Reviewer(s): Dima Pasechnik, François Bissey, Matthias Köppe
@vbraun vbraun merged commit 9c4fc77 into sagemath:develop Dec 6, 2023
34 of 51 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 6, 2023
@dimpase dimpase deleted the fricas_spkg_config branch December 12, 2023 11:24
@dimpase
Copy link
Member Author

dimpase commented Jan 9, 2024

fricas 1.3.10 is meanwhile out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

accept fricas from the system Feature for fricas
4 participants