spkg-configure.m4 for gap
Closed this issue · 25 comments
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
Attachment: spkg-configure.m4.gz
spkg-configure.m4 to be copied under build/pkgs/gap/
system package information also needs to be added
Changed author from gh-thierry-FreeBSD to Thierry Thomas
Which systems have a usable gap?
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.
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).
Description changed:
---
+++
@@ -7,3 +7,38 @@
- 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> ')
+```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"
Changed author from Thierry Thomas to Thierry Thomas, Samuel Lelièvre
Branch: public/29644
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
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).
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>
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/" ]
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.
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).
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.')
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 ?
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.
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.
Notes about GAP_ROOT_DIR:
- my patches in comment:17 and comment:18 are not good, because running
gapat 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/gapand/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.
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.