Update FriCAS to 1.2.7 (current)

[dimpase]

subject says it all, see http://fricas.sourceforge.net/

The current version in Sage, 1.2.4, does no longer build with our version of ECL. But 1.2.7 still has the same problem, so needs to get patched for it, replacing uffi: by ffi: in src/lisp/fricas-lisp.lisp.

---

Upstream source tarball:
https://sourceforge.net/projects/fricas/files/fricas/1.2.7/fricas-1.2.7-full.tar.bz2

(with ads, but automatic download after a few seconds)

Upstream: Fixed upstream, in a later stable release.

CC: @dkrenn @mantepse @hemmecke

Component: packages: experimental

Keywords: uffi ECL

Author: Dima Pasechnik

Branch: 6d91694

Reviewer: Martin Rubey, Leif Leonhardy

Issue created by migration from https://trac.sagemath.org/ticket/21209

[mantepse]

comment:4

I am currently rebuilding sage-develop...

Do I understand correctly that I should rename fricas-1.2.7-full.tar.bz2 to fricas-1.2.7.tar.bz2 before putting it into the upstream directory?

[nexttime]

comment:5

Replying to @mantepse:

Do I understand correctly that I should rename fricas-1.2.7-full.tar.bz2 to fricas-1.2.7.tar.bz2 before putting it into the upstream directory?

Would make sense.

Not sure if we can/should strip (=repackage) the tarball anyway; the description at least sounds as if there were things included we don't need for Sage / to ship. But it isn't huge either.

If you do, you could create an spkg-src script, or at least document what you did in "Special !Update/Build Instructions" in SPKG.txt.

[nexttime]

comment:6

More precisely, fricas-VERSION-full.tar.bz2 in checksums.ini is supposed to work as well.

[mantepse]

comment:7

Replying to @nexttime:

Replying to @mantepse:

Do I understand correctly that I should rename fricas-1.2.7-full.tar.bz2 to fricas-1.2.7.tar.bz2 before putting it into the upstream directory?

Would make sense.

Not sure if we can/should strip (=repackage) the tarball anyway; the description at least sounds as if there were things included we don't need for Sage / to ship. But it isn't huge either.

As far as I know: no, the things included in the "full" tarball are precisely those needed.

Unfortunately, build with ecl fails for the same reason as reported.

[mantepse]

comment:8

From Dima via sage-devel:

ECL does not use uffi: prefix any more, it's ffi: instead, according to their docs.
Replacing all the uffi: with ffi: in src/lisp/fricas-lisp.lisp makes compiler happy.

Currently running the build, takes a while...

[nexttime]

comment:9

Dima said the following works:

diff -Naur fricas-1.2.7.orig/src/lisp/fricas-lisp.lisp fricas-1.2.7/src/lisp/fricas-lisp.lisp
--- fricas-1.2.7.orig/src/lisp/fricas-lisp.lisp	2015-09-25 02:06:15.000000000 +0200
+++ fricas-1.2.7/src/lisp/fricas-lisp.lisp	2016-08-11 21:57:25.840863287 +0200
@@ -556,10 +556,10 @@
                     (dolist (el strs)
                         (setf wrapper `(FFI:WITH-CSTRING ,el ,wrapper)))
                     (setf wrapper `(defun ,name ,largs ,wrapper))
-                    `(progn (uffi:def-function (,c-name ,sym)
+                    `(progn (ffi:def-function (,c-name ,sym)
                                 ,fargs :returning ,l-ret)
                             ,wrapper))
