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: |
Description (last modified by )
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)
Change History (24)
Changed 2 years ago by
comment:1 Changed 22 months ago by
- Cc dimpase mjo slelievre added
system package information also needs to be added
comment:2 Changed 19 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:4 follow-up: ↓ 5 Changed 19 months ago by
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: ↓ 10 Changed 19 months ago by
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
- Description modified (diff)
comment:7 Changed 14 months ago by
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
- Branch set to public/29644
- Commit set to 2a41f491e5ce599ba185f5e93f02e0fcebd76c9e
comment:9 Changed 14 months ago by
- Milestone changed from sage-9.3 to sage-9.4
comment:10 in reply to: ↑ 5 Changed 13 months ago by
comment:11 Changed 10 months ago by
- Milestone changed from sage-9.4 to sage-9.5
comment:12 Changed 5 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:13 follow-up: ↓ 14 Changed 4 months ago by
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:
build/pkgs/gap/spkg-configure.m4
: setGAP_ROOT
using my suggestion above (while we are at it, check that runninggap
works, which is required by sage and I'm not sure is checked right now).- maybe check that the gap root is ok e.g. by looking for the file
${GAP_ROOT}/sysinfo.gap
. - if gap will not be used from system, unset
GAP_ROOT
AC_SUBST(GAP_ROOT)
- in
pkgs/sage-conf/sage_conf.py.in
add a line likeGAP_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
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
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
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
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
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: ↓ 20 Changed 4 months ago by
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
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
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
- Milestone changed from sage-9.6 to sage-9.7
comment:23 Changed 3 months ago by
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.
spkg-configure.m4 to be copied under build/pkgs/gap/