Opened 12 months ago

Closed 9 months ago

#26930 closed enhancement (fixed)

add GAP io and crypting pkgs

Reported by: dimpase Owned by:
Priority: critical Milestone: sage-8.7
Component: packages: optional Keywords:
Cc: embray, fbissey, gh-timokau, jhpalmieri, andrew.mathas Merged in:
Authors: Dima Pasechnik, Erik Bray Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: c84753f (Commits) Commit: c84753f36265886e31cbf9d7c31745d176a907c8
Dependencies: #27218, #27230, #27380 Stopgaps:

Description (last modified by dimpase)

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 mini-toolchain, used by some GAP pkgs. This involves patching of gac, and installing libtool, done in #27218.

Change History (100)

comment:1 Changed 12 months ago by dimpase

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

    a b cdef initialize(): 
    263263    # Note: we could use RTLD_NOLOAD and avoid the subsequent dlclose() but
    264264    # this isn't portable
    265265    cdef void* handle
    266     handle = dlopen("libgap.so", RTLD_NOW | RTLD_GLOBAL)
     266    # handle = dlopen("libgap.so", RTLD_NOW | RTLD_GLOBAL)
     267    handle = dlopen("/Users/dima/sagetrac-mirror/local/lib/python2.7/site-packages/sage/libs/gap/libgap.so", RTLD_NOW | RTLD_GLOBAL)
    267268    if handle == NULL:
    268269        raise RuntimeError(
    269270                "Could not dlopen() libgap.so even though it should already "

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?

comment:2 follow-up: Changed 12 months ago by embray

That's not really the intended "libgap.so". I meant the one from GAP itself, in $SAGE_LOCAL/lib/libgap.so. The re-dlopening the Python module might do it as well.

The other option, as I mentioned, would be to use Python's built-in 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 12 months ago by dimpase

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, absent-mindedly, 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 follow-up: Changed 12 months ago by 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., 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 12 months ago by dimpase

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., is database_gap now part of gap_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 12 months ago by SimonKing

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 follow-up: Changed 12 months ago by dimpase

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 12 months ago by SimonKing

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 follow-up 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 12 months ago by git

  • Commit changed from 3b47f262f0f0db432151826bea079c28b805b07b to eed5de7ffec325d75f08eb8b2b777228e148fabf

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

35abd34Merge branch 'develop' of trac.sagemath.org:sage into gap410pkg
db3f63emore libGAP in as_permutation(), less pexpect mess
2d77eeefollow up pyflake hints (all but one)
7dd5fa6reviewer's example and fix
ca05b24Merge branch 'u/dimpase/groups/better_as_permutation' of trac.sagemath.org:sage into gap410pkg
bd849fbdon't clean too early
ff7722esuppress "#I.." warnings
bf40575silencing of #I warning from GAP in the function body
37e2329tagged # random - answer is always mathematically OK, outputs differ
eed5de7inital support for IO, following Erik on #22626

comment:10 Changed 12 months ago by dimpase

rebased over the branch on #26856, added OSX (and perhaps even Cygwin?) support.

comment:11 Changed 12 months ago by git

  • Commit changed from eed5de7ffec325d75f08eb8b2b777228e148fabf to 932811179edc2f981d55f4af330134a6d3e1fa49

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

5f49332fix sdh_install of symlinks with invalid targets
9854c3aMerge branch 'u/embray/build/sdh-install-symlinks' into public/ticket-26856
963757fdon't clean too early
da38d2ctagged # random - answer is always mathematically OK, outputs differ
d8b912dinital support for IO, following Erik on #22626
0e6f33cIO has landed, no need for this workaround
8bf1044disable doctesting of these modules
a7c5ccdMerge branch 'public/ticket-26856' of trac.sagemath.org:sage into gap_io
c12daa6Merge branch 'u/dimpase/packages/gap_io' of trac.sagemath.org:sage into gap_io
9328111no need to suppress warining

comment:12 Changed 11 months ago by gh-timokau

  • Cc gh-timokau added

comment:13 Changed 11 months ago by git

  • Commit changed from 932811179edc2f981d55f4af330134a6d3e1fa49 to 20c676a5d3fea184e82809366543e486e6aa2c98

Branch pushed to git repo; I updated commit sha1. New commits:

20c676aMerge remote-tracking branch 'trac/develop' into HEAD

comment:14 Changed 11 months ago by dimpase

merged 8.6.rc0 in.

comment:15 Changed 11 months ago by embray

  • Milestone changed from sage-8.6 to sage-8.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 sage-pending or sage-wishlist.

comment:16 Changed 11 months ago by dimpase

  • Status changed from new to needs_review

comment:17 Changed 11 months ago by jdemeyer

Minor comment: if you are changing the spkg-install script of a package, you should also bump the patchlevel to force a rebuild.

comment:18 Changed 11 months ago by jdemeyer

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 11 months ago by jdemeyer

  • Authors set to Dima Pasechnik, Erik Bray

comment:20 Changed 11 months ago by jdemeyer

  • Dependencies #22626, #26856 deleted

comment:21 follow-up: Changed 11 months ago by jdemeyer

  • 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 11 months ago by jdemeyer

Finally a minor Cython style issue: I prefer handle is NULL as a more Pythonic spelling of handle == NULL.

comment:23 Changed 11 months ago by jdemeyer

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 ; follow-up: Changed 11 months ago by 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 sage-on-gentoo 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 11 months ago by dimpase

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 sage-on-gentoo 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 11 months ago by fbissey

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/io-4.5.4/src/io.c

    diff --git a/pkg/io-4.5.4/src/io.c b/pkg/io-4.5.4/src/io.c
    index 5aa4a082..07e6e312 100644
    a b  
    1111/* Try to use as much of the GNU C library as possible: */
    1212#define _GNU_SOURCE
    1313
    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            */
    1616
    1717#undef PACKAGE
    1818#undef PACKAGE_BUGREPORT

comment:27 Changed 11 months ago by dimpase

  • 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

rebased branch, using byte literals now, also internet-related tests fixed at one place


New commits:

13614cfrebased the messy branch from #26930
7796faeusing use byte literals
61afc49change in catergory name (in tests tagged "internet")

comment:28 Changed 11 months ago by jdemeyer

  • Status changed from needs_review to needs_work

17 and 21 have not been addressed yet (neither has 22 but that's just a style issue)

Last edited 11 months ago by jdemeyer (previous) (diff)

comment:29 Changed 11 months ago by jdemeyer

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 11 months ago by embray

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 more-or-less).

comment:31 follow-up: Changed 11 months ago by embray

This change

  • build/pkgs/gap_packages/spkg-install

    diff --git a/build/pkgs/gap_packages/spkg-install b/build/pkgs/gap_packages/spkg-install
    index 1a3e8c5..607d2c7 100644
    a b install_compiled_pkg() 
    4848    # Clean up any build artificts before installing the rest of the package
    4949    # Also remove configure/Makefiles
    5050    # Note: None, if any of the packages really have a proper install target
    51     make clean  # Works for some packages but not all
    52     rm -rf bin/
    53     rm -rf configure configure.* config.* autogen.sh *.m4 Makefile* m4/
    5451
    5552    # Create the bin/ symlink
    5653    ln -s "$SAGE_LOCAL/lib/gap/pkg/$pkg/bin" bin
    5754
    5855    # Install the rest of the package files
    5956    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/
    6060}
    6161
    6262# 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 11 months ago by embray

  • 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:: 
    184184    [ Group( [ f1, f2 ] ), Group( [ f2 ] ), Group( <identity> of ... ) ]
    185185    sage: print(gap.eval("G := SymmetricGroup( 4 )"))
    186186    Sym( [ 1 .. 4 ] )
    187     sage: print(gap.eval("normal := NormalSubgroups( G );"))
     187    sage: print(gap.eval("normal := NormalSubgroups( G );")) # random
    188188    [ Sym( [ 1 .. 4 ] ), Alt( [ 1 .. 4 ] ), Group([ (1,4)(2,3), (1,3)(2,4) ]),
    189189      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 non-determinism in the tests when AFAIK there shouldn't be.

comment:33 follow-up: Changed 11 months ago by 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(): 
    24502450        (416, 100, 36, 20)
    24512451    """
    24522452    from sage.libs.gap.libgap import libgap
    2453     libgap.eval("SetInfoLevel(InfoWarning,0)") # silence #I warnings from GAP (without IO pkg)
    24542453    libgap.LoadPackage("AtlasRep")
    24552454    g=libgap.AtlasGroup("G2(4)",libgap.NrMovedPoints,416)
    2456     libgap.eval("SetInfoLevel(InfoWarning,1)") # restore #I warnings
    24572455    h = Graph()
    24582456    h.add_edges(g.Orbit([1,5],libgap.OnSets))
    24592457    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 sage-the-distribution. E.g. on Debian it's perfectly possible (at least for now) to install the sagemath package without installing the gap-io package.

comment:34 in reply to: ↑ 33 Changed 11 months ago by dimpase

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(): 
    24502450        (416, 100, 36, 20)
    24512451    """
    24522452    from sage.libs.gap.libgap import libgap
    2453     libgap.eval("SetInfoLevel(InfoWarning,0)") # silence #I warnings from GAP (without IO pkg)
    24542453    libgap.LoadPackage("AtlasRep")
    24552454    g=libgap.AtlasGroup("G2(4)",libgap.NrMovedPoints,416)
    2456     libgap.eval("SetInfoLevel(InfoWarning,1)") # restore #I warnings
    24572455    h = Graph()
    24582456    h.add_edges(g.Orbit([1,5],libgap.OnSets))
    24592457    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 sage-the-distribution. E.g. on Debian it's perfectly possible (at least for now) to install the sagemath package without installing the gap-io package.

oh, I might have been confused by shaffling packages around. This chane makes sense if atlasrep is in gap-packages, but this is not the case any more.

comment:35 in reply to: ↑ 31 Changed 11 months ago by dimpase

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 whitespace-only difference there, for it looked totally the same :-(

comment:36 Changed 11 months ago by git

  • Commit changed from 61afc499792c831be0e6e10e60979765a7f48f05 to bce40a554af8ba8a7d17251ce9a5f108143a0da0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0f8a38aTrac #26930: add GAP io package
946393cusing use byte literals
cfb69ccchange in catergory name (in tests tagged "internet")
bce40a5took care of comments 17,22,29,31,33 on Trac #26930

comment:37 Changed 11 months ago by dimpase

What should we do with comment 21? Just patch the GAP/io package source?

comment:38 Changed 11 months ago by git

  • Commit changed from bce40a554af8ba8a7d17251ce9a5f108143a0da0 to 0d7016aa86ca24de03a3c3e7b5698af1e57040ff

Branch pushed to git repo; I updated commit sha1. New commits:

0d7016apatch source of GAP/io to correct header location

comment:39 Changed 11 months ago by dimpase

  • Status changed from needs_work to needs_review

comment:40 Changed 11 months ago by dimpase

  • 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 11 months ago by dimpase

piing...

comment:42 Changed 10 months ago by zerline

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 --with-gaproot=../..

Is it the only way?

comment:43 Changed 10 months ago by embray

  • 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 --with-gaproot 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 follow-up: Changed 10 months ago by dimpase

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 follow-up: Changed 10 months ago by zerline

after a bare ./configure --with-gaproot=../.. :

(sage-sh) odile@ancolie:json-2.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 10 months ago by dimpase

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 10 months ago by dimpase

and if you want more GAP packages installed, modify gap_packages/spkg-install accordingly.

comment:48 follow-up: Changed 10 months ago by 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)

Note: I'm a complete newbie to GAP, not to mention GAP-in-Sage ..

comment:49 Changed 10 months ago by zerline

Thanks anyway, I'll try that.

comment:50 Changed 10 months ago by zerline

Here is my gap_packages-4.10.0.loghttp://fluidinfo.fr/gap/gap_packages-4.10.0.log

Note: I have a crypting-0.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_packages-4.10.0.log.1

Last edited 10 months ago by zerline (previous) (diff)

comment:51 in reply to: ↑ 48 ; follow-up: Changed 10 months ago by 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...

comment:52 in reply to: ↑ 45 ; follow-up: Changed 10 months ago by fbissey

Replying to zerline:

after a bare ./configure --with-gaproot=../.. :

(sage-sh) odile@ancolie:json-2.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 10 months ago by dimpase

  • 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 ; follow-up: Changed 10 months ago by 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.

comment:55 in reply to: ↑ 52 Changed 10 months ago by zerline

Replying to fbissey:

Replying to zerline:

after a bare ./configure --with-gaproot=../.. :

(sage-sh) odile@ancolie:json-2.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

thank you

comment:56 in reply to: ↑ 54 Changed 10 months ago by dimpase

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 10 months ago by dimpase

  • Status changed from needs_review to needs_work

comment:58 Changed 10 months ago by dimpase

  • 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 10 months ago by embray

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_64-unknown-cygwin-default64
GAP_ABI=64
GAP_HPCGAP=no
GAP_ADDGUARDS2=

GAP_BIN_DIR="/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap-4.10.0/src"
GAP_LIB_DIR="/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap-4.10.0/src"

GAP_CC="gcc"
GAP_CFLAGS=" -O2 -g  "
GAP_CPPFLAGS=" -I/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap-4.10.0/src/gen -I/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap-4.10.0/src/src -I/home/embray/src/sagemath/sage/local/var/tmp/sage/build/gap-4.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 10 months ago by dimpase

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 10 months ago by embray

  • 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

This should be a dependent of #27218, not the other way around. I think #27218 will help here. I don't mind including crypting in this ticket as well since it is small and we know that it's been asked for. Plus it helps to have an additional test case.

comment:62 Changed 10 months ago by dimpase

  • Description modified (diff)

comment:63 Changed 10 months ago by embray

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 10 months ago by dimpase

  • Branch changed from public/packages/gap_io to u/dimpase/gap_io
  • Commit changed from 0d7016aa86ca24de03a3c3e7b5698af1e57040ff to 87f59ccc8aa0b1697da8d6f11991bd1236b562e9

New commits:

cc3d6feTrac #27218: Ensure that the same libtool that was used to build GAP is also
5bc1900make similar replacements in a couple more files that reference the GAP build directory
87f59ccrevise trac/public/packages/gap_io to use #27218

comment:65 Changed 10 months ago by embray

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 10 months ago by git

  • Commit changed from 87f59ccc8aa0b1697da8d6f11991bd1236b562e9 to 3a65f9e38b16c1c85fb581a327fa7855c613df68

Branch pushed to git repo; I updated commit sha1. New commits:

51aa946repackages the code for finding the libSingular.so as a somewhat
34b27b2rework _get_shared_lib_filename to cover more system-specific cases,
917a21bfixed the test to be a little more flexible
785bf10Merge branch 'u/embray/misc/sage-env-lib-filename' of trac.sagemath.org:sage into HEAD
3a65f9euse GAP_SO in the dlopen call

comment:67 Changed 10 months ago by dimpase

  • Dependencies changed from #27218 to #27218, #27230
  • Status changed from needs_work to needs_review

once #27230 done it will be ready for review

Last edited 10 months ago by dimpase (previous) (diff)

comment:68 Changed 10 months ago by git

  • Commit changed from 3a65f9e38b16c1c85fb581a327fa7855c613df68 to 0d7e1e81708a8543c4bb5d10ff4a00159c2a7414

Branch pushed to git repo; I updated commit sha1. New commits:

0d7e1e8add setting GAP_SO to env.py

comment:69 Changed 10 months ago by dimpase

so this is now really ready for review.

comment:70 follow-up: Changed 10 months ago by gh-timokau

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 10 months ago by dimpase

Replying to gh-timokau:

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 10 months ago by git

  • Commit changed from 0d7e1e81708a8543c4bb5d10ff4a00159c2a7414 to a2f76b272ef3e8626468257b34e05484cf4b82bd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4556593Trac #27218: Ensure that the same libtool that was used to build GAP is also
44d20c1repackages the code for finding the libSingular.so as a somewhat
1eebc0drework _get_shared_lib_filename to cover more system-specific cases,
ad3410bfixed the test to be a little more flexible
691eeb8rebased over 8.7.beta3 and the dependent tickets
a2f76b2add setting GAP_SO to env.py

comment:73 Changed 10 months ago by git

  • Commit changed from a2f76b272ef3e8626468257b34e05484cf4b82bd to 99c0afca419ea98ffa31762a21425c4591f98a83

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2456f8frebased over 8.7.beta3 and the dependent tickets
99c0afcadd setting GAP_SO to env.py

comment:74 Changed 10 months ago by git

  • Commit changed from 99c0afca419ea98ffa31762a21425c4591f98a83 to 4927c10700abe459289512245549ce035d0d9f52

Branch pushed to git repo; I updated commit sha1. New commits:

db932f0extra / breaks sdh_install on OSX
4927c10Merge 'u/dimpase/gap_io' with fix from #27380

comment:75 Changed 10 months ago by dimpase

  • Dependencies changed from #27218, #27230 to #27218, #27230, #27380

comment:76 Changed 10 months ago by dimpase

#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 10 months ago by dimpase

Tests pass on OSX and Linux. Please review.

comment:78 Changed 10 months ago by dimpase

  • Cc jhpalmieri andrew.mathas added

OSX-specific reviews are needed for this ticket and #27380

comment:79 Changed 10 months ago by embray

Is there a reason this first "installs" crypting by copy in

  • build/pkgs/gap_packages/spkg-install

    diff --git a/build/pkgs/gap_packages/spkg-install b/build/pkgs/gap_packages/spkg-install
    index 1a3e8c5..07ce727 100644
    a b sdh_install \ 
    1515    AutoDoc-* \
    1616    corelg \
    1717    crime-* \
     18    crypting-* \
    1819    cryst \
    1920    crystcat \
    2021    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 follow-up: Changed 10 months ago by dimpase

looks like a harmless typo...

comment:81 Changed 10 months ago by embray

Any comment on comment:32 ? This test really shouldn't be giving unpredictable results.

comment:82 in reply to: ↑ 80 Changed 10 months ago by embray

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 10 months ago by git

  • Commit changed from 4927c10700abe459289512245549ce035d0d9f52 to 1a2bb6daa62b266a8463ab1f1f40b3a3b4024b12

Branch pushed to git repo; I updated commit sha1. New commits:

1a2bb6donly install crypting once

comment:84 Changed 10 months ago by git

  • Commit changed from 1a2bb6daa62b266a8463ab1f1f40b3a3b4024b12 to 61600394e43bdefbc918fa03a817311b39af4a05

Branch pushed to git repo; I updated commit sha1. New commits:

6160039removed #random tag, as it's hopefully fixed

comment:85 Changed 10 months ago by dimpase

Fixed both things.

comment:86 follow-up: Changed 10 months ago by jhpalmieri

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 10 months ago by dimpase

Replying to jhpalmieri:

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.

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 10 months ago by jhpalmieri

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

comment:90 in reply to: ↑ 89 Changed 10 months ago by dimpase

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 10 months ago by embray

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/site-packages/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/site-packages/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/site-packages/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/guava-3.14/bin/x86_64-unknown-cygwin-default64/wtdist', '/home/embray/.sage/temp/NAVI-Brick/6764/tmp_7ZlhoX::code']' returned non-zero 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 10 months ago by dimpase

we didn't build this part of guava before, that's why...

comment:93 Changed 10 months ago by embray

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 10 months ago by embray

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 10 months ago by embray

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 10 months ago by dimpase

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 follow-up to this is #27295.

comment:97 Changed 10 months ago by embray

  • 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

Okay, I'm fine with this now, especially since you tested on OSX.


New commits:

ece3d59extra / breaks sdh_install on OSX
77a2f32Trac #26930: add setting GAP_SO to env.py
985e63aTrac: #26930: add GAP IO and crypting packages
bb77ab1Trac #26903: Bump package version

comment:98 Changed 10 months ago by git

  • Commit changed from bb77ab1b14ac08afebb95a08a4981230b6809d5b to c84753f36265886e31cbf9d7c31745d176a907c8
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

7e85cc6Trac #26930: add GAP IO and crypting packages
c84753fTrac #26903: Bump package version

comment:99 Changed 10 months ago by embray

  • Status changed from needs_review to positive_review

Just reworded one of the commit messages.

comment:100 Changed 9 months ago by vbraun

  • Branch changed from public/gap_io to c84753f36265886e31cbf9d7c31745d176a907c8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.