-                `(uffi:def-function (,c-name ,name)
+                `(ffi:def-function (,c-name ,name)
                      ,fargs :returning ,l-ret)))))
 
 (defmacro fricas-foreign-call (name c-name return-type &rest arguments)
@@ -788,14 +788,14 @@
 #+:ecl
 (progn
 
-(uffi:def-function ("sock_get_string_buf" sock_get_string_buf_wrapper)
+(ffi:def-function ("sock_get_string_buf" sock_get_string_buf_wrapper)
                    ((purpose :int) (buf (:array :unsigned-char 10000)) (len :int))
                    :returning :void)
 
 (defun |sockGetStringFrom| (purpose)
-    (uffi:with-foreign-object (buf '(:array :unsigned-char 10000))
+    (ffi:with-foreign-object (buf '(:array :unsigned-char 10000))
         (sock_get_string_buf_wrapper purpose buf 10000)
-        (uffi:convert-from-foreign-string buf)))
+        (ffi:convert-from-foreign-string buf)))
 
 )
 
@@ -825,7 +825,7 @@
   (LISP::defentry |makedir| (LISP::string)         (LISP::int "makedir")))
 
 #+:ecl
-(uffi:def-function ("directoryp" raw_file_kind)
+(ffi:def-function ("directoryp" raw_file_kind)
                    ((arg :cstring))
                    :returning :int)
 #+:ecl
@@ -834,7 +834,7 @@
            (raw_file_kind cname)))
 
 #+:ecl
-(uffi:def-function ("makedir" raw_makedir)
+(ffi:def-function ("makedir" raw_makedir)
                    ((arg :cstring))
                    :returning :int)
 

(sed -i 's/uffi:/ffi:/' fricas-1.2.7/src/lisp/fricas-lisp.lisp)

[mantepse]

comment:11

I can confirm this, the build just finished. I am now looking for tests.

[dimpase]

Upstream: Not yet reported upstream; Will do shortly.

[dimpase]

Commit: 68bbc30

[dimpase]

Branch: u/dimpase/fricas127

[dimpase]

New commits:

68bbc30

updated version and added patch

[dimpase]

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. Developers acknowledge bug.

[dimpase]

comment:13

Here is a discussion on FriCAS list.
I take it that they see this as a bug. Perhaps there is a more intelligent way to fix this, more CL-y one...

[dimpase]

Author: Dima Pasechnik

[mantepse]

comment:14

As far as I can see fix is CL-y, because it is fixing precisely the ecl-dependend functions...

I am still looking into the tests, but I suppose fixing them belongs to a different ticket.

Can I give this positive review? (It works on my computer...)

[nexttime]

comment:15

SPKG.txt should get fixed and updated (as the patch has no header)... ;-)

I'd also add [ -r "$patch" ] && ... as usual.

Is there a test suite?

Does make install install the docs as well? Do we otherwise want to install them, and if so, how, where?

[nexttime]

comment:16

P.S.: I'd also say fixing (now?) broken doctests belongs to updating the package.

[dimpase]

comment:17

Replying to @nexttime:

P.S.: I'd also say fixing (now?) broken doctests belongs to updating the package.

Needless to say, all the 44 tests from src/sage/interfaces/fricas.py pass for me :-)

[nexttime]

comment:18

Replying to @dimpase:

Replying to @nexttime:

P.S.: I'd also say fixing (now?) broken doctests belongs to updating the package.

Needless to say, all the 44 tests from src/sage/interfaces/fricas.py pass for me :-)

There are more elsewhere.

(I assume you did specify --optional=fricas... ;-) )

[mantepse]

comment:19

There is one trivial fix in axiom.py (line 686 should not be "\leqno(11)" but rather "..."), but the conversion of even basic fricas types to sage types is broken.

Can anybody tell me where the method variables comes from (is implemented):

sage: a = fricas(1/2)
sage: a.variables()
[]

?

[mantepse]

comment:20

Just found out, it's provided by FriCAS. But it's use is very likely abuse.

[sagetrac-git]

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

2ee5bbf

doctests fixes, except fricas(1/2).sage() in axiom.py

[nexttime]

comment:46

Replying to @dimpase:

Replying to @nexttime:

I won't comment on the 5043 style warnings... XD

Style 5043? No,please don't, indeed...

Well, unfortunately due to popular demand this product is currently out of stock.

[vbraun]

Changed branch from u/dimpase/fricas127 to 6d91694

[nexttime]

comment:48

Looks like I'm having more than a dozen AXIOMsys orphans from (testing) this...

[nexttime]

Changed commit from 6d91694 to none

[mantepse]

comment:49

Replying to @nexttime:

Looks like I'm having more than a dozen AXIOMsys orphans from (testing) this...

Could you please expand? Is there a problem? If so, did you test with the new interface #21231 , or the old one?

[nexttime]

comment:50

Replying to @mantepse:

Replying to @nexttime:

Looks like I'm having more than a dozen AXIOMsys orphans from (testing) this...

Could you please expand? Is there a problem? If so, did you test with the new interface #21231 , or the old one?

