Opened 3 years ago
Closed 3 years ago
#26930 closed enhancement (fixed)
add GAP io and crypting pkgs
Reported by:  dimpase  Owned by:  

Priority:  critical  Milestone:  sage8.7 
Component:  packages: optional  Keywords:  
Cc:  embray, fbissey, ghtimokau, jhpalmieri, andrew.mathas  Merged in:  
Authors:  Dima Pasechnik, Erik Bray  Reviewers:  Erik Bray 
Report Upstream:  N/A  Work issues:  
Branch:  c84753f (Commits, GitHub, GitLab)  Commit:  c84753f36265886e31cbf9d7c31745d176a907c8 
Dependencies:  #27218, #27230, #27380  Stopgaps: 
Description (last modified by )
These GAP packages extend GAP kernel functionality. On the libgap side, this requires tinkering with the loader, see on #22626 Erik proposed a patch.
Here is a branch that implements this.
We also need to fix GAP's gac/libtool minitoolchain, used by some GAP pkgs. This involves patching of gac, and installing libtool, done in #27218.
Change History (100)
comment:1 Changed 3 years ago by
comment:2 followup: ↓ 3 Changed 3 years ago by
That's not really the intended "libgap.so". I meant the one from GAP itself, in $SAGE_LOCAL/lib/libgap.so
. The redlopening the Python module might do it as well.
The other option, as I mentioned, would be to use Python's builtin sys.setdlopenflags(...)
, but that has to be called before importing any modules (e.g. sage.lib.gap.libgap
) that are linked with libgap. It would probably be good afterwards to set it back to its default which is RTLD_LAZY
.
But yes, it's probably more portable either way to give a full path to the relevant shared object.
comment:3 in reply to: ↑ 2 Changed 3 years ago by
Replying to embray:
That's not really the intended "libgap.so". I meant the one from GAP itself, in
$SAGE_LOCAL/lib/libgap.so
.
oh, right  except it's .dylib
, not .so
. (I searched for libgap.so
, absentmindedly, found unique match, tried it :))
And indeed simply replacing libgap.so
with libgap.dylib
in the dlopen call suffices,
no need for the full path.
So this is easy to fix  except that in C I would have gone for a macro with the right extension, how is one supposed to do this in Cython?
comment:4 followup: ↓ 5 Changed 3 years ago by
Your plan isn't clear from the ticket description. Is your plan to add IO to the gap_packages
optional package? By the way: Could you remind me of the current status? I.e., is database_gap
now part of gap_packages
, or is it still independent? Is it supposed to be standard soon?
comment:5 in reply to: ↑ 4 Changed 3 years ago by
Replying to SimonKing:
Your plan isn't clear from the ticket description. Is your plan to add IO to the
gap_packages
optional package? By the way: Could you remind me of the current status? I.e., isdatabase_gap
now part ofgap_packages
, or is it still independent? Is it supposed to be standard soon?
database_gap
spkg is there no more, it's part of gap
spkg; in fact, already
on #22626.
On #26856 Erik prefers to keep gap_packages
optional, at least for the upcoming (quick) release 8.6, to make Debian deadline of 12 Jan.
We have not included IO there, as it'd be an extra over the current gap_packages
, and something that needs more testing. Then we can extend gap_packages
(and merge everything into gap
, as we are already using the same tarball for gap
and gap_packages
anyway).
comment:6 Changed 3 years ago by
And now a question on the IO package: From its documentation, I see that it allows to pickle any GAP object into a file object. But that's cumbersome for serialisation in Sage.
In Sage, we want a __reduce__
method (or a different serialisation protocol) to return a data tuple that is eventually dumped to a string by standard Python tools. But it seems wrong to me if we have a complicated object (say, a cohomology ring) with several attributes, one of them being defined in libgap, and to write only this attribute directly to a file.
So, how do you envision libgap serialisation?
comment:7 followup: ↓ 8 Changed 3 years ago by
Would you rather prefer IO dumping to a string? (i.e. into memory, right?) I'm not sure, but perhaps this is doable via their streams; it sounds like a reasonable feature to add to IO, if it's not there yet.
As Python serialisation was never my concern, I have little idea about it.
comment:8 in reply to: ↑ 7 Changed 3 years ago by
Replying to dimpase:
As Python serialisation was never my concern, I have little idea about it.
This is a SageMath ticket. So, Python serialisation has to come into play sooner or later.
Having IO installed and working is a nice first step. But eventually,
sage: G = libgap.DihedralGroup(8) sage: loads(dumps(G))
should work. And also something like the following should work:
class Foo(Ring): def __init__(self, G): assert G in Groups() self.G = G ... def __reduce__(self): return Foo, self.G
and then
sage: loads(dumps(Foo(libgap.DihedralGroup(8))))
But that would be for two followup tickets. One for pickling, the other for making libgap aware of Sage's categories (thus, translating G in Groups()
into bool(G.IsGroup())
).
comment:9 Changed 3 years ago by
 Commit changed from 3b47f262f0f0db432151826bea079c28b805b07b to eed5de7ffec325d75f08eb8b2b777228e148fabf
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
35abd34  Merge branch 'develop' of trac.sagemath.org:sage into gap410pkg

