spkg-configure.m4 for gap

[thierry-FreeBSD]

This ticket adds an spkg-configure.m4 for gap, in order to use it from a system package if possible (see #27330).

With this, the installed gap is well detected, but for sagelib and gap packages to built, several paths must be modified:

• GAP_ROOT_DIR in src/sage/env.py

• GAP_ROOT in build/pkgs/gap_packages/spkg-install.in

• gap directory in src/sage/libs/gap/util.pyx

Moreover, the dependency on SAGE_LOCAL needs to be removed. Without it, one currently gets the following error

      File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/interfaces/expect.py", line 520, in _start
        raise RuntimeError("unable to start %s: %s" % (self.name(), msg))
    RuntimeError: unable to start gap: End Of File (EOF). Exception style platform.
    Gap finished running /__w/sagetrac-mirror/sagetrac-mirror/.tox/local-sudo-standard/local/bin/gap -r -b -p -T -E -o 401m -s 401m -m 64m /__w/sagetrac-mirror/sagetrac-mirror/src/sage/ext_data/gap/sage.g
    command: /__w/sagetrac-mirror/sagetrac-mirror/.tox/local-sudo-standard/local/bin/gap
    args: ['/__w/sagetrac-mirror/sagetrac-mirror/.tox/local-sudo-standard/local/bin/gap', '-r', '-b', '-p', '-T', '-E', '-o', '401m', '-s', '401m', '-m', '64m', '/__w/sagetrac-mirror/sagetrac-mirror/src/sage/ext_data/gap/sage.g']
    buffer (last 100 chars): b''
    before (last 100 chars): b'Set the environment variable SAGE_LOCAL.\r\n'
    after: <class 'pexpect.exceptions.EOF'>
    match: None
    match_index: None
    exitstatus: 1
    flag_eof: True
    pid: 3124
    child_fd: 26
    closed: False
    timeout: None
    delimiter: <class 'pexpect.exceptions.EOF'>
    logfile: None
    logfile_read: None
    logfile_send: None
    maxread: 4194304
    ignorecase: False
    searchwindowsize: None
    delaybeforesend: None
    delayafterclose: 0.1
    delayafterterminate: 0.1
    searcher: searcher_re:
        0: re.compile(b'gap> ')

CC: @dimpase @orlitzky @slel @isuruf @antonio-rojas

Component: build: configure

Keywords: gap, system packages

Author: Thierry Thomas, Samuel Lelièvre

Branch/Commit: public/29644 @ 2a41f49

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

[thierry-FreeBSD]

Attachment: spkg-configure.m4.gz

spkg-configure.m4 to be copied under build/pkgs/gap/

[mkoeppe]

comment:1

system package information also needs to be added

[mkoeppe]

Changed author from gh-thierry-FreeBSD to Thierry Thomas

[mkoeppe]

comment:3

Which systems have a usable gap?

[antonio-rojas]

comment:4

I'm using it in the distro sage package with no issues, with the patch from #29314. The only potential problem I'm aware of is that our gap-packages contains all packages shipped in the upstream tarball - this can cause high memory usage if it's installed.

[dimpase]

comment:5

Replying to @antonio-rojas:

I'm using it in the distro sage package with no issues, with the patch from #29314. The only potential problem I'm aware of is that our gap-packages contains all packages shipped in the upstream tarball - this can cause high memory usage if it's installed.

I don't think having all the GAP packages used causes any memory problems - the majority are not loaded by GAP by default (and even if they are, their memory footprint is very small).

[slel]

comment:7

Thierry, can you push a branch here?
Or would you prefer someone else to commit for you?

The file spkg-configure.m4 you attached can be
committed with you as the author using:

$ git commit --author="Your Name <[email protected]>"

with name and email found from your previous contributions using:

$ git log | grep "Author: Thierry Thomas"

[slel]

Changed author from Thierry Thomas to Thierry Thomas, Samuel Lelièvre

[slel]

Commit: 2a41f49

[slel]

comment:8

Not sure how to address any of the tasks in the ticket description.

But here is Thierry's spkg-configure.m4 and some distro info.

---

New commits:

2c98135	29644: Add spkg-configure.m4 for gap
2a41f49	29644: Add distro information for gap

[slel]

Branch: public/29644

[antonio-rojas]

comment:10

Replying to @dimpase:

I don't think having all the GAP packages used causes any memory problems - the majority are not loaded by GAP by default (and even if they are, their memory footprint is very small).

They are not loaded by GAP, but they are by Sage and it does cause problems - see #31761

[tornaria]

comment:13

We could get the gap root from

$ gap -q --nointeract --bare -r -c 'Display(KERNEL_INFO().GAP_ROOT_PATHS[1]);'
/usr/share/gap/

The code in sage.libs.gap.util.gap_root() tries to read the gap shell script to figure out the gap root but that doesn't work with the gap shell script shipped by gap. Moreover, it's only used when sage.env.GAP_ROOT_DIR is undefined, but this is hardcoded in src/sage/env.py to be SAGE_SHARE/gap.

Maybe:

a. build/pkgs/gap/spkg-configure.m4: set GAP_ROOT using my suggestion above (while we are at it, check that running gap works, which is required by sage and I'm not sure is checked right now).
b. maybe check that the gap root is ok e.g. by looking for the file ${GAP_ROOT}/sysinfo.gap.
c. if gap will not be used from system, unset GAP_ROOT
d. AC_SUBST(GAP_ROOT)
e. in pkgs/sage-conf/sage_conf.py.in add a line like

GAP_ROOT_DIR = "@GAP_ROOT@" or None

I'm not completely sure how to do (a) and (b) but it's probably easy for someone who understands autoconf.

Note that when not using system gap, GAP_ROOT_DIR should be set to None so that the fallback defined in src/sage/env.py takes precedence ("" is not good enough). Alternatively, when gap will not be used from system, set GAP_ROOT=\$SAGE_LOCAL/share/gap and then have sage_conf.py replace $SAGE_LOCAL as is done for other variables (and please remove the fallback from src/sage/env.py, there's too many different places where variables are set and each package seems to have a different way to do it).

[dimpase]

comment:14

Replying to @tornaria:

We could get the gap root from

$ gap -q --nointeract --bare -r -c 'Display(KERNEL_INFO().GAP_ROOT_PATHS[1]);'
/usr/share/gap/

Doesn't work for me on Debian 11:

dimpase@penguin:~/tmp$ gap -q --nointeract --bare -r -c 'Display(KERNEL_INFO().GAP_ROOT_PATHS[1]);'
/home/dimpase/gap/
dimpase@penguin:~/tmp$ ls /home/dimpase/gap
ls: cannot access '/home/dimpase/gap': No such file or directory
dimpase@penguin:~/tmp$ which gap
/usr/bin/gap
dimpase@penguin:~/tmp$ gap
 ┌───────┐   GAP 4.11.0 of 29-Feb-2020
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64-kv7
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loading the library and packages ...
 Packages:   Alnuth 3.1.2, AtlasRep 2.1.0, AutPGrp 1.10.2, CTblLib 1.3.1, 
             FactInt 1.6.3, GAPDoc 1.6.3, IO 4.7.0, Polycyclic 2.15.1, PrimGrp 3.4.0, 
             SmallGrp 1.4.1, TomLib 1.2.9, TransGrp 2.0.6
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> 

[dimpase]

comment:15

In case, here is my GAP_ROOT_PATHS:

  GAP_ROOT_PATHS := [ "/home/dimpase/.gap/", "/home/dimpase/gap/", 
      "/usr/local/lib/gap/", "/usr/local/share/gap/", "/usr/lib/gap/", 
      "/usr/share/gap/" ]

[tornaria]

comment:16

What are GAPInfo.UserGapRoot and KERNEL_INFO().GAP_ROOT_PATHS with and without -r?

Here I get:

$ gap --norepl --bare -b -c 'Display(GAPInfo.UserGapRoot); Display(KERNEL_INFO().GAP_ROOT_PATHS);'
/home/tornaria/.gap
[ "/home/tornaria/.gap/", "/usr/share/gap/" ]
$ gap -r --norepl --bare -b -c 'Display(GAPInfo.UserGapRoot); Display(KERNEL_INFO().GAP_ROOT_PATHS);'
/home/tornaria/.gap
[ "/usr/share/gap/" ]

Using -r prevents UserGapRoot to be prepended to GAP_ROOT_PATHS. It seems your GAP_ROOT_PATHS already has a user directory and other locations.

Which one is the "real" gap root? I think it should be either the one containing sysinfo.gap or else the one containing lib/init.g.

Alternatively, use the whole list of GAP_ROOT_PATHS, so that sage passes all of them to gap when initializing the library. This would need to rework sage.libs.gap.util.gap_root() so it accepts multiple directories (and it's not necessary for all directories to exist, it suffices that at least one contains lib/init.g).

To get the complete list for GAP_ROOT_PATHS we can use this:

$ gap -q --nointeract --bare -r -c 'Display(JoinStringsWithSeparator(KERNEL_INFO().GAP_ROOT_PATHS,";"));'
/usr/share/gap/

For me it gives the same since I only have one path in the system gap root, but for you it should give all of them.

[tornaria]

comment:17

Something like that seems to work as a fallback in case GAP_ROOT_DIR is not set:

--- sage-9.5.rc0/src/sage/libs/gap/util.pyx.orig	2022-01-09 07:49:26.000000000 -0300
+++ sage-9.5.rc0/src/sage/libs/gap/util.pyx	2022-01-13 01:57:11.072897453 -0300
@@ -178,6 +178,11 @@
     if os.path.exists(sage.env.GAP_ROOT_DIR):
         return sage.env.GAP_ROOT_DIR
 
+    from sage.interfaces.gap import gap
+    for gapdir in gap("KERNEL_INFO().GAP_ROOT_PATHS").sage():
+        if os.path.exists(os.path.join(gapdir, 'sysinfo.gap')):
+            return gapdir
+
     # Attempt to figure out the appropriate GAP_ROOT by reading the
     # local/bin/gap shell script; this is an ugly hack that exists for
     # historical reasons; the best approach to setting where Sage looks for

The drawback is that this will launch and keep running the gap interpreter. Maybe better to do a one-time run of gap just for this purpose (or run it at configure time as I proposed above).

[tornaria]

comment:18

Maybe this is better:

--- a/src/sage/libs/gap/util.pyx
+++ b/src/sage/libs/gap/util.pyx
@@ -23,6 +23,7 @@ from cysignals.signals cimport sig_on, sig_off
 import os
 import warnings
 import sage.env
+import subprocess
 
 from .gap_includes cimport *
 from .element cimport *
@@ -178,6 +179,16 @@ def gap_root():
     if os.path.exists(sage.env.GAP_ROOT_DIR):
         return sage.env.GAP_ROOT_DIR
 
+    gap_expr = 'JoinStringsWithSeparator(KERNEL_INFO().GAP_ROOT_PATHS,";")'
+    gap_root_paths = subprocess.getoutput(
+        f"gap -r -q --bare --nointeract -c 'Display({gap_expr});'"
+        ).strip().split(';')
+
+    for gapdir in gap_root_paths:
+        if os.path.exists(os.path.join(gapdir, 'sysinfo.gap')):
+            sage.env.GAP_ROOT_DIR = gapdir
+            return gapdir
+
     # Attempt to figure out the appropriate GAP_ROOT by reading the
     # local/bin/gap shell script; this is an ugly hack that exists for
     # historical reasons; the best approach to setting where Sage looks for

Also, sage.env._get_shared_lib_path() is broken in the sense that it looks for libgap.so but it should really look for libgap.so*. Indeed the soname for the library is libgap.so.0 and the symlink libgap.so -> libgap.so.0 is only provided by gap-devel package (which is installed when I build sage, but not necessarily when I run sage).

Simple fix:

--- a/src/sage/env.py
+++ b/src/sage/env.py
@@ -293,7 +293,7 @@ def _get_shared_lib_path(*libnames: str) -> Optional[str]:
             if sys.platform == 'darwin':
                 ext = 'dylib'
             else:
-                ext = 'so'
+                ext = 'so*'
 
             if SAGE_LOCAL:
                 search_directories.append(Path(SAGE_LOCAL) / 'lib')

OTOH, there's sage.misc.compat.find_library, maybe that could be used instead.

Finally, there's a test in src/sage_setup/optional_extension.py that fails when gap is installed from system so

--- a/src/sage_setup/optional_extension.py
+++ b/src/sage_setup/optional_extension.py
@@ -80,7 +80,7 @@ def OptionalExtension(*args, **kwds):
         sage: print(ext.__class__.__name__)
         CythonizeExtension
         sage: ext = OptionalExtension("foo", ["foo.c"], package="gap")
-        sage: print(ext.__class__.__name__)
+        sage: print(ext.__class__.__name__)     # not tested - fails with system gap
         Extension
     """
     try:

I also changed GAP_ROOT_DIR for gap_root() in
src/sage/libs/gap/saved_workspace.py:

--- a/src/sage/libs/gap/saved_workspace.py
+++ b/src/sage/libs/gap/saved_workspace.py
@@ -8,7 +8,7 @@ workspaces.

 import os
 import glob
-from sage.env import GAP_ROOT_DIR
+from sage.libs.gap.util import gap_root
 from sage.interfaces.gap_workspace import gap_workspace_file


@@ -31,7 +31,7 @@ def timestamp():
     """
     libgap_dir = os.path.dirname(__file__)
     libgap_files = glob.glob(os.path.join(libgap_dir, '*'))
-    gap_packages = glob.glob(os.path.join(GAP_ROOT_DIR, 'pkg', '*'))
+    gap_packages = glob.glob(os.path.join(gap_root(), 'pkg', '*'))
     files = libgap_files + gap_packages
     if len(files) == 0:
         print('Unable to find LibGAP files.')

[dimpase]

comment:19

How about asking GAP people (or doing a PR) to provide an appropriate feature? Which one does really matter? The one with sysinfo.gap ?

Did you look at how this is done in https://github.com/embray/gappy ?

[tornaria]

comment:20

Replying to @dimpase:

How about asking GAP people (or doing a PR) to provide an appropriate feature? Which one does really matter? The one with sysinfo.gap ?

Did you look at how this is done in https://github.com/embray/gappy ?

TL;DR: if the shared library for gap is in /PATH/TO/lib/, then try GAP_ROOT=/PATH/TO/share/gap. Decide the path is correct if a file $GAP_ROOT/lib/init.g exists.

This seems ok to me, in principle the path to the gap library is in GAP_SO.

[tornaria]

comment:21

For GAP_SO see #33446 which intends to make it no longer necessary. The patch to _get_shared_lib_path() would not be used either.

[tornaria]

comment:23

Notes about GAP_ROOT_DIR:

• my patches in comment:17 and comment:18 are not good, because running gap at sage runtime is too costly (about 0.7s in my box, which would double sage initialization time).
• sage installs all the gap packages in a single directory $SAGE_LOCAL/share/gap. However, system gap may have packages split in more than one location (e.g. debian uses /usr/share/gap and /usr/lib/gap, the latter to place compiled portions of packages).

Maybe the GAP_ROOT_DIR variable should be replaced by a GAP_ROOT_PATHS variable to be determined at configure time with some magic. Distros could override this variable as usual.

[orlitzky]

Has anything improved in the past year and a half? @tobiasdiez is working on a meson build for sagelib that uses only

gap = cc.find_library('gap')

to find GAP. If it's that easy now, we could finally write the spkg-configure.m4 for it; if it's not then it would be good to know the current list of pitfalls of that approach.

[mkoeppe]

Fixed in #36792