Opened 2 years ago

Last modified 3 months ago

#29644 new enhancement

spkg-configure.m4 for gap

Reported by: gh-thierry-FreeBSD Owned by:
Priority: major Milestone: sage-9.7
Component: build: configure Keywords: gap; spkg-configure; system packages
Cc: dimpase, mjo, slelievre, isuruf, arojas Merged in:
Authors: Thierry Thomas, Samuel Lelièvre Reviewers:
Report Upstream: N/A Work issues:
Branch: public/29644 (Commits, GitHub, GitLab) Commit: 2a41f491e5ce599ba185f5e93f02e0fcebd76c9e
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-tobiasdiez)

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

Attachments (1)

spkg-configure.m4 (235 bytes) - added by gh-thierry-FreeBSD 2 years ago.
spkg-configure.m4 to be copied under build/pkgs/gap/

Download all attachments as: .zip

Change History (24)

Changed 2 years ago by gh-thierry-FreeBSD

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

comment:1 Changed 22 months ago by mkoeppe

  • Authors changed from gh-thierry-FreeBSD to Thierry Thomas
  • Cc dimpase mjo slelievre added

system package information also needs to be added

comment:2 Changed 19 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 19 months ago by mkoeppe

  • Cc isuruf arojas added

Which systems have a usable gap?

comment:4 follow-up: Changed 19 months ago by arojas

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 19 months ago by dimpase

Replying to arojas:

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

comment:6 Changed 16 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:7 Changed 14 months ago by slelievre

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 <your.name@example.com>"

with name and email found from your previous contributions using:

$ git log | grep "Author: Thierry Thomas"

comment:8 Changed 14 months ago by slelievre

  • Authors changed from Thierry Thomas to Thierry Thomas, Samuel Lelièvre
  • Branch set to public/29644
  • Commit set to 2a41f491e5ce599ba185f5e93f02e0fcebd76c9e

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:

2c9813529644: Add spkg-configure.m4 for gap
2a41f4929644: Add distro information for gap

comment:9 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

comment:10 in reply to: ↑ 5 Changed 13 months ago by arojas

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

comment:11 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:12 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:13 follow-up: Changed 4 months ago by tornaria

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:

  1. 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).
  2. maybe check that the gap root is ok e.g. by looking for the file ${GAP_ROOT}/sysinfo.gap.
  3. if gap will not be used from system, unset GAP_ROOT
  4. AC_SUBST(GAP_ROOT)
  5. 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).

comment:14 in reply to: ↑ 13 Changed 4 months ago by dimpase

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> 

comment:15 Changed 4 months ago by dimpase

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/" ]

comment:16 Changed 4 months ago by tornaria

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.

comment:17 Changed 4 months ago by tornaria

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

comment:18 Changed 4 months ago by tornaria

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

comment:19 follow-up: Changed 4 months ago by 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 ?

comment:20 in reply to: ↑ 19 Changed 4 months ago by tornaria

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.

comment:21 Changed 3 months ago by tornaria

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.

comment:22 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7

comment:23 Changed 3 months ago by tornaria

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.

Note: See TracTickets for help on using tickets.