db3f63e  more libGAP in as_permutation(), less pexpect mess

2d77eee  follow up pyflake hints (all but one)

7dd5fa6  reviewer's example and fix

ca05b24  Merge branch 'u/dimpase/groups/better_as_permutation' of trac.sagemath.org:sage into gap410pkg

bd849fb  don't clean too early

ff7722e  suppress "#I.." warnings

bf40575  silencing of #I warning from GAP in the function body

37e2329  tagged # random  answer is always mathematically OK, outputs differ

eed5de7  inital support for IO, following Erik on #22626

comment:10 Changed 3 years ago by
rebased over the branch on #26856, added OSX (and perhaps even Cygwin?) support.
comment:11 Changed 3 years ago by
 Commit changed from eed5de7ffec325d75f08eb8b2b777228e148fabf to 932811179edc2f981d55f4af330134a6d3e1fa49
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
5f49332  fix sdh_install of symlinks with invalid targets

9854c3a  Merge branch 'u/embray/build/sdhinstallsymlinks' into public/ticket26856

963757f  don't clean too early

da38d2c  tagged # random  answer is always mathematically OK, outputs differ

d8b912d  inital support for IO, following Erik on #22626

0e6f33c  IO has landed, no need for this workaround

8bf1044  disable doctesting of these modules

a7c5ccd  Merge branch 'public/ticket26856' of trac.sagemath.org:sage into gap_io

c12daa6  Merge branch 'u/dimpase/packages/gap_io' of trac.sagemath.org:sage into gap_io

9328111  no need to suppress warining

comment:12 Changed 3 years ago by
 Cc ghtimokau added
comment:13 Changed 3 years ago by
 Commit changed from 932811179edc2f981d55f4af330134a6d3e1fa49 to 20c676a5d3fea184e82809366543e486e6aa2c98
Branch pushed to git repo; I updated commit sha1. New commits:
20c676a  Merge remotetracking branch 'trac/develop' into HEAD

comment:14 Changed 3 years ago by
merged 8.6.rc0 in.
comment:15 Changed 3 years ago by
 Milestone changed from sage8.6 to sage8.7
Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sagepending or sagewishlist.
comment:16 Changed 3 years ago by
 Status changed from new to needs_review
comment:17 Changed 3 years ago by
Minor comment: if you are changing the spkginstall
script of a package, you should also bump the patchlevel to force a rebuild.
comment:18 Changed 3 years ago by
Since dlopen
takes a const char*
, it's safer to use byte literals here:
for suffix in [b"so", b"dylib", b"dll"]: handle = dlopen(b"libgap." + suffix, RTLD_NOW  RTLD_GLOBAL)
comment:19 Changed 3 years ago by
comment:20 Changed 3 years ago by
 Dependencies #22626, #26856 deleted
comment:21 followup: ↓ 24 Changed 3 years ago by
 Status changed from needs_review to needs_work
Copying a file into $SAGE_LOCAL
like this is very fishy. At least it needs to be better explained why it's needed:
cp f "$SAGE_LOCAL/include/gap/config.h" "$GAP_ROOT/src/"
comment:22 Changed 3 years ago by
Finally a minor Cython style issue: I prefer handle is NULL
as a more Pythonic spelling of handle == NULL
.
comment:23 Changed 3 years ago by
By the way, the commit history is a mess. Maybe it would be better to squash and rebase on top of 8.6.
comment:24 in reply to: ↑ 21 ; followup: ↓ 25 Changed 3 years ago by
Replying to jdemeyer:
Copying a file into
$SAGE_LOCAL
like this is very fishy. At least it needs to be better explained why it's needed:cp f "$SAGE_LOCAL/include/gap/config.h" "$GAP_ROOT/src/"
If I remember correctly this is where io.c
expect some of its "include". I am planning to patch in sageongentoo rather than doing something like that.
Now going through the ticket and this bit in particular:
+ cdef void* handle + for suffix in ["so", "dylib", "dll"]: + handle = dlopen("libgap."+suffix, RTLD_NOW  RTLD_GLOBAL) + if handle != NULL: + break
where the point has been made that we don't need the full path. Now that starts to get unrelated but we do the same thing for libsingular in libs/singular.pyx
and we go to great length to get the full path even defining it in env.py
. If I read this correctly it is now way more complicated than it should be.
comment:25 in reply to: ↑ 24 Changed 3 years ago by
Replying to fbissey:
Replying to jdemeyer:
Copying a file into
$SAGE_LOCAL
like this is very fishy. At least it needs to be better explained why it's needed:cp f "$SAGE_LOCAL/include/gap/config.h" "$GAP_ROOT/src/"If I remember correctly this is where
io.c
expect some of its "include". I am planning to patch in sageongentoo rather than doing something like that.
That's right. Perhaps the proper way would be to use sdh_install
rather than cp
, but on the other hand it's not clear to me whether $GAP_ROOT/src/
is a location supported by this machinery.
Erik, could you look into this?
(Of course, GAP having its include files in the same directory as C sources is a problem, due to their lack of proper install mechanism...)
comment:26 Changed 3 years ago by
This is all I had to do compile io
against an installed gap (with config.h
installed in include/gap
like in vanilla sage)