Just with the old interface, hence I reported it here. (There were 27 orphans from perhaps testing src/sage/interfaces/* once or twice and one ptestlong I think. I only incidentally noticed them after weeks because they weren't very active either.)

So when reviewing #21231, one should look whether there's still a problem.

[mantepse]

comment:51

Replying to @nexttime:

Replying to @mantepse:

Replying to @nexttime:

Looks like I'm having more than a dozen AXIOMsys orphans from (testing) this...

Could you please expand? Is there a problem? If so, did you test with the new interface #21231 , or the old one?

Just with the old interface, hence I reported it here. (There were 27 orphans from perhaps testing src/sage/interfaces/* once or twice and one ptestlong I think. I only incidentally noticed them after weeks because they weren't very active either.)

So when reviewing #21231, one should look whether there's still a problem.

Could you provide a testcase, or some instructions how to see the orphans? That would make things much easier for me - because I have no idea how to do this...

[dimpase]

comment:52

To see if there are orphans on Linux, look at the output of

$ ps -def | grep fricas

I see a couple of these just after one run of

./sage -tp --optional=fricas,sage src/sage/interfaces/fricas.py 

[nexttime]

comment:53

Replying to @mantepse:

Replying to @nexttime:

Replying to @mantepse:

Replying to @nexttime:

Looks like I'm having more than a dozen AXIOMsys orphans from (testing) this...

Could you please expand? Is there a problem? If so, did you test with the new interface #21231 , or the old one?

Just with the old interface, hence I reported it here. (There were 27 orphans from perhaps testing src/sage/interfaces/* once or twice and one ptestlong I think. I only incidentally noticed them after weeks because they weren't very active either.)

So when reviewing #21231, one should look whether there's still a problem.

Could you provide a testcase, or some instructions how to see the orphans? That would make things much easier for me - because I have no idea how to do this...

Ah, sorry, forgot to answer.

You can also use e.g. ps x | grep AXIOMsys | grep -v grep.

(And kill $(ps x | grep AXIOMsys | grep -v grep | awk '{ print $1 }') ;-) )

[nexttime]

comment:54

Note that you should look a while after you've left Sage, as normally the Sage Cleaner should kill them (but that only works if the interfaces are properly written / their PIDs properly recorded).

[mantepse]

comment:55

Thank you. Indeed, there is a bug. However, I don't know yet how to fix it, I thought that fricas.quit() would send fricas._quit_string() to fricas, but apparently it doesn't. So, what should I do? (I couldn't find anything but _quit_string in other interfaces...)

[dimpase]

comment:56

Replying to @mantepse:

Thank you. Indeed, there is a bug. However, I don't know yet how to fix it, I thought that fricas.quit() would send fricas._quit_string() to fricas, but apparently it doesn't. So, what should I do? (I couldn't find anything but _quit_string in other interfaces...)

I think the problem is that more child processes of FriCAS are launched than sage-cleaner (a script that cleans up after Sage ends) is able to deal with.
In fact, you see that

sage: fricas._quit_string()
')lisp (quit)'

which is as it should be, as FriCAS inherits from PanAxiom class (in sage/interfaces/axiom.py), where _quit_string is defined.
And you see that fricas.quit() does kill the instance corresponding to fricas.pid(), as it should, by looking at the output of ps -def....

Here is how it goes: I start Sage and execute fricas.pid() at the prompt; I see that this results in launching of a bunch of things:

dima     30739 30652  0 10:21 pts/4    00:00:00 /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/sman -nox -noclef -ws /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys
dima     30745 30739  1 10:21 pts/5    00:00:00 /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys -- /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/
dima     30746 30739  0 10:21 pts/4    00:00:00 /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/lib/session
dima     30747 30739  0 10:21 pts/4    00:00:00 /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/lib/spadclient
dima     30748 30739  0 10:21 pts/4    00:00:00 /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/sman -nox -noclef -ws /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys

of which the 1st one corresponds to PID reported by fricas.pid().

If I issue fricas.quit() and then quit Sage then no zombies live on. But if I just quit Sage then I get a zombie, one of these

/home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys -- /home/dima/Sage/sage/local/lib/fricas/target/x86_64-unknown-linux/bin/

Probably, the information on these extra processes' PIDs has to be registered somewhere for sage-cleaner to succeed.

[mantepse]

comment:57

Are you sure that _quit_string is used at all? Because, if it is sent to fricas, then fricas should quit (and it does so on my machine).

Simply killing a few processes is error prone, I'd say.

[dimpase]

comment:58

_quit_string is used when you explicitly call fricas.quit(). But it is not used by sage-cleaner, AFAICS (although this is fixable, I guess). sage-cleaner would kill all sub-processed spawned by a process launched by a process with the PID it knows. But it seems that that zombie suprocesses of FriCAS don't have parent's PID equal to FriCAS, they are parented by the same PID as the main FriCAS process, if I read this correctly from the output of ps -def.

Do you know how FriCAS launches these (sub?)processes?

[nexttime]

comment:59

The purpose of the Sage Cleaner is not to shut down pexpect interfaces, that is supposed to be done by Sage when exiting.

The Sage Cleaner is just there to afterwards (after a while) kill spawned processes if something went wrong. And he doesn't know child processes spawned by these foreign ones, they have to belong to the same process group to make killing them work.

[mantepse]

comment:60

Replying to @dimpase:

_quit_string' is used when you explicitly call fricas.quit()`.

I cannot see this in the code. However, I do see it when tracing. Possibly a newline is missing?

Do you know how FriCAS launches these (sub?)processes?

No.

[dimpase]

comment:61

Replying to @nexttime:

The purpose of the Sage Cleaner is not to shut down pexpect interfaces, that is supposed to be done by Sage when exiting.

The Sage Cleaner is just there to afterwards (after a while) kill spawned processes if something went wrong. And he doesn't know child processes spawned by these foreign ones, they have to belong to the same process group to make killing them work.

OK, what happens is more subtle: if fricas.quit() is not invoked before exiting Sage, then zombie arises with parent's ID of systemd. Before exit, however, the (pre)zombie parent's ID is good, it's the same as shown by fricas.pid().

IMHO this means that sage-cleaner fails to kill it; when it did get fricas.quit() it flips to a ready to die state, but not otherwise.

[mantepse]

comment:62

I just noticed that fricas.quit() does work (at least with the new interface). However, it takes a very long time:

sage: def doit():
....:     F = fricas(1+1).parent(); a = time.time(); F.quit()
....:     result = ["testing"]
....:     while result != []:
....:         result = !ps x | grep AXIOMsys | grep -v grep
....:         sleep(1)
....:     print time.time() - a
....:     
sage: doit()
16.3083839417

It shouldn't. If I use fricas._expect.write(")quit\r"), fricas quits immediately. So, indeed, "\r" is missing. Should I simply modify the quitstring?

[dimpase]

comment:63

Replying to @mantepse:

I just noticed that fricas.quit() does work (at least with the new interface). However, it takes a very long time:

sage: def doit():
....:     F = fricas(1+1).parent(); a = time.time(); F.quit()
....:     result = ["testing"]
....:     while result != []:
....:         result = !ps x | grep AXIOMsys | grep -v grep
....:         sleep(1)
....:     print time.time() - a
....:     
sage: doit()
16.3083839417

It shouldn't. If I use fricas._expect.write(")quit\r"), fricas quits immediately. So, indeed, "\r" is missing. Should I simply modify the quitstring?

well, IMHO if \r was missing in _quit_string then fricas.quit() would not be working at all... (well, perhaps it is slow as it is because it invokes lisp.)

Here your command has no effect:

sage: fricas.pid()
6130
sage: fricas._expect.write(")quit\r")
sage: fricas.pid()
6130
sage: fricas.quit()
sage: fricas.pid() # right, now we get a new instance!
6507

Please check that you don't see the same picture with your new interface.

[mantepse]

comment:64

On my computer, fricas.pid() cannot be used to see whether fricas is running:

sage: fricas.pid()
10923
sage: !ps x | grep AXIOMsys | grep -v grep
10923 pts/3    Ss+    0:00 /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/sman -nox -noclef -ws /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys
10929 pts/4    Ss+    0:00 /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys -- /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/
10932 pts/3    S+     0:00 /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/sman -nox -noclef -ws /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys
sage: fricas._expect.write(")quit\r")
sage: !ps x | grep AXIOMsys | grep -v grep
sage: fricas.pid()
10923

[dimpase]

comment:65

Replying to @mantepse:

On my computer, fricas.pid() cannot be used to see whether fricas is running:

sage: fricas.pid()
10923
sage: !ps x | grep AXIOMsys | grep -v grep
10923 pts/3    Ss+    0:00 /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/sman -nox -noclef -ws /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys
10929 pts/4    Ss+    0:00 /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys -- /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/
10932 pts/3    S+     0:00 /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/sman -nox -noclef -ws /usr/local/lib/fricas/target/x86_64-unknown-linux/bin/AXIOMsys
sage: fricas._expect.write(")quit\r")
sage: !ps x | grep AXIOMsys | grep -v grep
sage: fricas.pid()
10923

no, what you see is that fricas is running, with pid 10923, and that fricas._expect.write(")quit\r") is ignored by it. If you implemented _quit_string properly you would also be able to stop it as is possible with the old interface.

[mantepse]

comment:66

Dima, I don't understand your reasoning. I would think that the first line below quits the fricas process on my machine, as the output of the second line shows.

sage: fricas._expect.write(")quit\r")
sage: !ps x | grep AXIOMsys | grep -v grep
sage: fricas.pid()
10923

The third line is in accordance with the doc of .pid():

   Return the PID of the underlying sub-process.

   REMARK:

   If the interface terminates unexpectedly, the original PID will
   still be used. But if it was terminated using "quit()", a new sub-
   process with a new PID is automatically started.

   EXAMPLE:

      sage: pid = gap.pid()
      sage: gap.eval('quit;')
      ''
      sage: pid == gap.pid()
      True
      sage: gap.quit()
      sage: pid == gap.pid()
      False

So, in my opinion a possible fix is to append a \r to the quitstring - and of course, this should work with the old interface, too, although I did not test.

No idea though why fricas needs it, and, for example, gap, doesn't.

[dimpase]

comment:67

Sorry, indeed, I didn't quite parse what you wrote. Anyhow, as soon as your new interface is ready, I'd be glad to have a look at it. Are there any git-related issues you need to fix?

[mantepse]

comment:68

Replying to @dimpase:

Sorry, indeed, I didn't quite parse what you wrote. Anyhow, as soon as your new interface is ready, I'd be glad to have a look at it.

Thank you!

Are there any git-related issues you need to fix?

Indeed - I made a mess with the commits and found no time to correct them. It would be of great help if you could, for example, create a "clean" branch from my branch, deleting all the commits which obviously don't belong (I made a list at the ticket).

I think the only work that remains to be done there is to add that "\r" to the quitstring, add a test to the quit() method, and probably some others to make the patchbot happy.

If the following an acceptable test for quit()?

sage: p = fricas.pid();
sage: l = !ps x | grep AXIOMsys | grep -v grep
sage: any(s.strip().startswith(str(p)) for s in l)
True
sage: fricas.quit()
sage: any(s.strip().startswith(str(p)) for s in l)
False

[nexttime]

comment:69

Replying to @mantepse:

Is the following an acceptable test for quit()?

sage: p = fricas.pid();
sage: l = !ps x | grep AXIOMsys | grep -v grep
sage: any(s.strip().startswith(str(p)) for s in l)
True
sage: fricas.quit()
sage: any(s.strip().startswith(str(p)) for s in l)
False

Unfortunately no. Neither ps, nor x, nor the output format are really portable I think^TM. So you'd presumably have to use some Python stuff instead. Not to mention you shouldn't use ! (but os.system() or one of its replacements instead).

Rather orthogonal to that, you must not use .startswith(), but rather .split()[0]==str(p).

[nexttime]

comment:70

Jeroen just suggested on sage-devel to make the Python package psutil standard.

You could look into that for retrieving process information in a portable way.

[mantepse]

comment:71

I just managed to install psutil, and found out:

sage: import psutil
sage: p = fricas.pid(); p
22184
sage: pr = psutil.Process(p); pr
<psutil.Process(pid=22184, name='sman') at 140240479360400>
sage: pr.children()
[<psutil.Process(pid=22190, name='AXIOMsys') at 140240479356752>,
 <psutil.Process(pid=22191, name='session') at 140240479356816>,
 <psutil.Process(pid=22192, name='spadclient') at 140240479356880>,
 <psutil.Process(pid=22193, name='sman') at 140240479356944>]
sage: fricas._expect.write(")quit\r")
sage: pr.children()
[]
sage: pr
<psutil.Process(pid=22184, name='sman') at 140240479360400>

This remaining job is killed when exiting sage. However:

sage: p = gap.pid(); p
22582
sage: pr = psutil.Process(p); pr
<psutil.Process(pid=22582, name='gap') at 140240479291088>
sage: pr.children()
[]
sage: gap.quit()
sage: pr
<psutil.Process(pid=22582 (terminated)) at 140240479291088>

So it's better, but not perfect. Is it good enough?