pkg/io4.5.4/src/io.c
diff git a/pkg/io4.5.4/src/io.c b/pkg/io4.5.4/src/io.c index 5aa4a082..07e6e312 100644
a b 11 11 /* Try to use as much of the GNU C library as possible: */ 12 12 #define _GNU_SOURCE 13 13 14 #include " src/compiled.h" /* GAP headers */15 #include " src/iostream.h" /* Signal Handling */14 #include "gap/compiled.h" /* GAP headers */ 15 #include "gap/iostream.h" /* Signal Handling */ 16 16 17 17 #undef PACKAGE 18 18 #undef PACKAGE_BUGREPORT
comment:27 Changed 3 years ago by
 Branch changed from u/dimpase/packages/gap_io to public/packages/gap_io
 Commit changed from 20c676a5d3fea184e82809366543e486e6aa2c98 to 61afc499792c831be0e6e10e60979765a7f48f05
 Status changed from needs_work to needs_review
comment:28 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:29 Changed 3 years ago by
Thanks for doing the rebase, but rebased the messy branch from #26930
is not really a good commit message. Better would be something like Trac #26930: add GAP io package
.
comment:30 Changed 3 years ago by
I would like to handle the path for libgap.so (and its related variants) better, and had planned to do so soon before I saw all the recent work on this ticket.
I think the path to libgap should be set in sage.env
and should be possible to override by environment variable, analogous to the handling of SINGULAR_SO
(it could even repurpose the same code moreorless).
comment:31 followup: ↓ 35 Changed 3 years ago by
This change

build/pkgs/gap_packages/spkginstall
diff git a/build/pkgs/gap_packages/spkginstall b/build/pkgs/gap_packages/spkginstall index 1a3e8c5..607d2c7 100644
a b install_compiled_pkg() 48 48 # Clean up any build artificts before installing the rest of the package 49 49 # Also remove configure/Makefiles 50 50 # Note: None, if any of the packages really have a proper install target 51 make clean # Works for some packages but not all52 rm rf bin/53 rm rf configure configure.* config.* autogen.sh *.m4 Makefile* m4/54 51 55 52 # Create the bin/ symlink 56 53 ln s "$SAGE_LOCAL/lib/gap/pkg/$pkg/bin" bin 57 54 58 55 # Install the rest of the package files 59 56 sdh_install * "$PKG_DIR/$pkg" 57 make clean # Works for some packages but not all 58 rm rf bin/ 59 rm rf configure configure.* config.* autogen.sh *.m4 Makefile* m4/ 60 60 } 61 61 62 62 # Build and install compiled packages:
is not good. You seem to have brought it over from some old branch for #26856, where I also had to remove it: https://trac.sagemath.org/ticket/26856#comment:110
comment:32 Changed 3 years ago by

src/doc/en/constructions/groups.rst
diff git a/src/doc/en/constructions/groups.rst b/src/doc/en/constructions/groups.rst index 30d8da4..0cc7b92 100644
a b Here's another way, working more directly with GAP:: 184 184 [ Group( [ f1, f2 ] ), Group( [ f2 ] ), Group( <identity> of ... ) ] 185 185 sage: print(gap.eval("G := SymmetricGroup( 4 )")) 186 186 Sym( [ 1 .. 4 ] ) 187 sage: print(gap.eval("normal := NormalSubgroups( G );")) 187 sage: print(gap.eval("normal := NormalSubgroups( G );")) # random 188 188 [ Sym( [ 1 .. 4 ] ), Alt( [ 1 .. 4 ] ), Group([ (1,4)(2,3), (1,3)(2,4) ]), 189 189 Group(()) ]
I'm confused why it's still giving you random results here. #26874 should have fixed this. I want to look into it. It's not too important of course, but it still troubles me that there's some nondeterminism in the tests when AFAIK there shouldn't be.
comment:33 followup: ↓ 34 Changed 3 years ago by
I don't know why you removed this:

src/sage/graphs/strongly_regular_db.pyx
diff git a/src/sage/graphs/strongly_regular_db.pyx b/src/sage/graphs/strongly_regular_db.pyx index 55a1428..90af15a 100644
a b def SRG_416_100_36_20(): 2450 2450 (416, 100, 36, 20) 2451 2451 """ 2452 2452 from sage.libs.gap.libgap import libgap 2453 libgap.eval("SetInfoLevel(InfoWarning,0)") # silence #I warnings from GAP (without IO pkg)2454 2453 libgap.LoadPackage("AtlasRep") 2455 2454 g=libgap.AtlasGroup("G2(4)",libgap.NrMovedPoints,416) 2456 libgap.eval("SetInfoLevel(InfoWarning,1)") # restore #I warnings2457 2455 h = Graph() 2458 2456 h.add_edges(g.Orbit([1,5],libgap.OnSets)) 2459 2457 h.relabel()
It's still not a standard package, so these warnings will still happen if the IO package is not installed. This isn't just a problem for sagethedistribution. E.g. on Debian it's perfectly possible (at least for now) to install the sagemath package without installing the gapio package.
comment:34 in reply to: ↑ 33 Changed 3 years ago by
Replying to embray:
I don't know why you removed this:
src/sage/graphs/strongly_regular_db.pyx
diff git a/src/sage/graphs/strongly_regular_db.pyx b/src/sage/graphs/strongly_regular_db.pyx index 55a1428..90af15a 100644
a b def SRG_416_100_36_20(): 2450 2450 (416, 100, 36, 20) 2451 2451 """ 2452 2452 from sage.libs.gap.libgap import libgap 2453 libgap.eval("SetInfoLevel(InfoWarning,0)") # silence #I warnings from GAP (without IO pkg)2454 2453 libgap.LoadPackage("AtlasRep") 2455 2454 g=libgap.AtlasGroup("G2(4)",libgap.NrMovedPoints,416) 2456 libgap.eval("SetInfoLevel(InfoWarning,1)") # restore #I warnings2457 2455 h = Graph() 2458 2456 h.add_edges(g.Orbit([1,5],libgap.OnSets)) 2459 2457 h.relabel() It's still not a standard package, so these warnings will still happen if the IO package is not installed. This isn't just a problem for sagethedistribution. E.g. on Debian it's perfectly possible (at least for now) to install the sagemath package without installing the gapio package.
oh, I might have been confused by shaffling packages around. This chane makes sense if atlasrep is in gappackages, but this is not the case any more.
comment:35 in reply to: ↑ 31 Changed 3 years ago by
Replying to embray:
This change is not good. You seem to have brought it over from some old branch for #26856, where I also had to remove it: https://trac.sagemath.org/ticket/26856#comment:110
duh... I looked at that diff, wondering whether there is some whitespaceonly difference there, for it looked totally the same :(
comment:36 Changed 3 years ago by
 Commit changed from 61afc499792c831be0e6e10e60979765a7f48f05 to bce40a554af8ba8a7d17251ce9a5f108143a0da0
comment:37 Changed 3 years ago by
What should we do with comment 21? Just patch the GAP/io package source?
comment:38 Changed 3 years ago by
 Commit changed from bce40a554af8ba8a7d17251ce9a5f108143a0da0 to 0d7016aa86ca24de03a3c3e7b5698af1e57040ff
Branch pushed to git repo; I updated commit sha1. New commits:
0d7016a  patch source of GAP/io to correct header location

comment:39 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:40 Changed 3 years ago by
 Description modified (diff)
 Summary changed from add GAP io package (and other GAP kernel modules) to add GAP io package, set up libgap for GAP kernel modules
comment:41 Changed 3 years ago by
piing...
comment:42 Changed 3 years ago by
Hi!
I'm currently trying to compile a bunch of GAP packages in Sage environment, and apparently, I need to add this:
$ CFLAGS="I../.. I../../gen" ./configure withgaproot=../..
Is it the only way?
comment:43 Changed 3 years ago by
 Status changed from needs_review to needs_work
I just looked into this with Odile, who was trying to compile the IO package. I don't think patching the sources is the way. It's strange that the configure script, even given a path to the gaproot, doesn't add this. I will look into what is intended for this and see if there's a reason what we're doing with just passing withgaproot
to configure doesn't work.
Either it's a bug on our end, or a bug in the package's configure. Odile was able to work around it by adding CFLAGS=I../../ I../../gen
and that seemed to work (where ../../
here was the relative path to the gaproot). If we have to do that we can, but it still smells like a hack.
I also want to address https://trac.sagemath.org/ticket/26930#comment:30
comment:44 followup: ↓ 59 Changed 3 years ago by
I don't see why we should not patch package's source.
It's written with GAP's sources and headers alike living in GAPROOT/src/, and
naturally you can either create some foo/src to use in I
, or else, indeed, replace src/
with gap/
.
I would like to see the log of the failure (without these extra CFLAGS)
comment:45 followup: ↓ 52 Changed 3 years ago by
after a bare ./configure withgaproot=../.. :
(sagesh) odile@ancolie:json2.0.0$ make CXX src/json.lo src/json.cc:5:10: fatal error: src/compiled.h: Aucun fichier ou dossier de ce type #include "src/compiled.h" /* GAP headers */ ^~~~~~~~~~~~~~~~ compilation terminated. make: *** [Makefile:501: src/json.lo] Error 1
comment:46 Changed 3 years ago by
Don't do things like this at sage sh
prompt, it's not meant for this.
Just do the usual ./sage f gap_packages
(with the branch pulled in, naturally).
comment:47 Changed 3 years ago by
and if you want more GAP packages installed, modify gap_packages/spkginstall
accordingly.
comment:48 followup: ↓ 51 Changed 3 years ago by
Could you point me to a documentation page for this?
(I just used https://wiki.sagemath.org/InstallingGapPackages, which recommends compiling within the Sage shell)
Note: I'm a complete newbie to GAP, not to mention GAPinSage ..
comment:49 Changed 3 years ago by
Thanks anyway, I'll try that.
comment:50 Changed 3 years ago by
Here is my gap_packages4.10.0.loghttp://fluidinfo.fr/gap/gap_packages4.10.0.log
Note: I have a crypting0.9 source directory, in my ./local/share/gap/pkg : for some reason it was not in the build directory as the other packages, I just copied it there and here is my second loghttp://fluidinfo.fr/gap/gap_packages4.10.0.log.1
comment:51 in reply to: ↑ 48 ; followup: ↓ 54 Changed 3 years ago by
Replying to zerline:
Could you point me to a documentation page for this?
(I just used https://wiki.sagemath.org/InstallingGapPackages, which recommends compiling within the Sage shell)
This link is totally obsolete  edited last time in 2013. I've just edited that page to make this clear. Sorry...
comment:52 in reply to: ↑ 45 ; followup: ↓ 55 Changed 3 years ago by
Replying to zerline:
after a bare ./configure withgaproot=../.. :
(sagesh) odile@ancolie:json2.0.0$ make CXX src/json.lo src/json.cc:5:10: fatal error: src/compiled.h: Aucun fichier ou dossier de ce type #include "src/compiled.h" /* GAP headers */ ^~~~~~~~~~~~~~~~ compilation terminated. make: *** [Makefile:501: src/json.lo] Error 1
That's exactly one of the things I patch see https://trac.sagemath.org/ticket/26930?replyto=45#comment:26
comment:53 Changed 3 years ago by
 Status changed from needs_work to needs_review
Let us separate a review for this ticket and adding yet more GAP packages. I just opened #27218 for this purpose.
comment:54 in reply to: ↑ 51 ; followup: ↓ 56 Changed 3 years ago by
Replying to dimpase:
Replying to zerline:
Could you point me to a documentation page for this?
(I just used https://wiki.sagemath.org/InstallingGapPackages, which recommends compiling within the Sage shell)
This link is totally obsolete  edited last time in 2013. I've just edited that page to make this clear. Sorry...
Yet this method works .. especially if you want to also test GAP by itself: from the GAP console, apparently these packages (those compiled with the method above) are loaded.
comment:55 in reply to: ↑ 52 Changed 3 years ago by
Replying to fbissey:
Replying to zerline:
after a bare ./configure withgaproot=../.. :
(sagesh) odile@ancolie:json2.0.0$ make CXX src/json.lo src/json.cc:5:10: fatal error: src/compiled.h: Aucun fichier ou dossier de ce type #include "src/compiled.h" /* GAP headers */ ^~~~~~~~~~~~~~~~ compilation terminated. make: *** [Makefile:501: src/json.lo] Error 1That's exactly one of the things I patch see https://trac.sagemath.org/ticket/26930?replyto=45#comment:26
thank you
comment:56 in reply to: ↑ 54 Changed 3 years ago by
Replying to zerline:
Replying to dimpase:
Replying to zerline:
Could you point me to a documentation page for this?
(I just used https://wiki.sagemath.org/InstallingGapPackages, which recommends compiling within the Sage shell)
This link is totally obsolete  edited last time in 2013. I've just edited that page to make this clear. Sorry...
Yet this method works .. especially if you want to also test GAP by itself: from the GAP console, apparently these packages (those compiled with the method above) are loaded.
there is no guarantee that one does not break GAP's installation in Sage this way  and it happened in the past often that people tried installing this way GAP packages with kernel modules (such as IO) and ended up with broken Sage installations.
comment:57 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:58 Changed 3 years ago by
 Description modified (diff)
 Priority changed from major to critical
 Summary changed from add GAP io package, set up libgap for GAP kernel modules to fix GAP's gac/libtool, add GAP io and crypting pgs, fix libgap for GAP kernel modules
 Work issues test it in libgap, also on OSX deleted
comment:59 in reply to: ↑ 44 Changed 3 years ago by
Replying to dimpase:
I don't see why we should not patch package's source.
It isn't necessary normally when building the package, so why should it have to be patched? ISTM the problem is just how GAP is being "installed" in Sage, which as we know is tricky since it doesn't have a proper installation system.
I think the stuff Odile pointed out last night about sysinfo.gap
clearly needs to be addressed (and in a separate ticket; this one is already trying to do too much).
For example, mine contains (this is just partial):
$ cat sysinfo.gap # This files has been generated by the GAP build system, # do not edit manually! GAParch=x86_64unknowncygwindefault64 GAP_ABI=64 GAP_HPCGAP=no GAP_ADDGUARDS2= GAP_BIN_DIR="/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap4.10.0/src" GAP_LIB_DIR="/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap4.10.0/src" GAP_CC="gcc" GAP_CFLAGS=" O2 g " GAP_CPPFLAGS=" I/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap4.10.0/src/gen I/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap4.10.0/src/src I/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap4.10.0/src DHAVE_CONFIG_H I/home/embray/src/sagemath/sage/local/include " GAP_LDFLAGS=" L/home/embray/src/sagemath/sage/local/lib L/home/embray/src/sagemath/sage/local/lib Wl,rpath,/home/embray/src/sagemath/sage/local/lib Wl,stack,16777216" GAP_LIBS=" lgmp lz lreadline "
In other words, it puts in a lot of paths, which seem to be important for building many packages, but that reference the SPKG build directory and not the actual final GAP_ROOT location (under $SAGE_LOCAL/share/gap
).
So I think this probably needs to be fixed up when "installing" GAP, at a minimum.
comment:60 Changed 3 years ago by
On the other hand, the problem of using src/ prefix for includes in quite a few GAP packages seems to be a bit too big to patch.
comment:61 Changed 3 years ago by
 Dependencies set to #27218
 Summary changed from fix GAP's gac/libtool, add GAP io and crypting pgs, fix libgap for GAP kernel modules to add GAP io and crypting pkgs
comment:62 Changed 3 years ago by
 Description modified (diff)
comment:63 Changed 3 years ago by
I added yet one more ticket for working on a way to get the libgap.so: #27230
The idea, as in comment:30, was to use the same code for both SINGULAR_SO
and a new LIBGAP_SO
variable which serves a similar purpose.
If that code ends up being contentious we can fall back on a simpler method, though I think either way it makes sense to reuse the code used for SINGULAR_SO
as well, rather than just looping over some extension names.
comment:64 Changed 3 years ago by
 Branch changed from public/packages/gap_io to u/dimpase/gap_io
 Commit changed from 0d7016aa86ca24de03a3c3e7b5698af1e57040ff to 87f59ccc8aa0b1697da8d6f11991bd1236b562e9
comment:65 Changed 3 years ago by
If you want, please also try merging #27230 into this and adding a LIBGAP_SO
to sage.env
like I suggested in that ticket, and then use that instead of the loop you have currently around dlopen
. That's a good observation that for Python 3 it will have to pass bytes to dlopen()
, so you can use str_to_bytes(LIBGAP_SO)
for that.
comment:66 Changed 3 years ago by
 Commit changed from 87f59ccc8aa0b1697da8d6f11991bd1236b562e9 to 3a65f9e38b16c1c85fb581a327fa7855c613df68
Branch pushed to git repo; I updated commit sha1. New commits:
51aa946  repackages the code for finding the libSingular.so as a somewhat

34b27b2  rework _get_shared_lib_filename to cover more systemspecific cases,

917a21b  fixed the test to be a little more flexible

785bf10  Merge branch 'u/embray/misc/sageenvlibfilename' of trac.sagemath.org:sage into HEAD

3a65f9e  use GAP_SO in the dlopen call

comment:67 Changed 3 years ago by
 Dependencies changed from #27218 to #27218, #27230
 Status changed from needs_work to needs_review
once #27230 done it will be ready for review
comment:68 Changed 3 years ago by
 Commit changed from 3a65f9e38b16c1c85fb581a327fa7855c613df68 to 0d7e1e81708a8543c4bb5d10ff4a00159c2a7414
Branch pushed to git repo; I updated commit sha1. New commits:
0d7e1e8  add setting GAP_SO to env.py

comment:69 Changed 3 years ago by
so this is now really ready for review.
comment:70 followup: ↓ 71 Changed 3 years ago by
Is this supposed to work on distro with an unchanged gap or is the io package now required?
comment:71 in reply to: ↑ 70 Changed 3 years ago by
Replying to ghtimokau:
Is this supposed to work on distro with an unchanged gap or is the io package now required?
no, io package is still optional. As you see, this ticket does two things:
 fixes Sage's libgap interface to allow kernel modules
 adds two packages (both needing the 1st thing) to its optional spkg gap_packages
comment:72 Changed 3 years ago by
 Commit changed from 0d7e1e81708a8543c4bb5d10ff4a00159c2a7414 to a2f76b272ef3e8626468257b34e05484cf4b82bd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4556593  Trac #27218: Ensure that the same libtool that was used to build GAP is also

44d20c1  repackages the code for finding the libSingular.so as a somewhat

1eebc0d  rework _get_shared_lib_filename to cover more systemspecific cases,

ad3410b  fixed the test to be a little more flexible

691eeb8  rebased over 8.7.beta3 and the dependent tickets

a2f76b2  add setting GAP_SO to env.py

comment:73 Changed 3 years ago by
 Commit changed from a2f76b272ef3e8626468257b34e05484cf4b82bd to 99c0afca419ea98ffa31762a21425c4591f98a83
comment:74 Changed 3 years ago by
 Commit changed from 99c0afca419ea98ffa31762a21425c4591f98a83 to 4927c10700abe459289512245549ce035d0d9f52
comment:75 Changed 3 years ago by
 Dependencies changed from #27218, #27230 to #27218, #27230, #27380
comment:76 Changed 3 years ago by
#27380 (extra /
removed (sic!)) has apparently fixed the problem I had with this branch on OSX, upon running tests on my ancient MacBook it should be ready to go.
comment:77 Changed 3 years ago by
Tests pass on OSX and Linux. Please review.
comment:78 Changed 3 years ago by
 Cc jhpalmieri andrew.mathas added
OSXspecific reviews are needed for this ticket and #27380
comment:79 Changed 3 years ago by
Is there a reason this first "installs" crypting by copy in

build/pkgs/gap_packages/spkginstall
diff git a/build/pkgs/gap_packages/spkginstall b/build/pkgs/gap_packages/spkginstall index 1a3e8c5..07ce727 100644
a b sdh_install \ 15 15 AutoDoc* \ 16 16 corelg \ 17 17 crime* \ 18 crypting* \ 18 19 cryst \ 19 20 crystcat \ 20 21 design \
but then installs it again (including compilation) further down in
@@ 63,7 +64,7 @@ install_compiled_pkg() # # These packages have an old ./configure that take the GAP_ROOT as a positional # argument for pkg in cohomolo* grape* guava* +for pkg in cohomolo* crypting* grape* guava* do echo "Building GAP package $pkg" cd "$PKG_SRC_DIR/$pkg"
comment:80 followup: ↓ 82 Changed 3 years ago by
looks like a harmless typo...
comment:81 Changed 3 years ago by
Any comment on comment:32 ? This test really shouldn't be giving unpredictable results.
comment:82 in reply to: ↑ 80 Changed 3 years ago by
Replying to dimpase:
looks like a harmless typo...
I wouldn't necessarily call it "harmless" but if it's a typo that's okay; I just wondered if there was a real reason or not.
comment:83 Changed 3 years ago by
 Commit changed from 4927c10700abe459289512245549ce035d0d9f52 to 1a2bb6daa62b266a8463ab1f1f40b3a3b4024b12
Branch pushed to git repo; I updated commit sha1. New commits:
1a2bb6d  only install crypting once

comment:84 Changed 3 years ago by
 Commit changed from 1a2bb6daa62b266a8463ab1f1f40b3a3b4024b12 to 61600394e43bdefbc918fa03a817311b39af4a05
Branch pushed to git repo; I updated commit sha1. New commits:
6160039  removed #random tag, as it's hopefully fixed

comment:85 Changed 3 years ago by
Fixed both things.
comment:86 followup: ↓ 87 Changed 3 years ago by
Seems to work on OS X. At least there is more stuff (io
, crypting
) in local/share/gap/pkg
with this branch than without it. I'm not sure what the breakage was from #27380: gap_packages
installed for me on top of a clean install of 8.7.beta5 without these branches.
What else would you like me to check?
comment:87 in reply to: ↑ 86 Changed 3 years ago by
Replying to jhpalmieri:
Seems to work on OS X. At least there is more stuff (
io
,crypting
) inlocal/share/gap/pkg
with this branch than without it. I'm not sure what the breakage was from #27380:gap_packages
installed for me on top of a clean install of 8.7.beta5 without these branches.
but some GAP packages were not fully functional, as their binaries (only few GAP packages have these) were installed in wrong directories...
What else would you like me to check?
could you run tests, just to have another data point? (I tested on OSX 10.13, I don't have 10.14, my OSX box is too old for it).
comment:88 Changed 3 years ago by
All tests pass for me. I didn't run full tests with gap_packages
but without this branch, but I ran limited tests and saw one failure in sage/groups
. So this looks good to me.
comment:89 followup: ↓ 90 Changed 3 years ago by
I don't have a great internet connection at the moment but I can test on macosx 10.14.2 by Monday, if not earlier. This said, since this is a package can some one please tell me what needs to be tested. Is sage i io && sage i macosx && make ptestlong
enough?
comment:90 in reply to: ↑ 89 Changed 3 years ago by
Replying to andrew.mathas:
I don't have a great internet connection at the moment but I can test on macosx 10.14.2 by Monday, if not earlier. This said, since this is a package can some one please tell me what needs to be tested. Is
sage i io && sage i macosx && make ptestlong
enough?
You just need to pull the branch, the GAP tarball is the same. So can be done over GSM :)
After merging the branch, you'll need to do
./sage f gap_packages && make ptestlong
And if there are errors in tests, please posts them (from logs/ptestlong.log
file).
Thanks!
comment:91 Changed 3 years ago by
On Cygwin I'm getting a few cases of this failure in src/sage/coding/linear_code.py
:
sage t long src/sage/coding/linear_code.py ********************************************************************** File "src/sage/coding/linear_code.py", line 3134, in sage.coding.linear_code.AbstractLinearCode.weight_distribution Failed example: C.weight_distribution(algorithm="leon") # optional  gap_packages (Guava package) Exception raised: Traceback (most recent call last): File "/home/embray/src/sagemath/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 671, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/embray/src/sagemath/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 1095, in compile_and_execute exec(compiled, globs) File "<doctest sage.coding.linear_code.AbstractLinearCode.weight_distribution[8]>", line 1, in <module> C.weight_distribution(algorithm="leon") # optional  gap_packages (Guava package) File "sage/misc/cachefunc.pyx", line 1950, in sage.misc.cachefunc.CachedMethodCaller.__call__ (build/cythonized/sage/misc/cachefunc.c:10265) w = self._instance_call(*args, **kwds) File "sage/misc/cachefunc.pyx", line 1826, in sage.misc.cachefunc.CachedMethodCaller._instance_call (build/cythonized/sage/misc/cachefunc.c:9750) return self.f(self._instance, *args, **kwds) File "/home/embray/src/sagemath/sage/local/lib/python2.7/sitepackages/sage/coding/linear_code.py", line 3184, in weight_distribution lines = subprocess.check_output([os.path.join(guava_bin_dir, 'wtdist'), input]) File "/home/embray/src/sagemath/sage/local/lib/python2.7/subprocess.py", line 223, in check_output raise CalledProcessError(retcode, cmd, output=output) CalledProcessError: Command '['/home/embray/src/sagemath/sage/local/share/gap/pkg/guava3.14/bin/x86_64unknowncygwindefault64/wtdist', '/home/embray/.sage/temp/NAVIBrick/6764/tmp_7ZlhoX::code']' returned nonzero exit status 11
The "exit status 11" indicates that the wtdist
program, whatever that is, from the guava package is segfaulting. Which is odd because I don't remember having any problem with that before, and it's not clear why it would start with this ticket (unless maybe it's doing something with the IO module that it didn't do when it was unavailable).
I'm going to try backing out the changes from this ticket and rebuilding gap_packages
again just to rule out the possibility that the problem is somehow related to this ticket.
comment:92 Changed 3 years ago by
we didn't build this part of guava before, that's why...
comment:93 Changed 3 years ago by
I don't see anything explicitly in this ticket that would cause it to start being built though, so I'm a little confused. Is it because we didn't have #27218? Does it just, silently not build or something?
comment:94 Changed 3 years ago by
AFAICT the code that uses it doesn't check if the wtdist
program exists or not or anything like that, so it should have just failed before with a command not found or something if it wasn't being built...
comment:95 Changed 3 years ago by
Well I was able to reproduce that issue with gap_packages
prior to this ticket so I guess it's unrelated. I'll make a new ticket for it. I'm not overly concerned about it in this case since gap_packages is not installed by default anyways.
As a side note, I realized in testing this that it this ticket should also bump the patch version on the SPKG version.
We should also clean up the commit history. I can go ahead and do that...
comment:96 Changed 3 years ago by
oops, sorry, I meant it was not installed before GAP 4.10.
I gather it could be something trivial like whether or not there is .exe
suffix...
Yes, please, bump up the version and rebase. The followup to this is #27295.
comment:97 Changed 3 years ago by
 Branch changed from u/dimpase/gap_io to public/gap_io
 Commit changed from 61600394e43bdefbc918fa03a817311b39af4a05 to bb77ab1b14ac08afebb95a08a4981230b6809d5b
 Reviewers set to Erik Bray
 Status changed from needs_review to positive_review
comment:98 Changed 3 years ago by
 Commit changed from bb77ab1b14ac08afebb95a08a4981230b6809d5b to c84753f36265886e31cbf9d7c31745d176a907c8
 Status changed from positive_review to needs_review
comment:99 Changed 3 years ago by
 Status changed from needs_review to positive_review
Just reworded one of the commit messages.
comment:100 Changed 3 years ago by
 Branch changed from public/gap_io to c84753f36265886e31cbf9d7c31745d176a907c8
 Resolution set to fixed
 Status changed from positive_review to closed
on OSX the dlopen call does not work. However, it does if given full path to the dll, i.e.
src/sage/libs/gap/util.pyx
name resolution procedures are different on OSX. Actually, I don't know what's happening on Linux so that libgap.so is found. Perhaps, it's already loaded on Linux, but not on OSX?