Opened 3 years ago

Closed 12 months ago

Last modified 11 months ago

#22626 closed enhancement (fixed)

Upgrade to GAP 4.10

Reported by: nthiery Owned by:
Priority: critical Milestone: sage-8.6
Component: interfaces Keywords: days85, libgap
Cc: alexk, dimpase, embray, fbissey, arojas, gh-sebasguts, jpflori, markuspf, nthiery, slelievre, vbraun, wstein, gh-timokau Merged in:
Authors: Nicolas M. Thiéry, Dima Pasechnik, Erik Bray, Jeroen Demeyer Reviewers: Erik Bray, Dima Pasechnik, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: b446ebb (Commits) Commit:
Dependencies: #26874 Stopgaps: error handling in libgap, documentation display

Description (last modified by embray)

GAP 4.10 comes with a completely rewritten build system that will simplify our packaging. In particular, libGAP no longer needs to be a separate package.

What the branch does:

  • Remove the libgap spkg
  • Update the gap spkg to the new build system and build and install libgap
  • Replace gap.shi.patch by a plain gap startup script for Sage

Rationale: GAP used to provide a startup shell script. The GAP devs are in the process of getting rid of it and provide a very minimal one. They recommend to just write our own rather than patching it.

  • Update a few doctests w.r.t. changes of output of some GAP functions
  • Reworks how gap is installed in $SAGE_LOCAL: Rather than installing the entire source tree we just install the gap and libgap binaries to standard locations, and add a $SAGE_LOCAL/share/gap containing the GAP_ROOT_DIR which is stripped down to the minimum needed for GAP to work (standard libs and packages, docs, as well as the source code for introspection of kernel functions, but all build artifacts are carefully omitted).
  • Some of how this is done is still broken w.r.t. building compiled packages; need to work on it a bit more.
  • - Possibly controversial: The new libgap currently 'does not come' with symbol rewriting (foobar -> libGAP_foobar). This avoids messing around with GAP's sources; in particular opening the door for using a stock GAP from the OS distribution. However there always is a risk of name conflicts. And indeed, GAP's enums T_INT, T_FLOAT, ... conflict with Python's constants defined in structmember.h. This is hopefully not actually a problem in practice due to the way how Cython orders includes.

Something similar was started by Volker at #19915.

  • Removes configure.patch: it was patching configure for better GMP detection under Cygwin (#13954). This should not be needed anymore with the new build system and use of --with-gmp. If it is, upstream asked for it to be reported and they will fix it.
  • Removes additional patches for now--we would like to focus on being able to work with a system GAP as much as possible.
  • Revert #19726 (not needed anymore)

Status

Currently broken - crashes deep inside GAP error handling system after few simple commands.

Basic tests on libgap:

sage: libgap.eval("GAPInfo.Version")
sage: libgap.DihedralGroup(10).CharacterTable()
CharacterTable( <pc group of size 10 with 2 generators> )
sage: libgap.Group(libgap.eval("[(1,2,3),(1,2)]")).Size()
6

Running most relevant tests:

sage -tp 8 sage/groups sage/libs/gap

Current status: lots of errors

  • Still have some miscellaneous segfaults and other weird crashes
  • Still have work to do on improving error handling; replacing the built-in ErrorInner function might help here.
  • SIGINT handling by Python is broken by GAP_Intialize; need to work around this.

Testing packages with dynamic loading (e.g. IO):

Install IO:

cd $SAGE_LOCAL/share/gap/pkg
wget https://www.gap-system.org/pub/gap/gap4/tar.gz/packages/io-4.5.4.tar.gz
tar xvf io-4.5.4.tar.gz
cd io-4.5.4
./configure --with-gaproot=../..
make

Test it locally:

cd $SAGE_ROOT
gap
gap> LoadPackage("IO");
true

This does not yet work:

#W dlopen() error: /home/embray/src/sagemath/sage/local/share/gap/pkg/io-4.5.4/bin/x86_64-pc-linux-gnu-default64/io.so: undefine\
d symbol: ChangedBags
Error, module '/home/embray/src/sagemath/sage/local/share/gap/pkg/io-4.5.4/bin/x86_64-pc-linux-gnu-default64/io.so' not found
Error, was not in any namespace
Syntax warning: Unbound global variable in /home/embray/src/sagemath/sage/local/share/gap/lib/init.g:803
    ColorPrompt( UserPreference( "UseColorPrompt" ) );
               ^
Error, SetGasmanMessageStatus: function is not yet defined
true

This should be fixed once GAP's gap binary is built on top of libgap. See: https://github.com/markuspf/gap/issues/1 I believe this is fixed, but there are still some problems with the way this ticket is "installing" GAP for $SAGE_LOCAL.

Note:

  • Max Horn reviewed the list of GAP symbols we use in Sage; some have already changed in 4.9. See this pad for notes.

Upstream PRs

A few pull requests we made to improve use of GAP as a library:

Other open issues that don't have PRs yet:

Some other PRs useful to this effort:

The update of gap_packages and database_gap is on #26856

Tarball: https://www.gap-system.org/pub/gap/gap-4.10/tar.gz/gap-4.10.0.tar.gz

Attachments (2)

ptestlong.log (298.5 KB) - added by nthiery 3 years ago.
groups.patch (8.9 KB) - added by dimpase 12 months ago.
patches for src/sage/groups/

Download all attachments as: .zip

Change History (473)

comment:1 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:2 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:3 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:4 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:5 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:6 Changed 3 years ago by nthiery

  • Branch set to u/nthiery/upgrade_to_gap_4_9

comment:7 Changed 3 years ago by git

  • Commit set to 3011ac0908d667c0f245ca21859e336511106b5f

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

1c645e0#22626: remove GAP's symbol prefixing in libgap: libGAP_Foo -> Foo
1ecc67b#22626: doctest update w.r.t. minor changes of output in GAP
e8ebbca#22626: GMP detection patch for cygwin should not be needed anymore
fd65b06#22626: Remove libgap spkg
9511733#22626: replace patch for GAP's startup script template in favor of a custom script
550625e#22626: remove GAP's symbol prefixing in libgap: libGAP_Foo -> Foo, and workaround GAP <-> Python symbol conflict
a9c859c#22626: updated gap spkg w.r.t. GAP's devel version and its new build system; also include compilation and installation of libgap
3011ac0Merge branch 'develop' into t/22626/upgrade_to_gap_4_9

comment:8 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:9 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:10 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:11 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:12 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:13 Changed 3 years ago by nthiery

  • Cc vbraun dimpase wstein added
  • Keywords days85 libgap added
  • Work issues set to Wait for gap 4.9 release

comment:14 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:15 Changed 3 years ago by fbissey

  • Cc fbissey added

comment:16 Changed 3 years ago by git

  • Commit changed from 3011ac0908d667c0f245ca21859e336511106b5f to 234c54b4b7e0495e343c3ab1b925e2c99f37e391

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

234c54b#22626: revert #19726 as it won't be needed for gap 4.9

comment:17 Changed 3 years ago by nthiery

  • Description modified (diff)

Changed 3 years ago by nthiery

comment:18 Changed 3 years ago by nthiery

  • Description modified (diff)

comment:19 Changed 3 years ago by git

  • Commit changed from 234c54b4b7e0495e343c3ab1b925e2c99f37e391 to 431845f6da27d20c78b5e7a42b5f47bea866201c

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

b301e4c#22626: ReST fix
431845f#22626: better work around for the GAP-Python name clash on T_INT, ...

comment:20 Changed 3 years ago by nthiery

  • Description modified (diff)

New commits:

b301e4c#22626: ReST fix
431845f#22626: better work around for the GAP-Python name clash on T_INT, ...

comment:21 Changed 3 years ago by git

  • Commit changed from 431845f6da27d20c78b5e7a42b5f47bea866201c to 7c04025083d61cab671df3302c32f353c4e28313

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

7c0402522626: document the work around for the GAP-Python name clash on T_INT, ...

comment:22 follow-ups: Changed 3 years ago by fbissey

So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code. Do we have an idea on when we'll have just have gap linking on libgap which would also solve this particular problem?

comment:23 in reply to: ↑ 22 Changed 3 years ago by nthiery

Salut François,

Replying to fbissey:

So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code.

Exactly.

Do we have an idea on when we'll have just have gap linking on libgap which would also solve this particular problem?

The GAP developers are well aware of that and planning to implement it. They have not yet done yet just to be more incremental; their new build system is a big PR already :-) I would assume that this will be done before GAP 4.9 which they are planning for a couple months from now.

See: https://github.com/markuspf/gap/issues/2

Btw: feel free to comment / expand on the list there. I'll advertise this issue for additional feedback on the sage-packaging mailing list.

comment:24 in reply to: ↑ 22 ; follow-ups: Changed 3 years ago by jdemeyer

Replying to fbissey:

So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code.

For this reason, I wonder why you didn't keep the separation GAP + libGAP in Sage too. If it effectively behaves as two packages, it seems more natural to keep it as two packages in Sage too. Imagine for example that we need to patch GAP but not libGAP or conversely, that would be harder with the current approach. That being said, this is mostly bikeshedding. So, if the current setup works well, there might be no reason to change it.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 3 years ago by dimpase

Replying to jdemeyer:

Replying to fbissey:

So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages" with the same source code.

For this reason, I wonder why you didn't keep the separation GAP + libGAP in Sage too. If it effectively behaves as two packages, it seems more natural to keep it as two packages in Sage too. Imagine for example that we need to patch GAP but not libGAP or conversely, that would be harder with the current approach. That being said, this is mostly bikeshedding. So, if the current setup works well, there might be no reason to change it.

Two completely separate packages would also mean that gap_packages should come in two different flavours, etc. I'd rather keep it simple and do not multiply these instances (but rather hope that libGAP will support all of GAP packages soon).

comment:26 in reply to: ↑ 25 ; follow-up: Changed 3 years ago by jdemeyer

Replying to dimpase:

Two completely separate packages would also mean that gap_packages should come in two different flavours, etc.

So, you are saying that we need to install gap_packages twice too? Once for GAP and once for libGAP?

comment:27 in reply to: ↑ 24 Changed 3 years ago by nthiery

Replying to jdemeyer:

For this reason, I wonder why you didn't keep the separation GAP + libGAP in Sage too.

I wanted to experiment with how far we were from having a proper single package, so as to give early feedback to the gap developers on any sticking points. And indeed, linking gap to libgap is basically the only sticking point. Since this point will be most likely resolved by the time this ticket gets merged in, we might as well shoot directly for the "right thing".

comment:28 in reply to: ↑ 26 ; follow-up: Changed 3 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

Two completely separate packages would also mean that gap_packages should come in two different flavours, etc.

So, you are saying that we need to install gap_packages twice too? Once for GAP and once for libGAP?

no, not at all---I mean to say that there exist GAP packages (not part of Sage ATM) that break libGAP now. As some of these packages are very useful, it's important to have this fixed (but not at expense of having 2 separate gap_packages).

comment:29 in reply to: ↑ 28 Changed 3 years ago by nthiery

Replying to dimpase:

As some of these packages are very useful, it's important to have this fixed

YES. In fact that is my main motivation to work on the libgap integration: I badly want to use Semigroups that requires IO! We went through the details with Markus and Max, and the shared aim is indeed to have this resolved with the upgrade to 4.9.

comment:30 Changed 3 years ago by jpflori

  • Cc jpflori added

comment:31 Changed 2 years ago by jdemeyer

Is there any news here?

comment:32 Changed 22 months ago by slelievre

  • Cc slelievre added

GAP 4.9.0, a beta release for GAP 4.9, has been released.

This means we can start working on building Sage with it.

comment:33 follow-up: Changed 22 months ago by dimpase

Do you know if they now provide a replacement for libGAP?

comment:34 Changed 22 months ago by slelievre

  • Cc alexk markuspf added
  • Description modified (diff)

Cc-ing @alexk and @markuspf.

comment:35 in reply to: ↑ 33 Changed 22 months ago by nthiery

Replying to dimpase:

Do you know if they now provide a replacement for libGAP?

In principle yes: the branch I posted here last April was using their stock alpha version. Now the next step is to revive the branch and adapt it if needed.

I can work on this, but probably not at once. Any volunteer to take over welcome.

Markus: do you foresee any major changes in how GAP's lib is to be built that could affect what I was doing last April?

comment:36 follow-up: Changed 22 months ago by dimpase

I just noticed that in 4.9 smallgrps and transgrps are released under a GPL-compatible license. This is great! We really should make them (i.e. database_gap) standard Sage package, and do away with the extra hurdle of having database_gap optional.

comment:37 follow-up: Changed 22 months ago by jdemeyer

If you're willing to wait 2 months, this might be a good task for the Cernay workshop https://github.com/OpenDreamKit/OpenDreamKit/issues/251

comment:38 in reply to: ↑ 37 Changed 22 months ago by nthiery

Replying to jdemeyer:

If you're willing to wait 2 months, this might be a good task for the Cernay workshop https://github.com/OpenDreamKit/OpenDreamKit/issues/251

Yes indeed; it will be helpful to have GAP people under hand. I may just become impatient before that for some research project of mine, in which case I'll have a head start :-)

comment:39 in reply to: ↑ 36 Changed 22 months ago by nthiery

Replying to dimpase:

I just noticed that in 4.9 smallgrps and transgrps are released under a GPL-compatible license. This is great! We really should make them (i.e. database_gap) standard Sage package, and do away with the extra hurdle of having database_gap optional.

Yes! I can't wait for all the simplifications this will bring to us!

comment:40 in reply to: ↑ 22 ; follow-up: Changed 22 months ago by jdemeyer

Replying to fbissey:

So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages".

With the past and future modifications to the Sage build system in mind, I now object more strongly than before to treating two separate packages (GAP + libGAP) as one package.

The reason is that we now try to separate the build and install stages of packages. But that is incompatible with the recipe here of first "build and install GAP" and then "build and install libGAP".

comment:41 Changed 22 months ago by jdemeyer

For an example of two Sage packages with the same sources, see gcc and gfortran. IMHO, you should do the same for GAP and libGAP (unless they are merged upstream but my impression is that this has not happened yet).

comment:42 in reply to: ↑ 40 Changed 22 months ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

So if I understand the mechanics involved, at the moment you have to configure and install gap. Then clean and configure and install for libgap. So it is still behaving like "two packages".

With the past and future modifications to the Sage build system in mind, I now object more strongly than before to treating two separate packages (GAP + libGAP) as one package.

The reason is that we now try to separate the build and install stages of packages. But that is incompatible with the recipe here of first "build and install GAP" and then "build and install libGAP".

Hopefully we will be getting something more serious at final release time, like with pari where you build libpari and then the gp executable. That would be the ideal scenario where it is really one package. Have to see how far they have gone with 4.9.

comment:43 Changed 22 months ago by jdemeyer

A lot of changes on this ticket (also the changes likely to lead to merge conflicts) are related to the unprefixing of libGAP symbols. To ease development, maybe we should try to do that separately from the upgrade to GAP 4.9.

I was thinking to do something like #19915 but changing only the Sage source code (not libGAP) and undoing the prefixing with macros like #define SomeGapFunction libGAP_SomeGapFunction. What do you think?

comment:44 follow-up: Changed 22 months ago by dimpase

there is also libgap unprefixing in Python/Cython? that has to be done. Perhaps separating these from C level unprefixing would help making it smoother.

comment:45 in reply to: ↑ 44 ; follow-up: Changed 22 months ago by jdemeyer

Replying to dimpase:

there is also libgap unprefixing in Python/Cython? that has to be done. Perhaps separating these from C level unprefixing would help making it smoother.

Yes, that is what I meant. Creating a separate ticket for unprefixing only at the Cython level by using macros to translate between the unprefixed names in Cython and the prefixed names in libGAP.

comment:46 in reply to: ↑ 45 ; follow-up: Changed 22 months ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

there is also libgap unprefixing in Python/Cython? that has to be done. Perhaps separating these from C level unprefixing would help making it smoother.

Yes, that is what I meant. Creating a separate ticket for unprefixing only at the Cython level by using macros to translate between the unprefixed names in Cython and the prefixed names in libGAP.

I am not sure I understand. Currently in Cython on can do something like

    from sage.libs.gap.libgap import libgap
    g=libgap.ProjectiveGeneralLinearGroup(3,3)

How will this change under what you propose?

comment:47 in reply to: ↑ 46 Changed 22 months ago by jdemeyer

Replying to dimpase:

    from sage.libs.gap.libgap import libgap
    g=libgap.ProjectiveGeneralLinearGroup(3,3)

How will this change under what you propose?

The Python interface won't change at all. I am talking about how sage.libs.gap calls libGAP.

comment:48 Changed 21 months ago by jdemeyer

As far as I can tell, GAP 4.9 has not been released. Does anybody know how close we are to an actual release?

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

comment:49 Changed 21 months ago by slelievre

I think GAP 4.9.0 is considered a public beta for GAP 4.9.

The list of releases at

still displays GAP 4.8.10 as the latest release.

In the list of past releases there, the GAP 4.8, 4.7, 4.6, 4.5 series start at GAP 4.8.2, 4.7.2, 4.6.2, 4.5.4, so probably GAP 4.x.y with those x and lower y were considered beta.

Still, seeing how 4.9.0 works with Sage would be nice.

comment:50 Changed 20 months ago by jdemeyer

  • Dependencies set to #25273

comment:51 follow-up: Changed 20 months ago by jdemeyer

Where is the GAP source tarball which is supposed to be used here?

comment:52 Changed 20 months ago by jdemeyer

  • Branch changed from u/nthiery/upgrade_to_gap_4_9 to u/jdemeyer/upgrade_to_gap_4_9

comment:53 Changed 20 months ago by git

  • Commit changed from 7c04025083d61cab671df3302c32f353c4e28313 to b7278a120c5db710d1e11297b3dd1411d69d302b

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

72b5066Unprefixed libGAP interface
b7278a1Upgrade to GAP 4.9

comment:54 in reply to: ↑ 51 Changed 20 months ago by dimpase

Replying to jdemeyer:

Where is the GAP source tarball which is supposed to be used here?

here: https://www.gap-system.org/pub/gap/gap-4.9/beta/

comment:55 Changed 20 months ago by jdemeyer

  • Description modified (diff)

comment:56 in reply to: ↑ description Changed 20 months ago by jdemeyer

Replying to nthiery:

The branch attached to this ticket updates Sage to run on top of a branch of GAP by Markus Pfeiffer that adds libgap compilation and will be merged soon in the devel version of GAP.

It seems that this branch which "will be merged soon" is still not merged. It would be good to have an idea of whether or not that will happen.

comment:57 Changed 20 months ago by git

  • Commit changed from b7278a120c5db710d1e11297b3dd1411d69d302b to 40e644a16bafa77e4b93e72c215f53a4afb19cc1

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

d28b575Unprefixed libGAP interface
daf8754Unprefixed libGAP interface
40e644aUpgrade to GAP 4.9

comment:58 Changed 20 months ago by nthiery

According to https://github.com/gap-system/gap/pull/1205#issuecomment-385670578, Markus's libgap branch won't be merged in GAP 4.9: it's not yet ready enough. Bummer.

We are discussing with Sebastian and Thomas what we can do to move forward here. They are very interested as well by libgap for the integration between GAP and Julia.

Tentative steps:

  • As a warm up, upgrade our current gap and libgap spkg to 4.8.10
  • Upgrade our current gap and libgap spkg to 4.9; with the many changes to the build system, this may get more tricky
  • To make the switch to GAP's libgap less urgent, investigate whether we could change our current libgap spkg to not prefix GAP symbols, in order to be able to use GAP's dynamic packages; see #25273
  • On the GAP side: move toward more incremental changes. In particular, extract the changesets from Markus branch that are non controversial and ready, and get them merged in GAP (4.9? 4.10?)
Last edited 20 months ago by nthiery (previous) (diff)

comment:59 Changed 20 months ago by nthiery

  • Cc ghsebasguts added

comment:60 Changed 19 months ago by gh-antonio-rojas

  • Cc gh-antonio-rojas added

comment:61 Changed 19 months ago by slelievre

  • Description modified (diff)
  • Milestone changed from sage-8.0 to sage-8.3

GAP 4.9.1 was released in May 2018.

This is the first stable release of GAP 4.9 (GAP 4.9.0 was a beta for GAP 4.9).

comment:62 Changed 19 months ago by slelievre

  • Cc embray gh-sebasguts nthiery added; ghsebasguts removed

comment:63 Changed 19 months ago by gh-antonio-rojas

I'm confused - I just built GAP 4.9.1 from the upstream tarball and 'make install' *does* install /usr/lib/libgap.so and libgap's headers. Is this a different libgap than the one mentioned here to be included in 4.10?

comment:64 Changed 19 months ago by slelievre

One comment at pull request 1205 in GAP's github repo said GAP won't ship libGAP before GAP 4.10:

Maybe this was changed though.

comment:65 Changed 18 months ago by nthiery

  • Description modified (diff)

comment:66 follow-up: Changed 18 months ago by jdemeyer

Has anybody asked GAP developers for a policy on the prefixing?

In Cernay, I proposed that GAP should prefix some/all of their names like T_INT -> GAP_T_INT. For compiling GAP (+ GAP packages) itself, we can use a file with defines like

#define  T_INT  GAP_T_INT

to ensure backwards compatibility. This way, in GAP itself, one could use either T_INT or GAP_T_INT. Outside of GAP, only GAP_T_INT would work.

I think this is relatively easy to implement (I'm willing to do it) but I don't know what upstream GAP thinks of it.

comment:67 Changed 18 months ago by arojas

  • Cc arojas added; gh-antonio-rojas removed

comment:68 Changed 17 months ago by slelievre

  • Description modified (diff)

GAP 4.9.2 was released in July 2018.

comment:69 Changed 17 months ago by gh-timokau

  • Cc gh-timokau added

comment:70 Changed 17 months ago by nthiery

Hi,

For the record: I experimented a bit a month ago and a bit more this morning with Dima with the latest gap master that has make libgap and make install-libgap rules. See the following branch, which builds libgap from GAP's pristine sources:

https://git.sagemath.org/sage.git/log/?h=u/nthiery/gap-libgap

Note that, at this stage, Sage won't start with it; maybe even not compile.

Last edited 17 months ago by nthiery (previous) (diff)

comment:71 in reply to: ↑ 66 Changed 17 months ago by nthiery

Has anybody asked GAP developers for a policy on the prefixing?

In Cernay, I proposed that GAP should prefix some/all of their names like T_INT -> GAP_T_INT. For compiling GAP (+ GAP packages) itself, we can use a file with defines like

#define  T_INT  GAP_T_INT

to ensure backwards compatibility. This way, in GAP itself, one could use either T_INT or GAP_T_INT. Outside of GAP, only GAP_T_INT would work.

I think this is relatively easy to implement (I'm willing to do it) but I don't know what upstream GAP thinks of it.

That's the idea indeed. Eventually they will provide a standalone header file that will define the official GAP API, and where all symbols will be prefixed. To bootstrap the process, we (well mostly Max!) reviewed and annotated the symbols that are used in Sage:

https://hackmd.io/emNi76svSWCh1fBeLKqPdA#

comment:72 Changed 17 months ago by gh-timokau

Thank you for working on this! Sage using an up-to-date gap and doing away with the additional libgap package will simplify packaging.

comment:73 Changed 16 months ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:74 Changed 15 months ago by slelievre

GAP 4.9.3 was released.

comment:75 Changed 15 months ago by dimpase

  • Description modified (diff)
  • Summary changed from Upgrade to GAP 4.9 to Upgrade to GAP 4.10
  • Work issues changed from Wait for gap 4.9 release to Wait for gap 4.10 release

comment:76 Changed 15 months ago by dimpase

  • Description modified (diff)

comment:77 Changed 15 months ago by dimpase

  • Description modified (diff)

note that the prefix of the tarball should start with src/ - else one gets weird error messages from the python's tar.

GAPs make install target now works in a preliminary form - on Linux for sure.

comment:78 Changed 15 months ago by dimpase

  • Branch changed from u/jdemeyer/upgrade_to_gap_4_9 to u/dimpase/WIP/libgap410
  • Commit changed from 40e644a16bafa77e4b93e72c215f53a4afb19cc1 to f06912253d01557727041fe3b4d03e459fb90a11

This is WIP - builds, but libgap.pyx interaction loop has to be rewritten using new API.

Also, sage --gap is broken

gap: hmm, I cannot find 'lib/init.g' maybe use option '-l <gaproot>'?

New commits:

c766ad6Merge branch 'u/jdemeyer/upgrade_to_gap_4_9' of trac.sagemath.org:sage into libgap
5152637building GAP and libGAP from GAP
6bed83achanges for using libGAP from GAP - part 1
f069122removing util*

comment:79 Changed 15 months ago by dimpase

oops, one still needs initialize() from util.pyx, in some form...

comment:80 Changed 15 months ago by git

  • Commit changed from f06912253d01557727041fe3b4d03e459fb90a11 to 9c22e727f7a87271c26885e9cdc9b52583f598c5

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

8c28e13Revert "removing util*"
9c22e72few comments

comment:81 Changed 15 months ago by nthiery

Great to see the progress! Sounds like a productive week in Siegen. Thanks so much.

comment:82 Changed 15 months ago by dimpase

  • Description modified (diff)

comment:83 Changed 15 months ago by git

  • Commit changed from 9c22e727f7a87271c26885e9cdc9b52583f598c5 to 14919d3d32074d36ece1da26815e5f5d7a24812b

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

14919d3update util and the GAP package

comment:84 Changed 15 months ago by git

  • Commit changed from 14919d3d32074d36ece1da26815e5f5d7a24812b to 8b5ccbe6323feb8ded8cf71cf1d00faa71da59c5

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

b3dc304Merge branch 'develop' into libgap
8a8a146use MarkBag instead of MARK_BAG
c5a5d4fno more *_BAG, and a call signature fix
d13fc80more compilation fixes
8b5ccberemoved debugging utils (they need a rewrite)

comment:85 Changed 15 months ago by git

  • Commit changed from 8b5ccbe6323feb8ded8cf71cf1d00faa71da59c5 to a0616deb562afcf9d5b1a8a4e02406b4a0479038

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

a0616deadding --nointeract to libgap startup options

comment:86 Changed 15 months ago by dimpase

at this point I am at

sage: libgap(1)
Error, Variable: '_rich_repr_' must have a value
Error, Variable: '_ipython_canary_method_should_not_exist_' must have a\
 value
[... several more errors like above]
gap: panic, could not open *errout*!

comment:87 Changed 15 months ago by git

  • Commit changed from a0616deb562afcf9d5b1a8a4e02406b4a0479038 to 9cc327f6004c1a16819774fd34cf5fc48ff4a001

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

9cc327fadding --nointeract to libgap startup options

comment:88 follow-up: Changed 15 months ago by dimpase

In the current branch I am able to run

sage: g=libgap.SymmetricGroup(4)
sage: print(g.Order())
24
sage: i=g.Order().sage()
sage: i
24

So it's outputting GAP object that is broken ATM.

I'll try fixing the "old" GAP interface, perhaps it will help.

comment:89 in reply to: ↑ 88 ; follow-up: Changed 15 months ago by nthiery

Replying to dimpase:

In the current branch I am able to run

sage: g=libgap.SymmetricGroup(4)
sage: print(g.Order())
24
sage: i=g.Order().sage()
sage: i
24

Nice! Making good progress!

So it's outputting GAP object that is broken ATM. I'll try fixing the "old" GAP interface, perhaps it will help.

I am not sure what's broken above?

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

Replying to nthiery:

So it's outputting GAP object that is broken ATM. I'll try fixing the "old" GAP interface, perhaps it will help.

I am not sure what's broken above?

see comment:86 - displaying (lib)GAP objects is broken. Also,

sage: gap(1)
** Gap crashed or quit executing 'SetUserPreference("HistoryMaxLines", 30);' **
Restarting Gap and trying again
** Gap crashed or quit executing '\$sage1:=1;;' **
...

the latter is probably due to some scripts gotten messed up by the upgrade.

And error handling in libgap interface needs work, too.

comment:91 Changed 15 months ago by dimpase

libgap's display problem seems to be ipython-specific. Indeed

$ ./sage --python
Python 2.7.15 (default, Sep 15 2018, 05:05:36) 
[GCC 8.2.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from sage.all import *
>>> libgap(1)
1

comment:92 Changed 15 months ago by dimpase

Some discussions related to libgap API:

and an accepted PR to fix a bug that was breaking libgap/Sage interface:

https://github.com/gap-system/gap/pull/2840

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

comment:93 Changed 15 months ago by git

  • Commit changed from 9cc327f6004c1a16819774fd34cf5fc48ff4a001 to ece8cfc39e5f4258a5ec2c8dc2a307f988218891

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

ece8cfcfix gap() interface and gap_console, removed some old stuff

comment:94 Changed 15 months ago by git

  • Commit changed from ece8cfc39e5f4258a5ec2c8dc2a307f988218891 to 043fb95c1ee6360ef5df9ac879d643a662bf6dbc

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

043fb95fixing all but one doctests in interfaces/gap.py

comment:95 Changed 15 months ago by dimpase

One error - tab completion does not work on GAP objects:

sage -t --long --warn-long 49.3 src/sage/interfaces/gap.py
**********************************************************************
File "src/sage/interfaces/gap.py", line 1628, in sage.interfaces.gap.GapElement._tab_completion
Failed example:
    'Centralizer' in s5._tab_completion()
Exception raised:
    Traceback (most recent call last):
      File "/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 659, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1070, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.interfaces.gap.GapElement._tab_completion[1]>", line 1, in <module>
        'Centralizer' in s5._tab_completion()
      File "sage/misc/cachefunc.pyx", line 2316, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:13467)
        self.cache = f(self._instance)
      File "/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 1633, in _tab_completion
        v = P.eval(r'\$SAGE.OperationsAdmittingFirstArgument(%s)'%self.name())
      File "/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 583, in eval
        result = Expect.eval(self, input_line, **kwds)
      File "/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1354, in eval
        for L in code.split('\n') if L != ''])
      File "/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 789, in _eval_line
        raise RuntimeError(message)
    RuntimeError: Gap produced error output
    Error, no method found! For debugging hints type ?Recovery from NoMethodFound
    Error, no 1st choice method found for `Iterator' on 1 arguments

       executing \$SAGE.OperationsAdmittingFirstArgument(x);
**********************************************************************
1 item had failures:
   1 of   3 in sage.interfaces.gap.GapElement._tab_completion

comment:96 Changed 15 months ago by dimpase

  • Stopgaps set to tab completion on GAP objects, error handling in libgap

comment:97 Changed 15 months ago by git

  • Commit changed from 043fb95c1ee6360ef5df9ac879d643a662bf6dbc to 0128112b501a6c66d21b570dde09526ecf63c9b1

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

0128112few important Cython types fixed

comment:98 Changed 15 months ago by git

  • Commit changed from 0128112b501a6c66d21b570dde09526ecf63c9b1 to 732cbdd54e24ac35c3fd52c75fb7608ad41603ce

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

c4d128epass the correct error callback function
732cbddremoving libgap_enter/exit

comment:99 Changed 15 months ago by dimpase

in gap.* interface, tab completion is broken due to outdated code in src/ext/gap/sage.g: with GAP 4.10 one gets

gap> Read("sage.g");
gap> s5:=SymmetricGroup(5);
Sym( [ 1 .. 5 ] )
gap> \$SAGE.OperationsAdmittingFirstArgument(s5);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `Iterator' on 1 arguments at /mnt/opt/Sage/sage-dev/local/gap/latest/lib/methsel2.g:250 called from
OPERATIONS[i + 1] at sage.g:69 called from
<function "LastReadValue">( <arguments> )
 called from read-eval loop at *stdin*:4
type 'quit;' to quit to outer loop

in GAP 4.8.6 this works:

gap> Read("sage.g");
gap> s5:=SymmetricGroup(5);
Sym( [ 1 .. 5 ] )
gap> \$SAGE.OperationsAdmittingFirstArgument(s5);
[ <Operation "ViewObj">, <Operation "ViewString">, <Operation "NameFunction">, <Operation "SetNameFunction">, 
...

I've asked here, as this is due to format change of internal GAP data...

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

comment:100 Changed 15 months ago by dimpase

Very similar outdated code is in OperationInspector.operations in sage/libs/gap/operations.py.

comment:101 Changed 15 months ago by git

  • Commit changed from 732cbdd54e24ac35c3fd52c75fb7608ad41603ce to 41218248115922fdae5c4ebb01a03a7df7bafc91

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

4121824fix for tab completion for GAP interface

comment:102 Changed 15 months ago by git

  • Commit changed from 41218248115922fdae5c4ebb01a03a7df7bafc91 to 1d60fdd386b98b11474df00074039f3b3ef26bca

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

1d60fddfix tab completion in libgap interface

comment:103 Changed 15 months ago by dimpase

  • Dependencies #25273 deleted
  • Stopgaps changed from tab completion on GAP objects, error handling in libgap to error handling in libgap

broken error handling:

sage: a=libgap(1)
sage: getattr(a,'_rich_repr_',None)
Error, Variable: '_rich_repr_' must have a value
sage: 

(with a default argument, getattr should just return it, with raising anything)

comment:104 Changed 15 months ago by dimpase

  • Stopgaps changed from error handling in libgap to error handling in libgap, documentation display

Please see https://github.com/gap-system/gap/issues/2874 for the current sticking point, which is error handling and processing of error messages. Basically, the question is whether it's possible to retain the behaviour of libGAP without patches to the GAP's kernel.

I'd like someone who knows more about libGAP error handling to have a look at badly butchered by me parts in the branch dealing with it, and suggest what should be done.

Another (easier) thing to fix is GAP and libgap documentation display.

comment:105 Changed 15 months ago by dimpase

  • Status changed from new to needs_info

comment:106 Changed 14 months ago by git

  • Commit changed from 1d60fdd386b98b11474df00074039f3b3ef26bca to e352f7e4ad34dfc1422afc04506332ef7674755a

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

e352f7emodest progress - can do libgap(1)

comment:107 Changed 14 months ago by dimpase

  • Status changed from needs_info to needs_work
  • Work issues changed from Wait for gap 4.10 release to work...

the interaction with ipython (which tries to get things like _rich_repr_ from GAP to display GAP objects) is far from done. The current problem:

sage: a=libgap(41)
sage: a
41
sage: b=libgap(42)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: libGAP: PrintTo: cannot open stream for output
Error, Length: <list> must be a list (not a boolean or fail)
...

this looks like a bug in my new (bad) implementation of GAP objects printing in Sage.

comment:108 Changed 14 months ago by dimpase

perhaps we should rather try rebasing libGAP to GAP 4.10, which looks a less daunting task now than a total replacement with "native" GAP's libgap, get it working, and then replace all the libgap_* functions by the native ones.

comment:109 follow-up: Changed 14 months ago by git

  • Commit changed from e352f7e4ad34dfc1422afc04506332ef7674755a to 406cae67f059056df29413b8fa36ac9964eb3c5b

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

406cae6still the interaction with ipython is broken

comment:110 Changed 14 months ago by dimpase

On the present branch, I get

sage: a=libgap(1)
sage: a
*** trouble in PRINT_OR_APPEND_TO_STREAM***
...
*** trouble in PRINT_OR_APPEND_TO_STREAM***
1

after this GAP is pretty much nuked. This happens during ipython's attempts to pretty-print a, so that it tries to find _rich_repr_ etc. Specifically, at least on the system I am running this, the trouble starts not immediately, but where _rich_png_ (or the previous thing in the loop) is checked for by get_real_method in IPython/utils/dir2.py.

If I run the same from ./sage --python (after loading Sage there as usual) at least this much (and more) works.

I don't have a good idea how to debug this, so I'd like to pass the token now.

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

comment:111 Changed 14 months ago by dimpase

  • Status changed from needs_work to needs_review

oops, wrong ticket

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

comment:112 Changed 14 months ago by dimpase

  • Status changed from needs_review to needs_work

comment:113 Changed 14 months ago by embray

As long as we're upgrading GAP we should make sure DESTDIR builds work now; see #25044 which this conflicts with. But if this can get done soon we can skip #25044 and incorporate it into this. I'll give you a patch for it if you want.

I'm working on testing this on Cygwin now.

comment:114 Changed 14 months ago by embray

Okay, I am again confused as to what upstream tarball to use, because the one in the ticket description no longer matches the checksum.

comment:115 Changed 14 months ago by dimpase

I just checked that md5=017738e0ba9e166a19084d68123d3e1f as in GAP package checksum matches the md5 of https://github.com/dimpase/gap/releases/download/dev/gap-4.dev.tar.gz

Perhaps your download just got corrupted?

comment:116 follow-up: Changed 13 months ago by thansen

GAP 4.9.3 was uploaded to Debian unstable yesterday, the sagemath Debian package is broken until we can use GAP 4.9. Is the patch here already enough to use libgap from GAP 4.9 directly? It does not apply cleanly to sage 8.4. Or would you say updating the separate libgap to 4.9 is a better idea?

comment:117 in reply to: ↑ 116 Changed 13 months ago by dimpase

Replying to thansen:

GAP 4.9.3 was uploaded to Debian unstable yesterday, the sagemath Debian package is broken until we can use GAP 4.9. Is the patch here already enough to use libgap from GAP 4.9 directly? It does not apply cleanly to sage 8.4.

the patch here is very far from a working one.

Or would you say updating the separate libgap to 4.9 is a better idea?

If you could manage this, it would be great and very useful, but it's a very nontrivial amount of work, due to substantial changes in GAP internals that happened in GAP 4.9.

comment:118 follow-up: Changed 13 months ago by thansen

I see. If there was progress on this from you guys it would be very much appreciated. The freeze period for the next Debian release starts in January. If we can't fix the problem in time, sage will not be in the next Debian release, which will be the stable release for two years.

comment:119 in reply to: ↑ 118 Changed 13 months ago by jdemeyer

Replying to thansen:

The freeze period for the next Debian release starts in January.

For those of us who are not Debian experts, where is this documented? And what kind of changes are still allowed during the "freeze" period? For example, suppose for the sake of argument that this SageMath ticket is only merged in February, could it still be added as a patch to the Debian SageMath package?

comment:120 Changed 13 months ago by thansen

It is documented here: https://release.debian.org/buster/freeze_policy.html

The broken sagemath will eventually be automatically removed from Debian testing, which will become the next Debian release. After February 12 it cannot migrate back to testing even if it is fixed. The earlier transition freeze on January 12 means that no more library updates with SONAME change can be done (which might be relevant for a sage update too).

comment:121 follow-up: Changed 13 months ago by thansen

About the example you mentioned: This ticket does not need to be merged, we just need a usable patch. It seems that the gap maintainer is open to providing libgap.la, see https://bugs.debian.org/912862 . It is not yet clear if gap 4.10 is released in time to be included before the freeze.

comment:122 Changed 13 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

This ticket mentions GAP 4.10, but should we not be focusing instead on GAP 4.9? Or would getting Sage working against GAP 4.10 imply support for 4.9 as well? I would want to check against both, and starting with the existing release (4.9) seems simpler...

Is there something at the system integration level I could be able to help with? ISTM this needs to be a high priority.

comment:123 in reply to: ↑ 121 Changed 13 months ago by embray

Replying to thansen:

About the example you mentioned: This ticket does not need to be merged, we just need a usable patch. It seems that the gap maintainer is open to providing libgap.la, see https://bugs.debian.org/912862 . It is not yet clear if gap 4.10 is released in time to be included before the freeze.

If that's the case, for the sake of integration, it might make more sense to pin GAP in Debia to 4.9 and focus on getting a Sage out that works against GAP 4.9 instead of focusing on 4.10. Given the amount of time left to do this, waiting on an uncertain GAP 4.10 package(s) landing in Debian seems a bit risky...

comment:124 Changed 13 months ago by dimpase

  • Description modified (diff)
  • Priority changed from major to critical

comment:125 Changed 13 months ago by dimpase

I think getting Sage's libGAP work with GAP 4.9 and/or 4.10 is much harder than in used to be in 4.8.x, due to major changes to GAP internals (although I have not tried it yet).

comment:126 follow-up: Changed 13 months ago by nthiery

Posting on behalf of Alexander Konovalov who does not yet have a trac account:

http://www.gap-system.org/pub/gap/gap-4.10/

has now GAP 4.10.0. The website will be updated in due course, but that means that GAP 4.10 is out.

That's excellent news. Now it's up to Sage to catch up.

comment:127 Changed 13 months ago by embray

Great! From what I saw yesterday it seemed like there were some doubts about how soon 4.10 would be released. But this being the case I rescind my previous comment.

comment:128 in reply to: ↑ 109 ; follow-up: Changed 13 months ago by embray

Replying to git:

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

406cae6still the interaction with ipython is broken

If I'm to understand correctly, you just added this patch for debugging purposes, right? It's not intended to go into the final product is it? I'm going to look into this problem.

comment:129 in reply to: ↑ 128 Changed 13 months ago by dimpase

Replying to embray:

Replying to git:

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

406cae6still the interaction with ipython is broken

If I'm to understand correctly, you just added this patch for debugging purposes, right? It's not intended to go into the final product is it? I'm going to look into this problem.

That's right, it's for debugging purposes (although it merely shows that something is going wrong, not more :-)). You might like to coordinate this with Jeroen, as he's going to work on this branch too...

comment:130 follow-up: Changed 13 months ago by embray

I'm updating it right now to the 4.10 release. Jeroen is probably a better person to work on the LibGAP integration just because he has more experience with it. But if there any specific segfaults or anything I could look at I will. I'll take a look, for example, at the tab-completion issue...

comment:131 Changed 13 months ago by embray

I'm also rebasing this branch on current develop, which needs a bit of work...

comment:132 in reply to: ↑ 126 Changed 13 months ago by dimpase

Replying to nthiery:

Posting on behalf of Alexander Konovalov who does not yet have a trac account:

http://www.gap-system.org/pub/gap/gap-4.10/

Alex should be aware that he can post on trac using his github account!

comment:133 Changed 13 months ago by embray

Another thing that it seems still hasn't been done is to update the spkg-install for GAP to use DESTDIR-based installation. I'll just work on that simultaneously with this. If we have to upgrade GAP anyways that should be part of the process (and may expose some flaws in GAP's new installation process...)

Last edited 13 months ago by embray (previous) (diff)

comment:134 Changed 13 months ago by embray

Needed to modify build/make/deps to directly make GAP a build dependency of sagelib as well, which is now needed (and remove the obsolete reference to the libgap spkg).

comment:135 Changed 13 months ago by jdemeyer

  • Authors changed from Nicolas M. Thiéry, ... to Nicolas M. Thiéry, Dima Pasechnik, Jeroen Demeyer
  • Description modified (diff)

comment:136 in reply to: ↑ 130 ; follow-up: Changed 13 months ago by jdemeyer

Replying to embray:

I'm updating it right now to the 4.10 release.

Are you actually doing that? I haven't seen any update on the branch here.

comment:137 Changed 13 months ago by jdemeyer

  • Authors changed from Nicolas M. Thiéry, Dima Pasechnik, Jeroen Demeyer to Nicolas M. Thiéry, Dima Pasechnik, Erik Bray, Jeroen Demeyer

comment:138 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:139 Changed 13 months ago by jdemeyer

Thanks to this Cython PR (by me!), the libGAP T_INT thing might not be an issue anymore: https://github.com/cython/cython/pull/2006

Cython will still include <structmember.h> but only after all user code. It's still very fragile to have two different meanings for T_INT in the same source file (one enum value and one #define), but it should work for now. Of course, nobody can promise that it will remain working in the future.

comment:140 in reply to: ↑ 136 ; follow-up: Changed 13 months ago by embray

Replying to jdemeyer:

Replying to embray:

I'm updating it right now to the 4.10 release.

Are you actually doing that? I haven't seen any update on the branch here.

Just locally. It doesn't even build for me yet so no point in pushing.

comment:141 Changed 13 months ago by embray

I should clarify: GAP itself compiles just fine. But "installation", such that there is any, is still a partially-broken headache, and I'm experimenting how best to incorporate a GAP installation into Sage.

For now I'll probably keep with something similar to what we have currently, of having a GAP_DIR under $SAGE_LOCAL/gap. But I'm trying to decide, in that case, what the best thing to do with libgap might be.

Though I'm also trying to see if we can do something entirely different with the installation. There's something close to a standard "make install" but it isn't quite right, and I don't know how it works with GAP packages.

Last edited 13 months ago by embray (previous) (diff)

comment:142 follow-up: Changed 13 months ago by embray

It seems that currently make install actually just installs the libtool wrapper script for gap, and not the plain gap executable. I'm not sure if this is intentional or not--normally this would not make a whole lot of sense I think. Though one advantage to it is that the libtool wrapper script adds the "GAP_ROOT" that it was built in to PATH, and thus it seems capable of finding its way to the original GAP_ROOT it was installed from. This doesn't really work though, from the perspective of system packaging. The paths hard-coded in the libtool wrapper script are to the build directory itself, so when we move all the GAP_ROOT files to a new location such as $SAGE_LOCAL/gap/gap-4.10 it doesn't work properly with that GAP_ROOT.

Better to do what we did traditionally of having bin/gap as a shell script that wraps the real gap executable and passes an appropriate -l flag. Dima's branch already has something to that effect, though rather than patching bin/gap.sh I think we should just install our own custom gap script, such as the one that's currently in this branch but unused.

I'm also experimenting with adding the actual executable under something like bin/gap-bin. I don't think we need all the files from the GAP source tree, much less build artifacts. I think we can have a minimal GAP_ROOT of sorts (e.g. under share/gap) that includes grp/, lib/, pkg/ at a minimum... Maybe doc/? And possibly also src/ (so that PageSource can work for kernel functions) but I'm not sure if that's needed or not...

Last edited 13 months ago by embray (previous) (diff)

comment:143 follow-up: Changed 13 months ago by embray

So far this "relatively minimal" GAP install seems to be working pretty well, and is much cleaner. I'm sure there are some glitches here and there, but I don't have the expertise to really know how to break it. A lot of the test suites under tst/ seem to work, or mostly work.

comment:144 in reply to: ↑ 143 Changed 13 months ago by dimpase

Replying to embray:

So far this "relatively minimal" GAP install seems to be working pretty well, and is much cleaner. I'm sure there are some glitches here and there, but I don't have the expertise to really know how to break it. A lot of the test suites under tst/ seem to work, or mostly work.

Breaking libGAP should be easy, but GAP itself should be OK, I think.

comment:145 Changed 13 months ago by embray

FWIW I'm also developing and testing this on Cygwin, just for some extra wind in my face.

comment:146 Changed 13 months ago by embray

Note: Also need to manually install config.h into the headers. There's even a note in GAP's Makefile: # TODO: take care of config.h :(

comment:147 Changed 13 months ago by embray

Progress on my branch so far here.

Currently it builds, but is broken (trying to do anything with libgap just hangs).

Last edited 13 months ago by embray (previous) (diff)

comment:148 in reply to: ↑ 140 Changed 13 months ago by jdemeyer

Replying to embray:

Just locally. It doesn't even build for me yet so no point in pushing.

Depends. If it's an improvement over Dima's branch...

The question is basically: if I want to work on the libGAP interface, on which branch should I work?

comment:149 Changed 13 months ago by dimpase

I'm building Erik's branch now, once done, I'll be able to say how well it does its job (Erik works on Windows, so this makes things even more interesting for him).

comment:150 Changed 13 months ago by dimpase

Erik's branch does refer to *.gz tarball, but GAP has .*bz2 tarball. Thus

--- a/build/pkgs/gap/checksums.ini
+++ b/build/pkgs/gap/checksums.ini
@@ -1,4 +1,4 @@
-tarball=gap-VERSION.tar.gz
-sha1=ab95077df0775bc39dbf13becdc05818935a53d2
-md5=4d3cd775a7dcf5582b8b4dbc2b484bd4
-cksum=1444102593
+tarball=gap-VERSION.tar.bz2
+sha1=60d0b6b0127758da4270569c6ff42ad2364cac7e
+md5=7a94c54d2b1190d6471084b8fcbda6a9
+cksum=40709676

comment:151 Changed 13 months ago by dimpase

  • Work issues changed from work... to fix libgap workspace loading, etc, and much more work...

Apart from this little issue, Erik's branch (u/embray/spkgs/gap-410) is OK---in the sense I can basically do what I was able to do on my branch. E.g. (after not forgetting to remove the stale (lib)GAP workspaces - all bets are off if they are present, e.g. it might be why Erik's job hang, probably something to rectify in the installation prcedure) I can do

sage: a=libgap(1)
sage: b=libgap(1)
sage: c=a+b
sage: c.sage()
2

and then get a segfault as follows:

sage: a
---------------------------------------------------------------------------
SignalError                               Traceback (most recent call last)
<ipython-input-5-3f786850e387> in <module>()
----> 1 a

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/IPython/core/displayhook.pyc in __call__(self, result)
    244             self.start_displayhook()
    245             self.write_output_prompt()
--> 246             format_dict, md_dict = self.compute_format_data(result)
    247             self.update_user_ns(result)
    248             self.fill_exec_result(result)

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/IPython/core/displayhook.pyc in compute_format_data(self, result)
    148 
    149         """
--> 150         return self.shell.display_formatter.format(result)
    151 
    152     # This can be set to True by the write_output_prompt method in a subclass

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/sage/repl/display/formatter.pyc in format(self, obj, include, exclude)
    211             exclude = self.default_mime()
    212         ipy_format, ipy_metadata = super(SageDisplayFormatter, self).format(
--> 213             obj, include=include, exclude=exclude)
    214         if not ipy_format:
    215             return sage_format, sage_metadata

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/IPython/core/formatters.pyc in format(self, obj, include, exclude)
    171             md = None
    172             try:
--> 173                 data = formatter(obj)
    174             except:
    175                 # FIXME: log the exception

<decorator-gen-9> in __call__(self, obj)

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/IPython/core/formatters.pyc in catch_format_error(method, self, *args, **kwargs)
    215     """show traceback on failed format call"""
    216     try:
--> 217         r = method(self, *args, **kwargs)
    218     except NotImplementedError:
    219         # don't warn on NotImplementedErrors

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/IPython/core/formatters.pyc in __call__(self, obj)
    334                 return printer(obj)
    335             # Finally look for special method names
--> 336             method = get_real_method(obj, self.print_method)
    337             if method is not None:
    338                 return method()

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/IPython/utils/dir2.pyc in get_real_method(obj, name)
     72 
     73     try:
---> 74         m = getattr(obj, name, None)
     75     except Exception:
     76         return None

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/sage/libs/gap/element.pyx in sage.libs.gap.element.GapElement.__getattr__ (build/cythonized/sage/libs/gap/element.c:6741)()
    581         try:
    582             proxy = make_GapElement_MethodProxy\
--> 583                 (self.parent(), gap_eval(name), self)
    584         except ValueError:
    585             raise AttributeError('name "'+str(name)+'" is not defined in GAP.')

/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:5412)()
    310 #    print("gap_string: "+gap_string+"\n")
    311     try:
--> 312             sig_on()
    313             result = GAP_EvalString(cmd)
    314             nresults = LEN_LIST(result)

SignalError: Segmentation fault

This is the sort of crashes I traced to ipython trying to do a rich display of a, basically, making GAP repeatedly report errors.

It seems that now I can reproduce this hanging, it seems that libgap workspace does not load correctly, so to start Sage and libgap I have to manually remove it in ~/.sage/gap/.

comment:152 in reply to: ↑ 142 ; follow-up: Changed 13 months ago by fbissey

Replying to embray:

It seems that currently make install actually just installs the libtool wrapper script for gap, and not the plain gap executable. I'm not sure if this is intentional or not--normally this would not make a whole lot of sense I think. Though one advantage to it is that the libtool wrapper script adds the "GAP_ROOT" that it was built in to PATH, and thus it seems capable of finding its way to the original GAP_ROOT it was installed from. This doesn't really work though, from the perspective of system packaging. The paths hard-coded in the libtool wrapper script are to the build directory itself, so when we move all the GAP_ROOT files to a new location such as $SAGE_LOCAL/gap/gap-4.10 it doesn't work properly with that GAP_ROOT.

Better to do what we did traditionally of having bin/gap as a shell script that wraps the real gap executable and passes an appropriate -l flag. Dima's branch already has something to that effect, though rather than patching bin/gap.sh I think we should just install our own custom gap script, such as the one that's currently in this branch but unused.

I'm also experimenting with adding the actual executable under something like bin/gap-bin. I don't think we need all the files from the GAP source tree, much less build artifacts. I think we can have a minimal GAP_ROOT of sorts (e.g. under share/gap) that includes grp/, lib/, pkg/ at a minimum... Maybe doc/? And possibly also src/ (so that PageSource can work for kernel functions) but I'm not sure if that's needed or not...

This is not my experience here with just plain gap-4.10.0 package

fbissey@moonloop ~/sandbox/local-gap/bin $ ldd -r gap
        linux-vdso.so.1 (0x00007ffc039c2000)
        libgmp.so.10 => /usr/lib64/libgmp.so.10 (0x00007fa163a4a000)
        libz.so.1 => /lib64/libz.so.1 (0x00007fa163833000)
        libreadline.so.7 => /lib64/libreadline.so.7 (0x00007fa1635e6000)
        libm.so.6 => /lib64/libm.so.6 (0x00007fa16325c000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007fa163058000)
        libutil.so.1 => /lib64/libutil.so.1 (0x00007fa162e55000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fa162a8d000)
        libncurses.so.6 => /lib64/libncurses.so.6 (0x00007fa162831000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fa1645f2000)
fbissey@moonloop ~/sandbox/local-gap/bin $ file gap
gap: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, with debug_info, not stripped

I just get plain executable binaries not libtool scripts after install. Both with the tarball in the description and the one from github (which is lighter because it doesn't include gap packages).

comment:153 Changed 13 months ago by embray

I was getting outright hanging as soon as libgap is initialized (the sage.libs.gap.util.initialize function. It would hang so badly that I could only terminate the process with a SIGKILL, and it would leave my terminal in some invalid state (I have to type reset to get it back--the characters aren't even echo'd). I should note, this was on Cygwin. I will try now on Linux and see if I get any further.

Regardless, libgap probably shouldn't be doing any monkeying around with the terminal, and that might be part of the problem on Cygwin.

Dima, from where did you download the 4.10.0 tarball? The one I got from gap-system.org was a tar.gz as in my branch.

comment:154 in reply to: ↑ 152 Changed 13 months ago by embray

Replying to fbissey:

This is not my experience here with just plain gap-4.10.0 package

fbissey@moonloop ~/sandbox/local-gap/bin $ ldd -r gap
        linux-vdso.so.1 (0x00007ffc039c2000)
        libgmp.so.10 => /usr/lib64/libgmp.so.10 (0x00007fa163a4a000)
        libz.so.1 => /lib64/libz.so.1 (0x00007fa163833000)
        libreadline.so.7 => /lib64/libreadline.so.7 (0x00007fa1635e6000)
        libm.so.6 => /lib64/libm.so.6 (0x00007fa16325c000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007fa163058000)
        libutil.so.1 => /lib64/libutil.so.1 (0x00007fa162e55000)
        libc.so.6 => /lib64/libc.so.6 (0x00007fa162a8d000)
        libncurses.so.6 => /lib64/libncurses.so.6 (0x00007fa162831000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fa1645f2000)
fbissey@moonloop ~/sandbox/local-gap/bin $ file gap
gap: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, with debug_info, not stripped

I just get plain executable binaries not libtool scripts after install. Both with the tarball in the description and the one from github (which is lighter because it doesn't include gap packages).

I can't tell exactly what file you're looking at, but if the bin/ directory you reference above is the one that gets created in the gap source three, that's the normal executable. I'm taking about the gap that gets installed to $PREFIX when you run make install.

comment:155 Changed 13 months ago by embray

It's possible this is only a bug on Cygwin too. I haven't played with it on Linux yet, but I am beginning to now.

comment:156 follow-ups: Changed 13 months ago by embray

On Linux I was able to reproduce Dima's results. The first libgap() call is a little slow, which is to be expected--the GAP interface is initialized lazily. After that it works up to outputting a, whereas a.sage() and the like works. I also get the tab-completion segfault.

Jeroen: do you have anything to report from your investigation so far? I'd like to look into these segfaults but I don't want to duplicate your effort if you've already made significant progress.

comment:157 Changed 13 months ago by embray

Ooh, interesting. After my initial crash I started Sage up again, tried libgap(1), and it hanged just like on Cygwin. The process just sits without any CPU usage and does not respond to ctrl-C. It does respond to a SIGTERM. Could this have something to do with a corrupt GAP workspace?

comment:158 Changed 13 months ago by embray

I suspected something like this: GAP is installing its own readline callbacks, overriding the Python REPL's. I think it should not be doing that at all if --nointeract.

comment:159 in reply to: ↑ 156 Changed 13 months ago by dimpase

Replying to embray:

On Linux I was able to reproduce Dima's results. The first libgap() call is a little slow, which is to be expected--the GAP interface is initialized lazily. After that it works up to outputting a, whereas a.sage() and the like works. I also get the tab-completion segfault.

Jeroen: do you have anything to report from your investigation so far? I'd like to look into these segfaults but I don't want to duplicate your effort if you've already made significant progress.

I suspect that I forgot that we must not hold pointers to GAP objects - as GAP’s GC is moving things, this would be asking for trouble. So this could be a rich source of bugs I introduced in my branch :(

comment:160 Changed 13 months ago by dimpase

https://github.com/gap-system/gap/pull/2988 is introducing more things to libgap API, have a look (and say)

comment:161 Changed 13 months ago by dimpase

GAP 4.10 tarballs are here: http://www.gap-system.org/Releases/

comment:162 Changed 13 months ago by embray

Apparently passing the -E argument disables readline support so we should do that. Trying it now.

comment:163 Changed 13 months ago by embray

The issue with "corrupt" (?) gap workspaces seems to affect old versions too. After a crash, I switched over to a Sage build with GAP 4.8 just to compare something, and calling libgap hanged again. I have to rm -rf the old workspace to get it to work again. Nevermind; was still running the development Sage. Old Sage does not appear to be impacted.

Last edited 13 months ago by embray (previous) (diff)

comment:164 Changed 13 months ago by embray

This comment from Markus seems relevant: https://github.com/gap-system/gap/pull/2702#issuecomment-414960197 I am observing something that sounds like what he describes, and it would explain the terminal non-responsiveness. I'm not exactly sure where or why this is happening though.

comment:165 Changed 13 months ago by embray

-E fixed problems with readline, but we're still having problems in error handling. Somehow the error handling code--particularly manipulation of the ERROR_OUTPUT stream, results in a stack overflow \o/

comment:166 follow-up: Changed 13 months ago by dimpase

libgap is started in sage.libs.gap.util.initialize(), and you see that

argv[11] = "--nointeract"

i.e. --nointeract is passed. So you add -E to this list, right?

The GAP patch you disabled in your branch makes the stack overflow you see a bit more transparent, I think...

comment:167 follow-up: Changed 13 months ago by dimpase

in my branch in sage.libs.gap.util.initialize() I have cdef int argc = 11, and this seems to be off by 1. Is it the root of all the evil?

comment:168 in reply to: ↑ 167 Changed 13 months ago by embray

Replying to dimpase:

in my branch in sage.libs.gap.util.initialize() I have cdef int argc = 11, and this seems to be off by 1. Is it the root of all the evil?

No, that seems right to me...? No, you're right. It's off by one.

Last edited 13 months ago by embray (previous) (diff)

comment:169 in reply to: ↑ 166 Changed 13 months ago by embray

Replying to dimpase:

libgap is started in sage.libs.gap.util.initialize(), and you see that

argv[11] = "--nointeract"

i.e. --nointeract is passed. So you add -E to this list, right?

Basically. I replaced -T with -E, since -T is implied by --no-interact (which should probably also imply -E, but it doesn't).

However, as you pointed out, there's an off-by-one and now I'm not so sure if --no-interact was being processed at all.

Last edited 13 months ago by embray (previous) (diff)

comment:170 Changed 13 months ago by embray

There's what I would consider a bug in GAP, where the function PRINT_OR_APPEND_TO_STREAM goes:

    /* try to open the file for output                                     */
    i = OpenOutputStream(stream);
    if ( ! i ) {
        ErrorQuit( "%s: cannot open stream for output", (Int)funcname, 0L );
        return 0;
    }

but if this is called to print to the ERROR_OUTPUT stream during ErrorQuit then we get an infinite recursion. This is the root cause of the segfault's we're seeing. What I'm not sure about is why OpenOutputStream would fail, since we close ERROR_OUTPUT after each error, and then re-open it as a new stream. That part bothers me. But I think GAP should check here if stream == ERROR_OUTPUT and if so reset ERROR_OUTPUT to its default value as a last resort, or something to that effect.

comment:171 follow-up: Changed 13 months ago by dimpase

Erik, do you have your updated branch somewhere online? My last attempt to build your branch ends with a glorious collision of GAP's T_INT and Python's T_INT...

Yes, I agree that PRINT_OR_APPEND_TO_STREAM is a problem, and the patch in my branch I mentioned in comment 166 provides a crude workaround. I already mentioned this to GAP devs more than once, see e.g. https://github.com/gap-system/gap/issues/2487

comment:172 follow-up: Changed 13 months ago by embray

No, I don't have an updated branch. I'm not seeing that problem before.

I think I have some idea what the issue is. The problem is that because we call sig_error() in our error handler, we jump out of GAP's eval loop before it can perform certain cleanup, including closing the output stream that it's using to capture output in GAP_EvalString.

So we're leaving things in a slightly fragile state; and more importantly we're not cleaning up after ourselves. I believe that's the case here. I wonder if we should forgo sig_error() entirely and just let GAP_EvalString return naturally. The error handler can just record somewhere that an error occurred, and we can raise an exception from within gap_eval`.

comment:173 Changed 13 months ago by embray

Yep. This patch fixes it:

  • src/sage/libs/gap/gap_includes.pxd

    diff --git a/src/sage/libs/gap/gap_includes.pxd b/src/sage/libs/gap/gap_includes.pxd
    index 06a82ee..f348e2c 100644
    a b cdef extern from "<gap/libgap-api.h>": 
    129129    ctypedef void (*CallbackFunc)()
    130130    void GAP_Initialize(int argc, char ** argv, char ** env,
    131131        CallbackFunc, CallbackFunc)
    132     Obj GAP_EvalString(const char *)
     132    Obj GAP_EvalString(const char *) except *
    133133    Obj GAP_ValueGlobalVariable(const char *)
    134134#    void libgap_start_interaction(char* inputline)
    135135#    char* libgap_get_output()
  • src/sage/libs/gap/util.pyx

    diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx
    index aa94e89..fb1e5e1 100644
    a b cdef initialize(): 
    221221    argv[8] = "64m"
    222222
    223223    argv[9] = "-q"    # no prompt!
    224     argv[10] = "-T"    # no debug loop
    225     argv[11] = "--nointeract"
     224    argv[10] = "-E"    # don't use readline as this will interfere with Python
     225    argv[11] = "--nointeract"  # Implies -T
    226226    argv[12] = NULL
    227227    cdef int argc = 11   # argv[argc] must be NULL
    228228
    cdef initialize(): 
    237237
    238238    # Initialize GAP and capture any error messages
    239239    # The initialization just prints error and does not use the error handler
    240     # libgap_initialize(argc, argv)
    241240    try:
    242241        GAP_Initialize(argc, argv, environ, &gasman_callback, &error_handler)
    243242    except RuntimeError as msg:
    244243        raise RuntimeError('libGAP initialization failed\n' + msg)
    245244
    246     # gap_error_msg = char_to_str(libgap_get_output())
    247     GAP_EvalString('libgap_errout := \"\"; ERROR_OUTPUT := OutputTextString(libgap_errout, false);')
    248 
    249     # The error handler is called if a GAP evaluation fails, e.g. 1/0
    250     # libgap_set_error_handler(&error_handler)
     245    # Set the ERROR_OUTPUT global in GAP to an output stream in which to
     246    # receive error output
     247    GAP_EvalString('libgap_errout := ""; '
     248                   'ERROR_OUTPUT := OutputTextString(libgap_errout, false);')
    251249
    252250    # Prepare global GAP variable to hold temporary GAP objects
    253251    global reference_holder
    cdef Obj gap_eval(str gap_string) except? NULL: 
    307305    # so that Cython doesn't dereference it before libGAP is done with
    308306    # its contents.
    309307    cmd = str_to_bytes(gap_string + ';\n')
    310 #    print("gap_string: "+gap_string+"\n")
    311308    try:
    312             sig_on()
    313             result = GAP_EvalString(cmd)
    314             nresults = LEN_LIST(result)
    315 #            print("nresults="+str(nresults)+"\n")
    316             if nresults > 1: # to mimick the old libGAP
    317                 raise ValueError('can only evaluate a single statement')
    318             result = ELM_LIST(result, 1) # 1-indexed!
    319 #            print("result's length: "+str(LEN_LIST(result))+"\n")
    320             if ELM_LIST(result, 1) != GAP_True:
    321                 # libgap_call_error_handler()
    322                         print("An error occurred, but libGAP has no handler set")
    323                         return GAP_False # needs work
    324             sig_off()
     309        sig_on()
     310        result = GAP_EvalString(cmd)
     311
     312        # If an error occurred in GAP_EvalString we won't even get
     313        # here if the error handler was set; but in case it wasn't
     314        # let's still check the result...
     315        nresults = LEN_LIST(result)
     316        if nresults > 1:  # to mimick the old libGAP
     317            # TODO: Get rid of this restriction eventually?
     318            raise ValueError('can only evaluate a single statement')
     319
     320        result = ELM_LIST(result, 1) # 1-indexed!
     321        if ELM_LIST(result, 1) != GAP_True:
     322            raise RuntimeError("an error occurred, but libGAP has no "
     323                               "error handler set")
    325324    except RuntimeError as msg:
    326             raise ValueError('libGAP: '+str(msg).strip())
     325        raise ValueError(f'libGAP: {msg}')
     326    finally:
     327        sig_off()
    327328
    328329    return ELM_LIST(result, 2)
    329330
    cdef void hold_reference(Obj obj): 
    357358
    358359cdef void error_handler():
    359360    """
    360     The libgap error handler
     361    The libgap error handler.
    361362
    362     We call ``sig_error()`` which causes us to jump back to the Sage
    363     signal handler. Since we wrap libGAP C calls in ``sig_on`` /
    364     ``sig_off`` blocks, this then jumps back to the ``sig_on`` where
    365     the ``RuntimeError`` we raise here will be seen.
     363    If an error occurred we set a RuntimeError; when the original
     364    GAP_EvalString returns this exception will be raised.
     365
     366    TODO: We should probably prevent re-entering this function if we
     367    are already handling an error; if there is an error in our stream
     368    handling code below it could result in a stack overflow.
    366369    """
    367370    cdef Obj r
     371    cdef char *msg
     372
     373    # TODO: Do we need/want this ClearError??
     374    ClearError()
     375
     376    # Close the error stream: This flushes any remaining output and closes
     377    # the stream for further writing; reset ERROR_OUTPUT to something sane
     378    # just in case (trying to print to a closed stream segfaults GAP)
     379    GAP_EvalString('CloseStream(ERROR_OUTPUT); '
     380                   'ERROR_OUTPUT := "*errout*"; '
     381                   'MakeImmutable(libgap_errout);');
    368382    r = GAP_ValueGlobalVariable("libgap_errout")
    369     GAP_EvalString("CloseStream(ERROR_OUTPUT);");
    370     GAP_EvalString('libgap_errout := \"\"; ERROR_OUTPUT := OutputTextString(libgap_errout, false);')
    371383
    372     msg_py = char_to_str(CSTR_STRING(r))
    373     msg_py = msg_py.replace('For debugging hints type ?Recovery from NoMethodFound\n', '')
     384    # Grab a pointer to the C string underlying the GAP string libgap_errout
     385    # then copy it to a Python str (char_to_str contains an implicit strcpy)
     386    msg = CSTR_STRING(r)
     387    if msg != NULL:
     388        msg_py = char_to_str(msg)
     389        msg_py = msg_py.replace('For debugging hints type ?Recovery from '
     390                                'NoMethodFound\n', '')
     391    else:
     392        # Shouldn't happen but just in case...
     393        msg_py = "An unknown error occurred in libGAP"
     394
     395    # Reset ERROR_OUTPUT with a new text string stream
     396    GAP_EvalString('libgap_errout := ""; '
     397                   'ERROR_OUTPUT := OutputTextString(libgap_errout, false);')
     398
    374399    PyErr_SetObject(RuntimeError, msg_py)
    375     ClearError()
    376     sig_error()
    377400
    378401
    379402############################################################################

Now printing GAP elements works, and it also fixes tab-completion. So a lot of basic stuff works with this. I make no guarantees about memory leaks, reference counting, etc.

Last edited 13 months ago by embray (previous) (diff)

comment:174 Changed 13 months ago by dimpase

you still have wrong argc count on line 227.

comment:175 Changed 13 months ago by dimpase

But otherwise it's a lot of progress, great!!!

comment:176 in reply to: ↑ 156 Changed 13 months ago by jdemeyer

Replying to embray:

Jeroen: do you have anything to report from your investigation so far? I'd like to look into these segfaults but I don't want to duplicate your effort if you've already made significant progress.

No, on the contrary: I'm not doing anything for the moment since I don't want to interfere were your (Erik and Dima) work.

comment:177 in reply to: ↑ 171 ; follow-up: Changed 13 months ago by jdemeyer

Replying to dimpase:

My last attempt to build your branch ends with a glorious collision of GAP's T_INT and Python's T_INT...

Which branch is that? According to my earlier analysis, that shouldn't happen. So I'd like to see that branch.

comment:178 in reply to: ↑ 172 Changed 13 months ago by jdemeyer

Replying to embray:

The problem is that because we call sig_error() in our error handler, we jump out of GAP's eval loop before it can perform certain cleanup, including closing the output stream that it's using to capture output in GAP_EvalString.

If so, the problem is not limited to sig_error() but also to interrupts. So it seems like your proposed solution of not using sig_error() will break interrupts, which I do not consider acceptable.

Would there be a way of patching GAP to make GAP_EvalString stateless?

comment:179 follow-ups: Changed 13 months ago by embray

I don't know what you mean that it would "break interrupts". Either way handling an interrupt with cysignals and not returning to GAP is going to break things.

There's definitely still a problem somewhere:

sage: b = libgap.eval('Sleep(5);')
Caught error at top-most level, probably quit from library loading

It sleeps, then displays that message immediately and quits the process. Strangely, the only source I can find for that message is at the end of InitializeGap which should have already exited. I think there is still some strange things going on in GAP's error handling...

On the other hand if I do:

sage: f = libgap.eval('Sleep;')
sage: f
<Gap function "Sleep">
sage: f(5)

it works. But when I hit Ctrl-C:

sage: f(10)
^CCaught error at top-most level, probably quit from library loading

and the process quits again.

Last edited 13 months ago by embray (previous) (diff)

comment:180 in reply to: ↑ 179 Changed 13 months ago by jdemeyer

Replying to embray:

I don't know what you mean that it would "break interrupts".

(A) If the keep the sig_on() statements, then CTRL-C would break GAP in the same way as sig_error() breaks GAP.

(B) If you remove the sig_on() statements, then GAP can no longer be interrupted with CTRL-C.

I consider both alternatives not acceptable.

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

comment:181 Changed 13 months ago by embray

I'm not even sure yet how GAP handles signals. Do you know anything about it? I haven't looked into it yet.

comment:182 Changed 13 months ago by embray

There is a RegisterSyLongjmpObserver that might be useful. I'm not sure if it's used at all in signal handling though.

Last edited 13 months ago by embray (previous) (diff)

comment:183 Changed 13 months ago by jdemeyer

Looking at the GAP sources right now... GAP seems to handle interrupts similar to CPython, by setting some global state. The function below is called by the SIGINT handler:

/****************************************************************************
**
*F  InterruptExecStat() . . . . . . . . interrupt the execution of statements
**
**  'InterruptExecStat'  interrupts the execution of   statements at the next
**  possible moment.  It is called from 'SyAnsIntr' if an interrupt signal is
**  received.  It is never called on systems that do not support signals.  On
**  those systems the executors test 'SyIsIntr' at regular intervals.
**
**  'InterruptExecStat' changes all entries   in the executor  dispatch table
**  'ExecStatFuncs'  to point to  'ExecIntrStat',  which changes  the entries
**  back, calls 'Error', and redispatches after a return from the break-loop.
*/
void InterruptExecStat ( void )
{
    /* remember the original entries from the table 'ExecStatFuncs'        */
    STATE(CurrExecStatFuncs) = IntrExecStatFuncs;
}

comment:184 follow-up: Changed 13 months ago by embray

Okay. GAP's own signal handling (which just handles SIGINT) uses InterruptExecStat to signal to the eval loop that an interrupt has occurred, so it will break out and return an error at the first opportunity. We can probably make use of that.

comment:185 Changed 13 months ago by embray

I see we've got our noses on the same trail :) I'm going to step away for a bit anyways.

comment:186 in reply to: ↑ 184 ; follow-up: Changed 13 months ago by jdemeyer

Replying to embray:

We can probably make use of that.

This will need new hooks in cysignals though. Also, all sig_on() and sig_off() statements should be removed from the GAP interface.

Given that this is not the first priority, I'll let you guys work further. We can deal with interrupts once we have a mostly-finished GAP interface.

comment:187 in reply to: ↑ 186 ; follow-up: Changed 13 months ago by embray

Replying to jdemeyer:

Replying to embray:

We can probably make use of that.

This will need new hooks in cysignals though. Also, all sig_on() and sig_off() statements should be removed from the GAP interface.

I was wondering about that. I'm not sure cysignals is needed at all really, though if it can provide a wrapper interface to make things easier that might be nice. I think it would be sufficient, when entering and leaving GAP_EvalString, to just register/unregister a custom signal handler, for SIGINT at the very least.

We also need to be careful around GAP_Initialize to restore any existing SIGINT handler that gets clobbered by GAP's initialization...

comment:188 in reply to: ↑ 187 Changed 13 months ago by jdemeyer

Replying to embray:

to just register/unregister a custom signal handler, for SIGINT at the very least.

Since that involves system calls, that would be less efficient than doing something within cysignals. I bet that this would cause a noticable slowdown for a calculation like 1 + 1.

comment:189 in reply to: ↑ 179 Changed 13 months ago by embray

Replying to embray:

I don't know what you mean that it would "break interrupts". Either way handling an interrupt with cysignals and not returning to GAP is going to break things.

There's definitely still a problem somewhere:

sage: b = libgap.eval('Sleep(5);')
Caught error at top-most level, probably quit from library loading

It sleeps, then displays that message immediately and quits the process. Strangely, the only source I can find for that message is at the end of InitializeGap which should have already exited. I think there is still some strange things going on in GAP's error handling...

The reason for this appears to be a bug in how we're processing the result list from GAP_EvalString--in particular it is the equivalent of a Python IndexError on the list. GAP is treating this as a sort of internal error, meaning it jumps to the last invocation of its TRY_IF_NO_ERROR/CATCH_ERROR macros. But since in our code we're not in a TRY/CATCH, perversely the last TRY is back in InitializeGap, so it invokes a longjmp back there.

It seems we can work around that by using the ELM_PLIST function directly rather than the higher-level generic ELM_LIST. The lower-level function does not invoke GAP error handling. I've added another comment about this here: https://github.com/gap-system/gap/pull/2988/files#r233021437

comment:190 Changed 13 months ago by embray

Another annoyance--albeit one that I'm not sure if Sage's libGAP handles either--is that if GAP Panic(...)s it will just exit(1) the process and I'm not sure if there's any way around that currently...

comment:191 in reply to: ↑ 177 Changed 13 months ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

My last attempt to build your branch ends with a glorious collision of GAP's T_INT and Python's T_INT...

Which branch is that? According to my earlier analysis, that shouldn't happen. So I'd like to see that branch.

Well, it was something cooked up a bit, it could be I just messed things up. I hope Erik can update his branch and share it, so that I can try again...

comment:192 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:193 follow-up: Changed 13 months ago by embray

  • Description modified (diff)

Should I just go ahead and attach my branch to this ticket?

It's up to date with my current work, which mostly works pretty well except for three major issues that I'm aware of:

1) The weirdness with hanging during GAP initialization if there is an existing workspace. This might have something to do with the issue Markus has mentioned w.r.t. spawning a shell (??) for some reason. I'm going to look into that next since I consider it the most severe issue for getting something usable. I'm sure I can figure it out quickly; I just haven't investigated it at all yet.

2) General issues regarding signal handling. The situation there mostly isn't great--I found some cases where even GAP's eval loop can't be interrupted. I agree with Jeroen that ideally that's "not acceptable", but I believe we may need to compromise in the near term (and the situation isn't all that better in current Sage either).

3) The Panic issue. I've confirmed that affects current Sage as well. I believe Markus has made some comment to suggest he's thinking about that (among other related issues) so we may have a solution at some point (I think the obvious one, at least broadly speaking, is just having a hook for what Panic actually does). But I'm not going to worry about that for the purposes of GAP 4.10 integration since it's at least not a regression.

comment:194 follow-up: Changed 13 months ago by dimpase

regarding 1), I guess it is just off by one argc thing I already mentioned, cause in the case of workspace loading, argc is right, so it does notexpect a shell...

by all means, change the branch to your branch...

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

comment:195 in reply to: ↑ 194 Changed 13 months ago by embray

Replying to dimpase:

regarding 1), I guess it is just off by one argc thing I already mentioned, cause in the case of workspace loading, argc is right, so it does notexpect a shell...

by all means, change the branch to your branch...

I don't think so, because I fixed that in my branch and it still happens. I previously thought it might have something to do with the workspace being corrupted since I was causing segfaults, but now all it takes is for me to start sage, do one operation with GAP (enough to initialize the workspace), exit sage, and then try to start GAP again...

comment:196 Changed 13 months ago by embray

  • Branch changed from u/dimpase/WIP/libgap410 to u/embray/spkgs/gap-410
  • Commit changed from 406cae67f059056df29413b8fa36ac9964eb3c5b to 30d1f7fa95fcc9855077c5ed6587748eed1a4cce

My semi-working branch...


Last 10 new commits:

39e6c46modest progress - can do libgap(1)
981e099still the interaction with ipython is broken
17df139Fix more libGAP prefixes
e76f38dremove bogus libgap dependency; add gap as a dependency for sagelib
bcce303updated gap package to the 4.10.0 final release
ad2ea55minor source updates to account for new installation layout
2c21db0Fix some argument handling:
a797f42some improvements to robustness of the ERROR_OUTPUT handling when handling
e58b9edAllow GAP_EvalString to raise a Python exception
30d1f7fcysignals docs sage sig_on() should be outside try

comment:197 Changed 13 months ago by dimpase

  • Description modified (diff)

link to the tarball used by the branch

comment:198 follow-up: Changed 13 months ago by embray

Thanks--sorry about that. No reason not to use the bz2-compressed tarball if it's smaller, either. I just haven't bothered with it yet. If you want to make a public branch off mine to mess around with things feel free to do so.

comment:199 Changed 13 months ago by embray

To my surprise, it's not in LoadWorkspace that it hangs. Regardless, there's something about there being an existing workspace to load that causes it to crash. Continuing my gdb session...

comment:200 in reply to: ↑ 198 Changed 13 months ago by dimpase

Replying to embray:

Thanks--sorry about that. No reason not to use the bz2-compressed tarball if it's smaller, either. I just haven't bothered with it yet. If you want to make a public branch off mine to mess around with things feel free to do so.

I'll have time in about 7 hours - taking a flight from Boston to London tonight. Please update your branch with whatever good things find...

comment:201 Changed 13 months ago by embray

Ah, there's a global function called POST_RESTORE, which I guess gets set by the workspace, which is where it hangs. It also appears in lib/session.g which is probably worth looking at.

comment:202 Changed 13 months ago by embray

Ah, the goldmine is in GAPInfo.PostRestoreFuncs. All kind of system-level and terminal-related stuff in here:

[ function (  )
        local option;
        for option in GAPInfo.CommandLineOptionData do
            if IsBound( option.short ) then
                GAPInfo.CommandLineOptionCanonicalName.(option.short) := option.short;
                if IsBound( option.long ) then
                    GAPInfo.CommandLineOptionCanonicalName.(option.long) := option.short;
                fi;
            else
                GAPInfo.CommandLineOptionCanonicalName.(option.long) := option.long;
            fi;
        od;
        return;
    end, function (  )
        local j, i, CommandLineOptions, opt, InitFiles, line, word, value, padspace;
        GAPInfo.KernelInfo := KERNEL_INFO(  );
        GAPInfo.KernelVersion := GAPInfo.KernelInfo.KERNEL_VERSION;
        GAPInfo.BuildVersion := GAPInfo.KernelInfo.BUILD_VERSION;
        GAPInfo.BuildDateTime := GAPInfo.KernelInfo.BUILD_DATETIME;
        GAPInfo.Architecture := GAPInfo.KernelInfo.GAP_ARCHITECTURE;
        GAPInfo.SystemCommandLine := GAPInfo.KernelInfo.COMMAND_LINE;
        GAPInfo.SystemEnvironment := GAPInfo.KernelInfo.ENVIRONMENT;
        GAPInfo.RootPaths := GAPInfo.KernelInfo.GAP_ROOT_PATHS;
        if IsBound( GAPInfo.SystemEnvironment.HOME ) then
            GAPInfo.UserHome := GAPInfo.SystemEnvironment.HOME;
        else
            GAPInfo.UserHome := fail;
        fi;
        if IsBound( GAPInfo.KernelInfo.DOT_GAP_PATH ) then
            GAPInfo.UserGapRoot := GAPInfo.KernelInfo.DOT_GAP_PATH;
        else
            GAPInfo.UserGapRoot := fail;
        fi;
        GAPInfo.DirectoriesPrograms := false;
        GAPInfo.DirectoryCurrent := false;
        GAPInfo.DirectoriesLibrary := rec(
             );
        GAPInfo.DirectoriesTemporary := [  ];
        GAPInfo.DirectoriesSystemPrograms := [  ];
        if IsBound( GAPInfo.SystemEnvironment.PATH ) then
            j := 1;
            for i in [ 1 .. LENGTH( GAPInfo.SystemEnvironment.PATH ) ] do
                if GAPInfo.SystemEnvironment.PATH[i] = ':' then
                    if i > j then
                        ADD_LIST_DEFAULT( GAPInfo.DirectoriesSystemPrograms,
                         MakeImmutable( GAPInfo.SystemEnvironment.PATH{[ j .. i - 1 ]} ) );
                    fi;
                    j := i + 1;
                fi;
            od;
            if j <= LENGTH( GAPInfo.SystemEnvironment.PATH ) then
                ADD_LIST_DEFAULT( GAPInfo.DirectoriesSystemPrograms,
                 MakeImmutable(
                   GAPInfo.SystemEnvironment.PATH{[ j .. LENGTH( GAPInfo.SystemEnvironment.PATH ) ]} )
                 );
            fi;
        fi;
        CommandLineOptions := rec(
             );
        for opt in GAPInfo.CommandLineOptionData do
            if IsBound( opt.short ) then
                CommandLineOptions.(opt.short) := SHALLOW_COPY_OBJ( opt.default );
            else
                CommandLineOptions.(opt.long) := SHALLOW_COPY_OBJ( opt.default );
            fi;
        od;
        InitFiles := [  ];
        line := GAPInfo.SystemCommandLine;
        i := 2;
        while i <= LENGTH( line ) do
            word := line[i];
            i := i + 1;
            if word = "" then
                PRINT_TO( "*errout*", "Ignoring empty command line argument\n" );
            elif word[1] = '-' and (LENGTH( word ) = 2 or word[2] = '-') then
                opt := word{[ 2 .. LENGTH( word ) ]};
                if opt[1] = '-' then
                    opt := opt{[ 2 .. LENGTH( opt ) ]};
                fi;
                if not IsBound( GAPInfo.CommandLineOptionCanonicalName.(opt) ) then
                    PRINT_TO( "*errout*", "Unrecognised command line option: ", word, "\n" );
                else
                    opt := GAPInfo.CommandLineOptionCanonicalName.(opt);
                    value := CommandLineOptions.(opt);
                    if IS_BOOL( value ) then
                        CommandLineOptions.(opt) := not CommandLineOptions.(opt);
                    elif IS_INT( value ) then
                        CommandLineOptions.(opt) := CommandLineOptions.(opt) + 1;
                    elif i <= LENGTH( line ) then
                        if IS_STRING_REP( value ) then
                            CommandLineOptions.(opt) := line[i];
                            i := i + 1;
                        elif IS_LIST( value ) then
                            ADD_LIST_DEFAULT( CommandLineOptions.(opt), line[i] );
                            i := i + 1;
                        fi;
                    else
                        PRINT_TO( "*errout*", "Command line option ", word, " needs an argument.\n" );
                    fi;
                fi;
            else
                ADD_LIST_DEFAULT( InitFiles, word );
            fi;
        od;
        CommandLineOptions.g := CommandLineOptions.g mod 3;
        CommandLineOptions.E := GAPInfo.KernelInfo.HAVE_LIBREADLINE;
        if CommandLineOptions.nointeract then
            CommandLineOptions.T := true;
            CommandLineOptions.norepl := true;
        fi;
        MakeImmutable( CommandLineOptions );
        MakeImmutable( InitFiles );
        if CommandLineOptions.L = "" or CommandLineOptions.R then
            GAPInfo.CommandLineOptionsPrev := [  ];
            GAPInfo.InitFilesPrev := [  ];
        else
            ADD_LIST_DEFAULT( GAPInfo.CommandLineOptionsPrev, GAPInfo.CommandLineOptions );
            ADD_LIST_DEFAULT( GAPInfo.InitFilesPrev, GAPInfo.InitFiles );
        fi;
        GAPInfo.CommandLineOptions := CommandLineOptions;
        GAPInfo.InitFiles := InitFiles;
        if GAPInfo.CommandLineOptions.D then
            InfoRead1 := Print;
        fi;
        padspace := function ( strlen, len )
              local i;
              for i in [ strlen + 1 .. len ] do
                  PRINT_TO( "*errout*", " " );
              od;
              return;
          end;
        if GAPInfo.CommandLineOptions.h then
            PRINT_TO( "*errout*", "usage: gap [OPTIONS] [FILES]\n",
             "       run the Groups, Algorithms and Programming system, Version ",
             GAPInfo.KernelVersion, "\n\n" );
            for i in [ 1 .. LENGTH( GAPInfo.CommandLineOptionData ) ] do
                if
                 IsBound( GAPInfo.CommandLineOptionData[i] )
                    and IsBound( GAPInfo.CommandLineOptionData[i].help ) then
                    opt := GAPInfo.CommandLineOptionData[i];
                    if IsBound( opt.short ) then
                        PRINT_TO( "*errout*", " -", opt.short );
                        if IsBound( opt.long ) then
                            PRINT_TO( "*errout*", ", --", opt.long );
                            padspace( 4 + LENGTH( opt.long ), 16 );
                        else
                            padspace( 0, 16 );
                        fi;
                        if IsBound( opt.arg ) then
                            PRINT_TO( "*errout*", " ", opt.arg );
                            padspace( LENGTH( opt.arg ) + 1, 8 );
                        else
                            padspace( 0, 8 );
                        fi;
                    else
                        PRINT_TO( "*errout*", "   " );
                        PRINT_TO( "*errout*", "  --", opt.long );
                        padspace( 4 + LENGTH( opt.long ), 16 );
                        if IsBound( opt.arg ) then
                            PRINT_TO( "*errout*", " ", opt.arg );
                            padspace( LENGTH( opt.arg ) + 1, 8 );
                        else
                            padspace( 0, 8 );
                        fi;
                    fi;
                    if IsBound( opt.long ) and LENGTH( opt.long ) > 12 then
                        PRINT_TO( "*errout*", "\n" );
                        padspace( 0, 3 + 16 + 8 + 3 );
                    else
                        PRINT_TO( "*errout*", "   " );
                    fi;
                    PRINT_TO( "*errout*", opt.help[1], "\n" );
                    for j in [ 2 .. LENGTH( opt.help ) ] do
                        padspace( 0, 3 + 16 + 8 + 3 );
                        PRINT_TO( "*errout*", opt.help[j], "\n" );
                    od;
                else
                    if not IsBound( GAPInfo.CommandLineOptionData[i] ) then
                        PRINT_TO( "*errout*", "\n" );
                    fi;
                fi;
            od;
            PRINT_TO( "*errout*", "\n",
             "  Boolean options (b,q,e,r,A,D,E,M,N,T,X,Y) toggle the current value\n",
             "  each time they are called. Default actions are indicated first.\n", "\n" );
            QUIT_GAP(  );
        fi;
        return;
    end, function (  )
        if DEBUG_LOADING then
            InfoRead1 := Print;
        else
            InfoRead1 := Ignore;
        fi;
        MAKE_READ_WRITE_GLOBAL( "DEBUG_LOADING" );
        UNBIND_GLOBAL( "DEBUG_LOADING" );
        ASS_GVAR( "BreakOnError", not GAPInfo.CommandLineOptions.T );
        ASS_GVAR( "AlwaysPrintTracebackOnError", GAPInfo.CommandLineOptions.alwaystrace );
        ASS_GVAR( "SilentNonInteractiveErrors", false );
        return;
    end, function (  )
        local nondigits, digits, i, char, have, need, haveint, needint;
        if
         not IS_STRING( GAPInfo.NeedKernelVersion )
              and not GAPInfo.KernelVersion in GAPInfo.NeedKernelVersion
            or IS_STRING( GAPInfo.NeedKernelVersion )
              and GAPInfo.KernelVersion <> GAPInfo.NeedKernelVersion then
            Print( "\n\n", "You are running a GAP kernel which does not fit with the library.\n\n" );
            nondigits := [  ];
            digits := "0123456789";
            for i in [ 0 .. 255 ] do
                char := CHAR_INT( i );
                if not char in digits then
                    ADD_LIST_DEFAULT( nondigits, char );
                fi;
            od;
            have := SplitStringInternal( GAPInfo.KernelVersion, nondigits, "" );
            need := SplitStringInternal( GAPInfo.NeedKernelVersion, nondigits, "" );
            haveint := [  ];
            needint := [  ];
            for i in [ 1 .. 3 ] do
                haveint[i] := IntHexString( have[i] );
                needint[i] := IntHexString( need[i] );
            od;
            Print( "Current kernel version:   ", haveint[1], ".", haveint[2], ".", haveint[3], "\n" );
            Print( "Library requires version: ", needint[1], ".", needint[2], ".", needint[3], "\n" );
            if haveint > needint then
                Print( "The GAP kernel is newer than the library.\n\n" );
            else
                Print(
                 "The GAP kernel is older than the library. Perhaps you forgot to recompile?\n\n" );
            fi;
            Error( "Update to correct kernel version!\n\n" );
        fi;
        return;
    end, function (  )
        local tmp, a;
        tmp := [  ];
        for a in REC_NAMES( OPER_FLAGS ) do
            ADD_LIST( tmp, OPER_FLAGS.(a) );
            Unbind( OPER_FLAGS.(a) );
        od;
        for a in tmp do
            OPER_FLAGS.(MASTER_POINTER_NUMBER( a[1] )) := a;
        od;
        return;
    end, function (  )
        ASS_GVAR( "ERROR_COUNT", 0 );
        ASS_GVAR( "ErrorLevel", 0 );
        ;
        ASS_GVAR( "QUITTING", false );
        return;
    end, function (  )
        local env, pos, enc, a, PositionSublist;
        PositionSublist := function ( str, sub )
              local i;
              for i in [ 1 .. Length( str ) - Length( sub ) + 1 ] do
                  if str{[ i .. i + Length( sub ) - 1 ]} = sub then
                      return i;
                  fi;
              od;
              return fail;
          end;
        if not IsBound( GAPInfo.TermEncodingOverwrite ) then
            if IsList( GAPInfo.SystemEnvironment ) then
                env := rec(
                     );
                for a in GAPInfo.SystemEnvironment do
                    pos := Position( a, '=' );
                    env.(a{[ 1 .. pos - 1 ]}) := a{[ pos + 1 .. Length( a ) ]};
                od;
            else
                env := GAPInfo.SystemEnvironment;
            fi;
            enc := fail;
            if IsBound( env.LC_CTYPE ) then
                enc := env.LC_CTYPE;
            fi;
            if enc = fail and IsBound( env.LC_ALL ) then
                enc := env.LC_ALL;
            fi;
            if enc = fail and IsBound( env.LANG ) then
                enc := env.LANG;
            fi;
            if enc <> fail then
                enc := STRING_LOWER( enc );
                if PositionSublist( enc, "utf-8" ) <> fail or PositionSublist( enc, "utf8" ) <> fail
                    then
                    GAPInfo.TermEncoding := "UTF-8";
                fi;
            fi;
            if not IsBound( GAPInfo.TermEncoding ) then
                GAPInfo.TermEncoding := "ISO-8859-1";
            fi;
        else
            GAPInfo.TermEncoding := GAPInfo.TermEncodingOverwrite;
        fi;
        MakeImmutable( GAPInfo.TermEncoding );
        return;
    end, function (  )
        if
         not (GAPInfo.CommandLineOptions.q or GAPInfo.CommandLineOptions.b
               or GAPInfo.CommandLineOptions.L <> "") then
            ShowKernelInformation(  );
        fi;
        return;
    end, function (  )
        ASS_GVAR( "IN_LOGGING_MODE", false );
        return;
    end, function (  )
        if IsBool( GAPInfo.DirectoryCurrent ) then
            GAPInfo.DirectoryCurrent := Directory( "./" );
        fi;
        return GAPInfo.DirectoryCurrent;
    end, function (  )
        READ_GAP_ROOT( "gap.ini" );
        return;
    end, function (  )
        if not GAPInfo.CommandLineOptions.O and UserPreference( "ReadObsolete" ) <> false
            and not IsBound( GAPInfo.Read_obsolete_gd ) then
            ReadLib( "obsolete.gd" );
            GAPInfo.Read_obsolete_gd := true;
        fi;
        return;
    end, function (  )
        local env, pos, enc, a;
        if not IsBound( GAPInfo.TermEncodingOverwrite ) then
            if IsList( GAPInfo.SystemEnvironment ) then
                env := rec(
                     );
                for a in GAPInfo.SystemEnvironment do
                    pos := Position( a, '=' );
                    env.(a{[ 1 .. pos - 1 ]}) := a{[ pos + 1 .. Length( a ) ]};
                od;
            else
                env := GAPInfo.SystemEnvironment;
            fi;
            enc := fail;
            if IsBound( env.LC_CTYPE ) then
                enc := env.LC_CTYPE;
            fi;
            if enc = fail and IsBound( env.LC_ALL ) then
                enc := env.LC_ALL;
            fi;
            if enc = fail and IsBound( env.LANG ) then
                enc := env.LANG;
            fi;
            if
             enc <> fail and (PositionSublist( enc, ".UTF-8" ) <> fail
                  or PositionSublist( enc, ".utf8" ) <> fail) then
                GAPInfo.TermEncoding := "UTF-8";
            fi;
            if not IsBound( GAPInfo.TermEncoding ) then
                GAPInfo.TermEncoding := "ISO-8859-1";
            fi;
        else
            GAPInfo.TermEncoding := GAPInfo.TermEncodingOverwrite;
        fi;
        MakeImmutable( GAPInfo.TermEncoding );
        return;
    end, function (  )
        local banner, msg, pair, excludedpackages, name, record;
        if IsBoundGlobal( "BANNER" ) then
            banner := ValueGlobal( "BANNER" );
            MakeReadWriteGlobal( "BANNER" );
            UnbindGlobal( "BANNER" );
        fi;
        BindGlobal( "BANNER", false );
        if GAPInfo.CommandLineOptions.L = "" then
            msg := "entering AutoloadPackages (no workspace)";
        else
            msg := Concatenation( "entering AutoloadPackages (workspace ", GAPInfo.CommandLineOptions.L
                , ")" );
        fi;
        LogPackageLoadingMessage( PACKAGE_DEBUG, msg, "GAP" );
        SetInfoLevel( InfoPackageLoading, UserPreference( "InfoPackageLoadingLevel" ) );
        GAPInfo.ExcludeFromAutoload := [  ];
        GAPInfo.PackagesInfoInitialized := false;
        InitializePackagesInfoRecords(  );
        GAPInfo.delayedImplementationParts := [  ];
        if ForAny( GAPInfo.Dependencies.NeededOtherPackages, function ( p )
                  return not IsBound( GAPInfo.PackagesLoaded.(p[1]) );
              end ) then
            LogPackageLoadingMessage( PACKAGE_DEBUG,
             Concatenation( [ "trying to load needed packages" ],
               List( GAPInfo.Dependencies.NeededOtherPackages, function ( pair )
                      return Concatenation( pair[1], " (", pair[2], ")" );
                  end ) ), "GAP" );
            if GAPInfo.CommandLineOptions.A then
                PushOptions( rec(
                    OnlyNeeded := true ) );
            fi;
            for pair in GAPInfo.Dependencies.NeededOtherPackages do
                if LoadPackage( pair[1], pair[2], false ) <> true then
                    LogPackageLoadingMessage( PACKAGE_ERROR, Concatenation( "needed package ",
                       pair[1], " cannot be loaded" ), "GAP" );
                    Error( "failed to load needed package `", pair[1], "' (version ", pair[2], ")" );
                fi;
            od;
            LogPackageLoadingMessage( PACKAGE_DEBUG, "needed packages loaded", "GAP" );
            if GAPInfo.CommandLineOptions.A then
                PopOptions(  );
            fi;
        fi;
        if not IsBound( GAPInfo.LibraryLoaded ) then
            LogPackageLoadingMessage( PACKAGE_DEBUG, [ "read the impl. part of the GAP library" ],
             "GAP" );
            ReadGapRoot( "lib/read.g" );
            GAPInfo.LibraryLoaded := true;
            GAPInfo.LoadPackageLevel := GAPInfo.LoadPackageLevel + 1;
            LoadPackage_ReadImplementationParts( GAPInfo.delayedImplementationParts, false );
            GAPInfo.LoadPackageLevel := GAPInfo.LoadPackageLevel - 1;
        fi;
        Unbind( GAPInfo.delayedImplementationParts );
        MakeReadWriteGlobal( "BANNER" );
        UnbindGlobal( "BANNER" );
        if IsBound( banner ) then
            BindGlobal( "BANNER", banner );
        fi;
        if GAPInfo.CommandLineOptions.A then
            LogPackageLoadingMessage( PACKAGE_DEBUG,
             "omitting packages suggested via \"PackagesToLoad\" (-A option)", "GAP" );
        elif ValueOption( "OnlyNeeded" ) = true then
            LogPackageLoadingMessage( PACKAGE_DEBUG,
             [ "omitting packages suggested via \"PackagesToLoad\"", " ('OnlyNeeded' option)" ], "GAP"
             );
        elif ForAny( List( UserPreference( "PackagesToLoad" ), LowercaseString ), function ( p )
                  return not IsBound( GAPInfo.PackagesLoaded.(p) );
              end ) then
            excludedpackages := List( UserPreference( "ExcludeFromAutoload" ), LowercaseString );
            LogPackageLoadingMessage( PACKAGE_DEBUG,
             Concatenation( [ "trying to load suggested packages" ], UserPreference( "PackagesToLoad"
                 ) ), "GAP" );
            for name in UserPreference( "PackagesToLoad" ) do
                if LowercaseString( name ) in excludedpackages then
                    LogPackageLoadingMessage( PACKAGE_DEBUG,
                     Concatenation( "excluded from autoloading: ", name ), "GAP" );
                elif not IsBound( GAPInfo.PackagesLoaded.(LowercaseString( name )) ) then
                    LogPackageLoadingMessage( PACKAGE_DEBUG,
                     Concatenation( "considering for autoloading: ", name ), "GAP" );
                    if LoadPackage( name, false ) <> true then
                        LogPackageLoadingMessage( PACKAGE_DEBUG, Concatenation( "suggested package ",
                           name, " cannot be loaded" ), "GAP" );
                    else
                        LogPackageLoadingMessage( PACKAGE_DEBUG, Concatenation( name, " loaded" ),
                         "GAP" );
                    fi;
                fi;
            od;
            LogPackageLoadingMessage( PACKAGE_DEBUG, "suggested packages loaded", "GAP" );
        fi;
        LogPackageLoadingMessage( PACKAGE_DEBUG,
         "call LoadPackageDocumentation for not loaded packages", "GAP" );
        for name in RecNames( GAPInfo.PackagesInfo ) do
            if not IsBound( GAPInfo.PackagesLoaded.(name) ) then
                record := First( GAPInfo.PackagesInfo.(name), IsRecord );
                if record <> fail then
                    LoadPackageDocumentation( record );
                fi;
            fi;
        od;
        LogPackageLoadingMessage( PACKAGE_DEBUG,
         "LoadPackageDocumentation for not loaded packages done", "GAP" );
        Unbind( GAPInfo.ExcludeFromAutoload );
        LogPackageLoadingMessage( PACKAGE_DEBUG, "return from AutoloadPackages", "GAP" );
        return;
    end, function (  )
        local xy;
        xy := [  ];
        if GAPInfo.CommandLineOptions.x <> "" then
            xy[1] := SMALLINT_STR( GAPInfo.CommandLineOptions.x );
        fi;
        if GAPInfo.CommandLineOptions.y <> "" then
            xy[2] := SMALLINT_STR( GAPInfo.CommandLineOptions.y );
        fi;
        if xy <> [  ] then
            SizeScreen( xy );
        fi;
        if GAPInfo.CommandLineOptions.g = 0 then
            SetGasmanMessageStatus( "none" );
        elif GAPInfo.CommandLineOptions.g = 1 then
            SetGasmanMessageStatus( "full" );
        else
            SetGasmanMessageStatus( "all" );
        fi;
        GAPInfo.ViewLength := UserPreference( "ViewLength" );
        ColorPrompt( UserPreference( "UseColorPrompt" ) );
        return;
    end, function (  )
        if not GAPInfo.CommandLineOptions.O and UserPreference( "ReadObsolete" ) <> false
            and not IsBound( GAPInfo.Read_obsolete_gi ) then
            ReadLib( "obsolete.gi" );
            GAPInfo.Read_obsolete_gi := true;
        fi;
        return;
    end, function (  )
        if UserPreference( "SaveAndRestoreHistory" ) = true then
            InstallAtExit( SaveCommandLineHistory );
            ReadCommandLineHistory(  );
        fi;
        return;
    end, function (  )
        if READ_GAP_ROOT( "gaprc" ) then
            GAPInfo.HasReadGAPRC := true;
        elif not IsExistingFile( GAPInfo.UserGapRoot ) then
            if not (GAPInfo.CommandLineOptions.r or GAPInfo.HasReadGAPRC) then
                if ARCH_IS_UNIX(  ) then
                    GAPInfo.gaprc := SHALLOW_COPY_OBJ( GAPInfo.UserHome );
                    if IsString( GAPInfo.gaprc ) then
                        APPEND_LIST_INTR( GAPInfo.gaprc, "/.gaprc" );
                    fi;
                else
                    GAPInfo.gaprc := "gap.rc";
                fi;
                if IsString( GAPInfo.gaprc ) and READ( GAPInfo.gaprc ) then
                    Info( InfoWarning, 1, "You are using an old ", GAPInfo.gaprc, " file. " );
                    Info( InfoWarning, 1, "See '?Ref: The former .gaprc file' for hints to upgrade." );
                    GAPInfo.HasReadGAPRC := true;
                fi;
            fi;
        fi;
        return;
    end, function (  )
        if not (GAPInfo.CommandLineOptions.q or GAPInfo.CommandLineOptions.b) then
            if GAPInfo.CommandLineOptions.L <> "" then
                ShowKernelInformation(  );
            fi;
            ShowPackageInformation(  );
        fi;
        return;
    end, function (  )
        local i, status;
        for i in [ 1 .. Length( GAPInfo.InitFiles ) ] do
            status := READ_NORECOVERY( GAPInfo.InitFiles[i] );
            if status = fail then
                PRINT_TO( "*errout*", "Reading file \"", GAPInfo.InitFiles[i],
                 "\" has been aborted.\n" );
                if i < Length( GAPInfo.InitFiles ) then
                    PRINT_TO( "*errout*",
                     "The remaining files on the command line will not be read.\n" );
                fi;
                break;
            elif status = false then
                PRINT_TO( "*errout*", "Could not read file \"", GAPInfo.InitFiles[i], "\".\n" );
            fi;
        od;
        return;
    end ]

comment:203 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:204 in reply to: ↑ 193 ; follow-up: Changed 13 months ago by jdemeyer

Replying to embray:

I think the obvious one, at least broadly speaking, is just having a hook for what Panic actually does.

The obvious thing to do would be to treat this as any other error condition.

comment:205 in reply to: ↑ 204 Changed 13 months ago by embray

Replying to jdemeyer:

Replying to embray:

I think the obvious one, at least broadly speaking, is just having a hook for what Panic actually does.

The obvious thing to do would be to treat this as any other error condition.

Yes, but I don't know that that's necessarily the right thing to do in the case of GAP. Much of GAP's error handling code is actually written in the GAP language (in the error.g library), where as Panic() seems primarily reserved for cases where the GAP kernel cannot be guaranteed to work anymore, much handle errors correctly.

A similar, but more subtle approach, would be to bypass most of the normal higher-level error-handling functionality, but still use JUMP_TO_CATCH to allow the error to be handled. If the GAP interpreter process wants to just exit upon such errors it can do that in the appropriate CATCH handler.

That's probably still naïve but I would bet it's closer to what they might have in mind to do. Of course, we can always ask...

comment:206 Changed 13 months ago by embray

  • Description modified (diff)

I was slightly barking up the wrong tree with the PostRestoreFuncs. Turns out it's hanging right after that in the function called SESSION. That's where it invokes that problematic SHELL() call:

 36 BIND_GLOBAL("SESSION",
 37     function()
 38     local   prompt;
 39
 40     if GAPInfo.CommandLineOptions.q then
 41         prompt := "";
 42     else
 43         prompt := "gap> ";
 44     fi;
 45
 46     SHELL( GetBottomLVars(), # in global context
 47         false, # no return
 48         false, # no return  obj
 49         3,     # set last, last2 and last3 each command
 50         true,  # set time after each command
 51         prompt,
 52         function()
 53             if IsBound(OnGAPPromptHook) and IsFunction(OnGAPPromptHook) then
 54                 OnGAPPromptHook();
 55             else
 56                 return;
 57             fi;
 58         end,
 59         "*stdin*",
 60         "*stdout*",
 61         true);
 62
 63     BreakOnError := false;
 64 end);

I'm still no exactly sure what this is for or why it causes libgap to hang in Sage. But it seems we can avoid it by using the --norepl option. What's strange is that --nointeract should imply --norepl but that doesn't appear to be working. That would have been the case with the off-by-one error in argc that Dima pointed out. But I fixed that and it didn't seem to help :/

Perhaps this is simple...

Last edited 13 months ago by embray (previous) (diff)

comment:207 Changed 13 months ago by jdemeyer

  • Description modified (diff)

Erik, please take care not the change the ticket description again... you accidentally reverted a chance that I made twice now.

comment:208 follow-up: Changed 13 months ago by embray

This is basically the issue that was reported in https://github.com/gap-system/gap/pull/2840/files, but it appears to still be broken in some way if I load an existing workspace. The --nointeract and/or --norepl options get clobbered.

comment:209 Changed 13 months ago by embray

Sorry. Dangers of using an outdated platform.

comment:210 in reply to: ↑ 208 Changed 13 months ago by embray

Replying to embray:

This is basically the issue that was reported in https://github.com/gap-system/gap/pull/2840/files, but it appears to still be broken in some way if I load an existing workspace. The --nointeract and/or --norepl options get clobbered.

I realized somewhat too late that although this PR was merged well before 4.10.0 was released, that fix was never included in the release. So either we wait for 4.10.1 (and see if we can ensure that this fix is included) or we patch it, or work around it somehow. I'm looking into maybe using a gaprc file to hack around it, but I don't have high hopes for that...

comment:211 follow-up: Changed 13 months ago by embray

Ok. Putting the following gaprc in GAP_ROOT works around it:

if GAPInfo.CommandLineOptions.norepl then
    # GAP 4.10.0 has a bug that an interactive session will be started
    # even if --norepl was set; see https://github.com/gap-system/gap/pull/2840
    # To work around this we redefine the SESSION function to a no-op
    MAKE_READ_WRITE_GLOBAL("SESSION");
    UNBIND_GLOBAL("SESSION");
    BIND_GLOBAL("SESSION", function() end);
fi;

my one concern is that if there is a gaprc in the UserGapRoot this will be ignored. I'm not 100% sure if that's the way it works or not. Need to double-check.

Last edited 13 months ago by embray (previous) (diff)

comment:212 in reply to: ↑ 211 Changed 13 months ago by embray

Replying to embray:

my one concern is that if there is a gaprc in the UserGapRoot this will be ignored. I'm not 100% sure if that's the way it works or not. Need to double-check.

Yes. That does appear to be a problem... Another option might be to install this via a gap.ini, but that would theoretically have the same problem I think.

comment:213 Changed 13 months ago by embray

A third possibility, which may be good enough for Sage's case, would be to install a pre-configured "default workspace" that includes this fix, and just always load that workspace if no other is found.

comment:214 follow-up: Changed 13 months ago by embray

Ah, best of all (I feel like I discovered this once a long time ago too): If we just pass a filename argument to libgap's init it will read that filename too, after everything else. This happens during the PostRestoreFuncs processing so it's still early enough, before the session is initialized.

Therefore for Sage's libgap initialization we can include a file with any number of patches to GAP.

comment:215 Changed 13 months ago by git

  • Commit changed from 30d1f7fa95fcc9855077c5ed6587748eed1a4cce to ed8fc3058db547b2af7ab1e72e468a60aa92895c

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

28535a5remove (apparently?) superfluous gap_eval
97be615when a statement in libgap.eval returns NULL, just return Python's None instead; makes for empty output to the command prompt
2b7bdcdUse ELM_PLIST directly instead of ELM_LIST when processing GAP_EvalString
ed8fc30install a special file called sage.gaprc that is read at the end of libgap

comment:216 Changed 13 months ago by embray

^ An attempt at a fix. Seems to work, and I'm pretty happy with the approach.

Last edited 13 months ago by embray (previous) (diff)

comment:217 in reply to: ↑ 214 Changed 13 months ago by embray

Replying to embray:

Ah, best of all (I feel like I discovered this once a long time ago too): If we just pass a filename argument to libgap's init it will read that filename too, after everything else. This happens during the PostRestoreFuncs processing so it's still early enough, before the session is initialized.

Therefore for Sage's libgap initialization we can include a file with any number of patches to GAP.

I see now that the pexpect interface does something similar with sage.g. It just lives in SAGE_EXTCODE rather than being installed in the package itself (I obviously prefer the latter). For libgap we probably shouldn't need most of the stuff in sage.g but I'm not sure.

comment:218 Changed 13 months ago by git

  • Commit changed from ed8fc3058db547b2af7ab1e72e468a60aa92895c to f3a909cdec30f70891b515fed05c53072fdfec09

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

f3a909cdifferent way of repr-ing a GAP element more consistent with the previous method

comment:219 follow-up: Changed 13 months ago by embray

It appears that next we need to focus on the GAP pexpect interface, which is also broken. It just hangs as soon as you try to do anything :(

comment:220 in reply to: ↑ 219 Changed 13 months ago by embray

Replying to embray:

It appears that next we need to focus on the GAP pexpect interface, which is also broken. It just hangs as soon as you try to do anything :(

It appears to get lost in gap_reset_workspace while loading the packages 'sonata' and 'guava'. It doesn't run into problems with other packages. So there are two problems:

1) The fact that something is broken when loading those packages

2) The fact that Sage's pexpect parser for GAP seems to go into an infinite loop upon those error conditions (it even prints "Warning: this should never happen"--so whatever "this" is apparently can happen, and the parser does not behave well when it does.

comment:221 follow-up: Changed 13 months ago by alexk

Looking at this ticket. Of course it became very long and I can't get an overview of all thread, but some comments while scrolling through it:

1) There is a smaller archive https://www.gap-system.org/pub/gap/gap4core/gap-4.10.0-core.zip of the core GAP system and four _required_ packages, if it helps to reduce the bandwidth. You can then get matching package archive from https://www.gap-system.org/pub/gap/gap4pkgs/ (ask me which). Also, may be helpful to reduce interaction with packages

2) From doc/dev - PostRestoreFunctions are called after a workspace has been loaded to finish linking up the kernel and workspace. Some of those from GAPInfo.PostRestoreFunctions may be added by packages (GAPDoc, GRAPE).

3) Did you get it to work without the workspace already?

4) "A lot of the test suites under tst/ seem to work, or mostly work" - the word "mostly" sounds alarming. see e.g. https://travis-ci.org/gap-system/gap-docker-stable-4.10-testsuite - everything should work there.

5) doc directory of GAP - of course, please include it. Allow the user to use it from GAP command line via ?<search_term> mechanism.

All from me for now!

Last edited 13 months ago by alexk (previous) (diff)

comment:222 follow-up: Changed 13 months ago by alexk

Try to load GUAVA and SONATA in GAP session and check their output:

gap> LoadPackage("sonata");
#I  You may wish to install the xgap package
#I  and enjoy the graphic capabilities of SONATA.

  ___________________________________________________________________________
 /        ___
||       /   \                 /\    Version 2.9.1
||      ||   ||  |\    |      /  \               /\       Erhard Aichinger
 \___   ||   ||  |\\   |     /____\_____________/__\      Franz Binder
     \  ||   ||  | \\  |    /      \     ||    /    \     Juergen Ecker
     ||  \___/   |  \\ |   /        \    ||   /      \    Peter Mayr
     ||          |   \\|  /          \   ||               Christof Noebauer
 \___/           |    \|                 ||

 System    Of   Nearrings     And      Their Applications
 Info: https://gap-packages.github.io/sonata/

true
gap> LoadPackage("guava");

   ____                          |
  /            \           /   --+--  Version 3.14
 /      |    | |\\        //|    |
|    _  |    | | \\      // |     the GUAVA Group
|     \ |    | |--\\    //--|     
 \     ||    | |   \\  //   |     
  \___/  \___/ |    \\//    |      
                                  

true
gap>

You may want to use the 2nd argument to disable their banners:

gap> LoadPackage("sonata",false);
#I  You may wish to install the xgap package
#I  and enjoy the graphic capabilities of SONATA.
true
gap> LoadPackage("guava",false);
true
gap> 

comment:223 in reply to: ↑ 221 Changed 13 months ago by embray

Replying to alexk:

Looking at this ticket. Of course it became very long and I can't get an overview of all thread, but some comments while scrolling through it:

Yes, sorry for the length. We are just taking notes here as we go I think. Some point soon I will make a summary progress report and link to it in the description, perhaps giving some indication as to where to skip to in the comments.

comment:224 in reply to: ↑ 222 Changed 13 months ago by embray

Replying to alexk:

Try to load GUAVA and SONATA in GAP session and check their output:

It works normally, but when I run gap with the -p flag ("enable/disable package output mode") it hangs like:

@p1.@n ┌───────┐@n   GAP 4.10.0 of 01-Nov-2018@J@n │  GAP  │   https://www.gap-system.org@J@n └───────┘@n   Architecture: x86_64-pc-linux-gnu-default64@J@n Configuration:  gmp 6.0.0@n@J@n Loading the library @nand packages ...@J@!968002+@"06191+@#378371+@$19131+@%13743+@&63556+@!90963+@"6494+@#15447+@$0255+@%45192+@&63556+@!47803+@"4843+@#72844+@$0625+@%03152+@&63556+@!63803+@"1393+@#19753+@$8863+@%17602+@&63556+@!03712+@"6513+@#27151+@$3431+@%03171+@&63556+@!75471+@"1752+@#44851+@$1441+@%75241+@&63556+@!85661+@"6242+@#31881+@$0691+@%54511+@&63556+@!52121+@"3761+@#3368+@$687+@%1669+@&63556+@!48091+@"5051+@#45203+@$7951+@%1287+@&63556+@!18252+@"6241+@#42751+@$3632+@%6495+@&63556+@!5723+@"464+@#77637+@$1581+@%2345+@&63556+@!365+@"93+@#242821+@$4492+@%4735+@&63556+@n Packages:   GAPDoc 1.6.2@n, PrimGrp 3.3.2@n, SmallGrp 1.3@n, TransGrp 2.0.4@n@J@n Try '??help' for help. See also '?copyright', '?cite' and '?authors'@J@ngap> @iLoadPackage("sonata");
LoadPackage("sonata");
@rLoadPackage("sonata");@J@w3+XCN

I don't fully understand what -p means or what all the output above is, though I know that Sage's pexect interface to GAP uses this debug output extensively to understand what's going on.

Same if I do LoadPackage("sonata", false), so at least it has nothing to do with the banner??

If I press enter a few times while it's hanging (or really any other input) it eventually outputs

@fError, @fwindow system: @fIllegal Answer@f at /home/embray/src/sagemath/sage/local/share/gap/pkg/xgap-4.28/lib/color.gi:160@f called from@f@J@f<function "CreateColors">( <arguments> )@J@f called from read-eval loop @fat /home/embray/src/sagemath/sage/local/share/gap/pkg/xgap-4.28/lib/color.gi:220@J@nyou can 'quit;' to quit to outer loop, or@J@nyou can 'return;' to continue@J@fbrk> @e

@r@J@fbrk> @e
Last edited 13 months ago by embray (previous) (diff)

comment:225 Changed 13 months ago by embray

I see now---p is really intended for use by xgap and the like; it also just happens to be taken advantage of for Sage's pexpect interface as well (not unreasonably). This makes me wonder though why a couple of packages would stall out in this case. I hope it's not actually trying to talk to an X server...

comment:226 follow-up: Changed 13 months ago by embray

We need some way, when starting GAP for Sage's pexpect interface, to force "no, xgap is really not available"

comment:227 Changed 13 months ago by dimpase

Note that we are installing GAP with the patch

--- a/lib/package.gi    2016-11-12 14:25:15.000000000 +0000
+++ b/lib/package.gi    2017-01-23 22:45:39.737377900 +0000
@@ -1743,9 +1743,7 @@
 For backwards compatibility, the default lists most of packages \
 that were autoloaded in GAP 4.4 (add or remove packages as you like)."
     ],
-  default:= [ "autpgrp", "alnuth", "crisp", "ctbllib", "factint", "fga", 
-              "irredsol", "laguna", "polenta", "polycyclic", "resclasses", 
-              "sophus", "tomlib" ],
+  default:= [],
   values:= function() return RecNames( GAPInfo.PackagesInfo ); end,
   multi:= true,
   ) );

Perhaps it has become somewhat outdated in GAP 4.10.

comment:228 in reply to: ↑ 226 ; follow-up: Changed 13 months ago by arojas

Replying to embray:

We need some way, when starting GAP for Sage's pexpect interface, to force "no, xgap is really not available"

FWIW we (Arch) faced a similar issue some time ago:

https://bugs.archlinux.org/task/55174

My "fix" was to patch out loading xgap from sonata/PackageInfo.g

comment:229 Changed 13 months ago by dimpase

I've noticed that after installing this branch I still have gap in SAGE_LOCAL/bin pointing at the wrong location of GAP executable.

In fact, I also get gap-bin in SAGE_LOCAL/bin, and by right, gap should be a link to the latter.

Correction - one should change the script to point to the right GAP_DIR too, it seems...

#!/bin/sh
GAP_DIR=$SAGE_LOCAL/gap/latest
GAP_EXE=$GAP_DIR
exec "$GAP_EXE/gap" -l "$GAP_DIR" "$@"

It seems that spkg-install should remove the whole SAGE_LOCAL/gap/ to make sure to stale copies of GAP are used after the update.

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

comment:230 in reply to: ↑ 228 ; follow-up: Changed 13 months ago by embray

Replying to arojas:

Replying to embray:

We need some way, when starting GAP for Sage's pexpect interface, to force "no, xgap is really not available"

FWIW we (Arch) faced a similar issue some time ago:

https://bugs.archlinux.org/task/55174

My "fix" was to patch out loading xgap from sonata/PackageInfo.g

My solution is to add the following to sage.g:

  • src/ext/gap/sage.g

    diff --git a/src/ext/gap/sage.g b/src/ext/gap/sage.g
    index 2a4ce13..dafb003 100644
    a b  
    1 
    21#
    32# SAGE support utilities to read into the GAP session.
    43#
     4
     5# Prevent loading the xgap package; we use the -p flag to GAP in order to
     6# communicate with it via the pexpect interface; this is normally used by
     7# for an xgap window to communicate with GAP, so unfortunatelly setting this
     8# flag also allows the xgap package to be loaded and for some packages to
     9# attempt to communicate with a "window handler" that doesn't exist.
     10# Therefore we must explicitly disable loading of the xgap package.
     11
     12# Don't use SetUserPreference since that leads to reloading the workspace,
     13# which confusing the pexpect interface
     14if IsBound(GAPInfo.ExcludeFromAutoload) then
     15    Append(GAPInfo.ExcludeFromAutoload, "xgap");
     16else
     17    GAPInfo.ExcludeFromAutoload := [ "xgap" ];
     18fi;
     19
    520\$SAGE := rec();
    621
    722\$SAGE.OldPager := Pager;

This might be slightly fragile, as I don't think GAPInfo.ExcludeFromAutoload is meant to be manipulated directly (normally it should be set via SetUserPreference, but it's too late to do that in sage.g). Seems to work though.

The patch Dima mentioned isn't relevant here because none of those default packages use xgap. I don't think we should rely on that being patched out anyways.

comment:231 follow-up: Changed 13 months ago by embray

Dima, can I ask you about this patch?

  • src/sage/interfaces/gap.py

    commit 3d12d9c432e551c57241cf615d57e7cbd80fc23a
    Author: Dima Pasechnik <dimpase@gmail.com>
    Date:   Wed Sep 26 12:18:55 2018 +0100
    
        fixing all but one doctests in interfaces/gap.py
    
    diff --git a/src/sage/interfaces/gap.py b/src/sage/interfaces/gap.py
    index 55d41aa..6cf740c 100644
    a b class GapFunction(ExpectFunction): 
    16661666
    16671667            sage: print(gap.SymmetricGroup.__doc__)
    16681668            <BLANKLINE>
    1669             50 Group Libraries
     1669              50.1-12 SymmetricGroup
    16701670            <BLANKLINE>
    1671             When you start GAP, it already knows several groups. Currently GAP initially
    1672             knows the following groups:
     1671              ‣ SymmetricGroup( [filt, ]deg ) ─────────────────────────────────── function
    16731672            ...
    16741673        """
    16751674        M = self._parent

This test fails for me now. It gives the full output of that documentation file starting with "50 Group Libraries"...

That is not great though. If I ask for gap.SymmetricGroup.__doc__ it should only display the relevant section of that document, not the whole document. I wonder if there's some patch I removed, or some other reason to expect that asking for ? SymmetricGroup should only start on the relevant line...

comment:232 Changed 13 months ago by embray

I think I see the problem. This is a bit buggy in general (even before this ticket...)

comment:233 Changed 13 months ago by embray

We have some other serious problems with error handling, in particular in the areas where we go outside of what is currently possible with the libgap API--particularly when we do things like call functions directly with CALL_<N>ARGS. Unfortunately I don't see a great way around that right now. The current, very limited API provided to us has a way to get a pointer to a global variable (e.g. with GAP_ValueGlobalVariable) but then no way to really do anything with it--in particular no way to pass it as an argument to a function--without going outside the API.

That means that before errors occur somewhere down in the GAP kernel resulting in a longjump to STATE(ReadJmpError), we need to always ensure that it has somewhere to jump to that makes sense.

Currently when we wrap those calls in sig_on() and sig_off() we at least get a segfault handled by cysignals, but we can lose any useful error reporting from GAP if an error occurs somewhere that doesn't go through our error handling. Further confusing matters, there's an internal function ReadEvalError() that's kind of like a raise which just jump to the last TRY_IF_NO_ERROR. And unlike JUMP_TO_CALL it does not process our custom error handler. It can also just jump off into the weeds if we're not careful.

comment:234 Changed 13 months ago by embray

I think it would mostly clear things up if we either did away with sig_on() (or left it there for last-resort error handling) and added a similar macro, based on GAP's TRY_IF_NO_ERROR macro, the effect of which is also to set a return point for longjmps. If we don't do that then GAP can wind up jumping to some weird places, or even invalid longjmp buffers (resulting in segfaults).

comment:235 Changed 13 months ago by gh-ChrisJefferson

Re: 2b7bdcd. Don't replace ELM_LIST / LEN_LIST with ELM_PLIST / LEN_PLIST. You are going to get horrible crashes you will never fix.

LEN_LIST should be fine, use ELM0_LIST if you want to get the 0 for unbound elements of the list.

comment:236 in reply to: ↑ 230 Changed 13 months ago by dimpase

Replying to embray:

Replying to arojas:

Replying to embray:

We need some way, when starting GAP for Sage's pexpect interface, to force "no, xgap is really not available"

FWIW we (Arch) faced a similar issue some time ago:

https://bugs.archlinux.org/task/55174

My "fix" was to patch out loading xgap from sonata/PackageInfo.g

My solution is to add the following to sage.g: [...] This might be slightly fragile, as I don't think GAPInfo.ExcludeFromAutoload is meant to be manipulated directly (normally it should be set via SetUserPreference, but it's too late to do that in sage.g). Seems to work though.

xgap is a 20+ years old X11 code. It's not working on many X11 setups (won't build, or builds, but the resulting package does not work), is tricky get working on OSX or Windows, and there is a Jupyter-based replacement being worked on. I don't think Sage can realistically support it.

comment:237 in reply to: ↑ 231 Changed 13 months ago by dimpase

Replying to embray:

Dima, can I ask you about this patch?

  • src/sage/interfaces/gap.py

    commit 3d12d9c432e551c57241cf615d57e7cbd80fc23a
    Author: Dima Pasechnik <dimpase@gmail.com>
    Date:   Wed Sep 26 12:18:55 2018 +0100
    
        fixing all but one doctests in interfaces/gap.py
    
    diff --git a/src/sage/interfaces/gap.py b/src/sage/interfaces/gap.py
    index 55d41aa..6cf740c 100644
    a b class GapFunction(ExpectFunction): 
    16661666
    16671667            sage: print(gap.SymmetricGroup.__doc__)
    16681668            <BLANKLINE>
    1669             50 Group Libraries
     1669              50.1-12 SymmetricGroup
    16701670            <BLANKLINE>
    1671             When you start GAP, it already knows several groups. Currently GAP initially
    1672             knows the following groups:
     1671              ‣ SymmetricGroup( [filt, ]deg ) ─────────────────────────────────── function
    16731672            ...
    16741673        """
    16751674        M = self._parent

This test fails for me now. It gives the full output of that documentation file starting with "50 Group Libraries"...

That is not great though. If I ask for gap.SymmetricGroup.__doc__ it should only display the relevant section of that document, not the whole document. I wonder if there's some patch I removed, or some other reason to expect that asking for ?

No, you get the same with the Sage's current GAP 4.8.6.

SymmetricGroup should only start on the relevant line...

Sure.

I don't have an answer to this immediately - this particular change was a result of testing on a custom tarball I built from source; perhaps the upstream tarball packages docs in a bit different way. Let's postpone this until other things are done here.

comment:238 Changed 13 months ago by alexk

What I see with my limited knowledge of SageMath internals, you have interfaces/gap.py which mentions chapter 50 Group Libraries and in it 50.1-12 SymmetricGroup. If you check whether that section exists at http://www.gap-system.org/Manuals/doc/ref/chap50.html, you will see that it is now 50.1-10. GAP does not promise that the order and numbers of manual sections are the same, so for SageMath the best way would be to avoid such references.

Last edited 13 months ago by alexk (previous) (diff)

comment:239 Changed 13 months ago by alexk

Regarding XGAP and other packages, I'd prefer using user preferences over patches to GAP and /or packages. Furthermore, this is what is going on in SageMath with SONATA and XGAP. XGAP needs to be started from xgap.sh script - it can not be loaded via LoadPackage. The -p option to start GAP is reserved for XGAP. If you start GAP with -p, then you trick SONATA into assuming that XGAP is loaded. Can you do not call GAP with -p option, and in this case you do not to do anything with XGAP and SONATA at all.

comment:240 follow-up: Changed 13 months ago by alexk

P.S. rereading some earlier posts, it seems that you need -p for Sage's pexpect interface. Fine then. I suggest only that instead of tweaking GAPInfo.ExcludeFromAutoload in gap/sage.g you use another user preference:

##  These packages are not regarded as available. This doesn't work for
##  packages which are needed by the GAP library, or which are already
##  loaded in a workspace.
# SetUserPreference( "PackagesToIgnore", [ ] );

This way, you ignore this package if GAP is loaded for SageMath, but if it is compiled properly, it still can be used if one works with GAP directly.

comment:241 in reply to: ↑ 240 Changed 13 months ago by embray

Replying to alexk:

P.S. rereading some earlier posts, it seems that you need -p for Sage's pexpect interface. Fine then. I suggest only that instead of tweaking GAPInfo.ExcludeFromAutoload in gap/sage.g you use another user preference:

##  These packages are not regarded as available. This doesn't work for
##  packages which are needed by the GAP library, or which are already
##  loaded in a workspace.
# SetUserPreference( "PackagesToIgnore", [ ] );

This way, you ignore this package if GAP is loaded for SageMath, but if it is compiled properly, it still can be used if one works with GAP directly.

The problem with this is where to put the SetUserPreference call. It should go somewhere like a gap.ini but the thing I don't like gap.ini and gaprc handling is that it will only read the first one it finds. So even if I install a customized gap.ini to be read by Sage, if the user has a UserGapRoot containing their own gap.ini it will take full precedence, and there is no way, for example, to inherit also from a "system" gap.ini.

Therefore the solution that Sage has apparently used for some time is to read a file, passed on the command line, containing additional customizations when starting GAP. This ensures that it will always be read. But I found that putting SetUserPreference in at such a late stage in the initialization didn't work, especially when loading an existing workspace. I might be wrong about that but when I tried this it didn't work.

comment:242 Changed 13 months ago by embray

Yes, I tried it again and calling SetUserPreference late, after GAP initialization has already mostly completed, appears to have no effect if I then start loading some packages. I have to modify GAPInfo.ExcludeFromAutoload directly.

This would be avoidable if there were a sort of gap.ini that can be loaded early on in initialization that I could guarantee will be run, but unless I'm missing something that does not appear to be quite possible...

Last edited 13 months ago by embray (previous) (diff)

comment:243 Changed 13 months ago by alexk

I agree that you need to tweak GAPInfo in gap/sage.g - but it does not make sense to tweak ExcludeFromAutoload there - you better change PackagesToIgnore. XGAP is not autoloaded at all, so adding it there is not semantically correct.

But perhaps a better way would be to make gap.ini working, by passing a directory that contains the gap.ini file as needed by Sage to GAP using the -l option. See 9.2 GAP Root Directories to start with.

comment:244 Changed 13 months ago by alexk

P.S. -r option will disable reading user's .gap/gap.ini

comment:245 Changed 13 months ago by alexk

proof of concept:

$ gap -b -r -l "~/sage/;~/gap-4.10.0/"

Reading sage/gap.ini

comment:246 Changed 13 months ago by embray

I believe we do currently use -r actually, so maybe there's something to be said for that, though it's also unfortunate: Perhaps the user has some things they want to set, and we're now crippling that capability. Of course, that is the current case as well, and is not so good.

I'm also not sure where the best place would be to put a Sage-specific gap.ini. For Sage's own gap package we're setting up our own GAP root into which we could put such things, so that's straightforward.

But I really want Sage to be able to work better with a system version of GAP and in that case it's not so obvious. If we do something like -l /path/to/sage-specific-gap-root;/path/to/system/gap/root would it still be able to access packages and libraries in the system GAP root?

comment:247 follow-up: Changed 13 months ago by alexk

1) If the user has own ~/.gap that may be what they use to customise their own GAP installation - perhaps what they want to set for other GAP, not the one in Sage?

2) If you have own GAP root already, putting it there seems natural to me

3) yes, sure - it will access packages and libraries in the system GAP root. Just try my example without -b to the the full GAP banner.

Last edited 13 months ago by alexk (previous) (diff)

comment:248 in reply to: ↑ 247 Changed 13 months ago by embray

Replying to alexk:

1) If the user has own ~/.gap that may be what they use to customise their own GAP installation - perhaps what they want to set for other GAP, not the one in Sage?

Point being, I want to do away with the notion of a "GAP in Sage" being necessary as opposed to an existing GAP installation. Sage should work and integrate with an existing GAP and not provide its own GAP as its own remote universe separate from the GAP installation that a user might already be using. Practically-speaking that isn't totally possible yet, but I am trying to move in that direction.

3) yes, sure - it will access packages and libraries in the system GAP root. Just try my example without -b to the the full GAP banner.

Okay, I'll give that a try then. That said, I'm still not sure I want to rely on -r since that hinders a user from working in GAP directly and in Sage with the same configuration (to the extent possible).

comment:249 Changed 13 months ago by embray

Some progress: With a few mostly trivial exceptions I have all the tests in the following modules working

  • sage.interfaces.gap
  • sage.libs.gap.element
  • sage.libs.gap.libgap
  • sage.libs.gap.util

Some of the others not so much. For example sage.libs.gap.all_documented_functions times out. In that case I have to wonder if it's just because I have too many packages available or something. Will have to investigate that more later.

Things are still something of a mess so I'll work next Monday or Tuesday on cleaning up some more before I push my work. The biggest mess is still in the area of error handling, but I think I have something of a handle on it. I'm also still concerned about memory leaks, though I don't have any immediate evidence that this is a problem.

Last edited 13 months ago by embray (previous) (diff)

comment:250 Changed 13 months ago by embray

Even if I reduce the number of packages down to the minimum required for GAP to work, sage.libs.gap.all_documented_functions takes ages, and eventually times out. On GAP 4.8.6 it's also quite slow for me--takes about two minutes. But now it's even worse. Is this really something we need/want, and is there a reason it's quite so slow...?

comment:251 follow-up: Changed 13 months ago by dimpase

Let's limit the scope of this ticket to concentrate on the core problems: error handling and interaction with GAP's GC, something that needs a lot of skills. We can iron out the kinks in e.g. sage.libs.gap.all_documented_functions and in GAP workspaces handling later.

comment:252 in reply to: ↑ 251 Changed 13 months ago by embray

Replying to dimpase:

Let's limit the scope of this ticket to concentrate on the core problems: error handling and interaction with GAP's GC, something that needs a lot of skills. We can iron out the kinks in e.g. sage.libs.gap.all_documented_functions and in GAP workspaces handling later.

I agree, you're right. And there's more work to be done on that. I have something that mostly works, but I want to think a little more systematically about it and come up with something a bit cleaner.

Related to that, I think, alas, there is still a need for some kind of libgap_enter/exit functions like from Volker's libgap. Obviously the idea of the official "libgap API" would be such that nothing like this is needed. But right now there are still many areas where we are (more or less unavoidably) bypassing that and calling GAP kernel functions directly.

GASMAN needs to know where the bottom of the execution stack is (at least as far as GAP objects are present) in order to properly mark bags that are initialized in global variables, and I believe we may have some cases like that. I think this might explain some weird segfaults and other errors. I'm getting from time to time. The problem is that GAP assumes that everything stems from an initial InitializeGap call that's at the bottom of the stack. But when using GAP as a library that is clearly not true.

I'm not sure what the GAP developers intend us to do in this case, as this is clearly a problem for anyone using GAP as a library. Do we have to manage references ourselves even for local variables? That would seem fragile, whereas the old libgap_enter() very clearly marks the "bottom" of the stack so far as GASMAN needs to be concerned. I will ask.

Last edited 13 months ago by embray (previous) (diff)

comment:253 follow-up: Changed 13 months ago by dimpase

I received repeated assurances from GAP devs that these libgap_enter/libgap_exit funcs are not needed.

My understanding is that GASMAN searches the stack for pointers, and so the local vars are safe.

comment:254 in reply to: ↑ 253 ; follow-up: Changed 13 months ago by embray

Replying to dimpase:

I received repeated assurances from GAP devs that these libgap_enter/libgap_exit funcs are not needed.

My understanding is that GASMAN searches the stack for pointers, and so the local vars are safe.

I'm not sure that's true. I may be wrong, but the global variable StackBottomBags (which in official GAP, as opposed to the old libgap, is declared static so we can't easily do anything with it) is set once, when the InitBags function is called from InitializeGap.

Garbage collections then either search up from this location if StackBottomBags is below the top of the stack. Or if, for some reason, StackBottomBags is above, it will search down from there. But not both. ISTM that this implementation detail is just meant to account for both architectures were the stack grows up or down, but it still bakes in an apparent assumption that StackBottomBags is initialized from some stack frame outside any where GAP objects are initialized.

Since InitializeGap is called in some arbitrary stack frame that may be either above or below future stack frames that call GAP functions, this does not seem safe unless we ensure that all future calls to GAP functions happen to occur in a deeper stack frame than when InitializeGap is called, which means it needs to be called early in the Python interpreter startup, just for example.

Again, I'm probably missing something but I have my doubts about this...

Garbage collection issue aside, I also believe some kind of libgap_enter() is necessary for error handling to work correctly, or else you can wind up in situations where if an error occurs you can wind up longjmp-ing to some weird place, like back into the InitializeGap call, for example. The only other way around is to do as you originally did, and have our error_handler longjmp completely out of GAP to a location of our choosing, but that can cause GAP not to perform necessary error cleanup of its own (such as closing streams).

Last edited 13 months ago by embray (previous) (diff)

comment:255 Changed 13 months ago by embray

Another relatively minor, but difficult to deal with issue I encountered last week but only just now looked into:

Syntax errors are not handled "correctly" insofar as it ignores the custom ERROR_OUTPUT variable that has been provided, and just prints the error directly to the libc stderr. No bueno. It's a minor thing, but this alone has me thinking that the new libgap is not fully ready for prime-time. But if we can get stuff like this fixed in a GAP 4.10.x and set that as the minimum required version, that might be good enough, both for Sage and for downstream packagers. I'll open an issue about it.

On a broader note, I really wish GAP relied a lot less at the lower levels on printing to I/O streams for error reporting, and instead set an error message somewhere along with an error indicator (there is STATE(NrError) but no equivalent for storing error messages). Something along the lines of having something similar to a PyErr_Occurred() would be more helpful for libraries to use, and would still integrate easily into the GAP REPL (where printing becomes appropriate). I have a similar complaint about how ViewObj works. I will open issues about these as well but I don't know if we can expect a fix anytime soon. In the meantime it is okay (but not ideal) to work around by capturing the relevant I/O streams (but in the case of syntax errors even that workaround is broken :(

Last edited 13 months ago by embray (previous) (diff)

comment:256 follow-up: Changed 13 months ago by gh-ChrisJefferson

Some general comments from a gap developer.

We know the libgap is far from complete -- we will accept PRs to add functionality and (if it doesn't require Ganges to other code) are happy to add new functions in minor 4.10 releases.

For GASMAN, yes it does support stacks going up and down -- GASMAN over the years has run on all kinds of systems, including Amigas and Ataris. For Sage's purpose, the best option would be to the stack base at he base of the stack -- is it possible to get that in Python? If not, a function which sets the pointer to some value above any stack values is sufficient.

Could Sage try to run GAP in multiple threads? In that case we would have to track all the stacks. That is also possible, but would require a GAP core change.

We have had trouble in the past with error entering an infinite loop, and are happy to accept PRs to fix it. Any changes must not change functionality for existing GAP users, and break any packages (I realise that is a high bar). It is already possible (with care) to replace the break loop, and that replacement could store the error generated.

Changes to ViewObj? should be done with discussion. We are generally awarez and not entirely happy, with some printing functionality, but it's so fundamental it's very hard to change anything without breaking dozens of packages.

comment:257 in reply to: ↑ 256 Changed 13 months ago by embray

Replying to gh-ChrisJefferson:

For GASMAN, yes it does support stacks going up and down -- GASMAN over the years has run on all kinds of systems, including Amigas and Ataris. For Sage's purpose, the best option would be to the stack base at he base of the stack -- is it possible to get that in Python? If not, a function which sets the pointer to some value above any stack values is sufficient.

What do you mean by "a function which sets the pointer to some value above any stack values"? Right now nothing like that exists officially, although I could probably do it in some gnarly hack. Otherwise we would need a patch to GAP.

The best I could do would be to call GAP_Initialize as early as possible in the Python interpreter start-up, but for Sage at least--and I think likely for other applications--this would not be acceptable. Sage initializes GAP lazily only when it is first used. Adding GAP initialization to Sage start-up would noticeably impact startup time, especially in cases when the workspace needs to be reinitialized.

In any case, IIUC, setting the stack base for GAP from InitializeGap is not good for GAP used as a library, since there's no way to guarantee that the InitializeGap call will remain a base of future stack frames.

I'm not positive yet, but I think this is a reason for some problems I'm having with bag pointers being moved from under my feet once the number of GAP objects in memory grows large. For heap objects it's not a problem--they remain valid because we track our references to them and use the kindly-provided markBagsCallback interface to mark them all. But then when I attempt function calls (e.g. Filter) on large lists of GAP objects things start moving under my feet.

Could Sage try to run GAP in multiple threads? In that case we would have to track all the stacks. That is also possible, but would require a GAP core change.

In theory sure--anything could try to run GAP code multi-threaded. But that's never been safe AFAIK so I'm not worried about that here. In Python multi-threading is not very common anyways since limitations in the Python interpreter make it less useful for performance purposes, so multi-process parallelism is more common.

comment:258 follow-up: Changed 13 months ago by gh-ChrisJefferson

Is there any simple way to get a pointer to something early in the stack, before anything else that might call GAP? Does Sage run any early C function which doesn't return, where it could stash the stack pointer? Looking at Python itself, a _PyMain object is put on the stack very early on, which is they passed by pointer. Is it possible to get that pointer back?

EDIT: _PyCoreConfig also looks like it is set up very early and stored on the stack.

Last edited 13 months ago by gh-ChrisJefferson (previous) (diff)

comment:259 in reply to: ↑ 258 Changed 13 months ago by embray

Replying to gh-ChrisJefferson:

Is there any simple way to get a pointer to something early in the stack, before anything else that might call GAP? Does Sage run any early C function which doesn't return, where it could stash the stack pointer? Looking at Python itself, a _PyMain object is put on the stack very early on, which is they passed by pointer. Is it possible to get that pointer back?

EDIT: _PyCoreConfig also looks like it is set up very early and stored on the stack.

I was thinking about something like that, but in the end it still wouldn't help, at least not with GAP_Initialize() as it's currently defined:

void GAP_Initialize(int              argc,
                    char **          argv,
                    char **          env,
                    GAP_CallbackFunc markBagsCallback,
                    GAP_CallbackFunc errorCallback)
{
    InitializeGap(&argc, argv, env);
    SetExtraMarkFuncBags(markBagsCallback);
    STATE(JumpToCatchCallback) = errorCallback;

    GAP_True = True;
    GAP_False = False;
    GAP_Fail = Fail;
}

Here it passes &argc to IntializeGap, where the compiler effectively makes argc a stack local variable and thus &argc is a pointer to the GAP_Initialize stack frame.

I could, as you say, create (or abuse) some area earlier on the stack, and temporarily shove the value for argc in there. Then I would have manually re-implement GAP_Initialize so that I can pass that pointer to InitializeGap.

Point being, of course, none of this should be necessary, and there ought to be a way to set the stack bottom for GASMAN any time to wherever is appropriate (this is what libgap_enter did, primarily).

comment:260 follow-up: Changed 13 months ago by gh-ChrisJefferson

There are two seperate issues here:

1) We need a way to set the stack bottom in GASMAN. Indeed there is no such function. As has been said multiple times, the libgap api was only introduced a short while ago as a well-defined place to put functionality needed by code outside GAP, and we can easily add a function to it to allow passing in a stack base.

2) Do we need something more like the existing libgap_enter/leave, which kept updating this pointer all the time, and had to be called in pairs. I don't think we do, as long as we can get some pointer into the stack quite high up.

comment:261 in reply to: ↑ 254 ; follow-up: Changed 13 months ago by jdemeyer

Replying to embray:

I also believe some kind of libgap_enter() is necessary for error handling to work correctly

Isn't this exactly the sig_on()/sig_error() use case? Earlier, you said that longjmping using sig_error() was problematic.

comment:262 in reply to: ↑ 260 Changed 13 months ago by embray

Replying to gh-ChrisJefferson:

There are two seperate issues here:

1) We need a way to set the stack bottom in GASMAN. Indeed there is no such function. As has been said multiple times, the libgap api was only introduced a short while ago as a well-defined place to put functionality needed by code outside GAP, and we can easily add a function to it to allow passing in a stack base.

That's fine. I know (now) that it's incomplete. My fear is that it won't be "ready" enough to fully replace Sage's use of its custom GAP fork in time for the next release, and in particular in time for the upcoming Debian freeze. Regardless, now we must work together to discover these kinds of issues and resolve them.

2) Do we need something more like the existing libgap_enter/leave, which kept updating this pointer all the time, and had to be called in pairs. I don't think we do, as long as we can get some pointer into the stack quite high up.

Yes, that is possible. But there are some problems with that:

  • It places the burden on libgap users to have to worry about issues like stack frames, and ensuring that this is set up early in their application and in such a way that all future function calls are children to some call in which this is set up, which may be not always be obvious or trivial, especially (for example) if trying to implement a generic pyGAP library or the like. It could be done, but would require hacks.
  • It involves a kind of "non-local" behavior that is hard to debug. Actual calls to GAP functionality may not occur in an application until far from where this "stack bottom pointer" is initialized. It's also inefficient. I could grab and abuse a pointer to a local variable in Py_Main for example, but from there a typical code eval may be 30-40 stack frames deep, if not much more, none of which have anything to do with GAP. I haven't evaluated exactly how much that affects the efficiency of GAP's garbage collection, but it's certainly adding needless overhead.
  • Instead, having explicit libgap_enter/exit functions clearly demarcates where GAP-related code is expected to run. It is both clearer to the developer (they don't even have to understand what it's for or why--just document "all code that uses libgap must be demarcated with these functions/macros)"; it's clearer for the reader of existing code ("this is where the GAP stuff starts/ends"); and it's more efficient for the garbage collector ("this marks the end of stack frames I'm responsible for").

An alternative might be to design an extensive libgap API such that this kind of memory management responsibility is completely taken away from the user, by never allowing them to allocate new Bags in their own local variables, but instead ensures that all GAP objects returned by the API are allocated from some pool that is entirely managed by GAP. Then all GAP API calls could begin with the effective equivalent of "libgap_enter" while taking that responsibility entirely away from the user. I would definitely prefer only using functions from GAP that are designated as part of some public API. I know the API has already expanded significantly since the 4.10.0 release, but I don't think it has this feature in terms of memory management.

comment:263 in reply to: ↑ 261 ; follow-up: Changed 13 months ago by embray

Replying to jdemeyer:

Replying to embray:

I also believe some kind of libgap_enter() is necessary for error handling to work correctly

Isn't this exactly the sig_on()/sig_error() use case? Earlier, you said that longjmping using sig_error() was problematic.

libgap_enter/exit() has nothing really to do with error handling. The problem with calling our own longjmp from within a custom error handler is that it causes GAP's own internal error cleanup (in CATCH_ERROR blocks) blocks to be bypassed.

comment:264 in reply to: ↑ 263 ; follow-up: Changed 13 months ago by embray

Replying to embray:

Replying to jdemeyer:

Replying to embray:

I also believe some kind of libgap_enter() is necessary for error handling to work correctly

Isn't this exactly the sig_on()/sig_error() use case? Earlier, you said that longjmping using sig_error() was problematic.

libgap_enter/exit() has nothing really to do with error handling. The problem with calling our own longjmp from within a custom error handler is that it causes GAP's own internal error cleanup (in CATCH_ERROR blocks) blocks to be bypassed.

Sorry, I think I misread what you were responding to here, since I was apparently the one who mentioned libgap_enter in the context of error handling. What I meant here was that in addition to setting the stack bottom, libgap_enter might also be needed to work as something like sig_on(), but that uses an alternative location for the jmp_buf (specifically GAP's internal STATE(ReadErrorJump) as opposed to cysignals' cysigs.env). To that end I think an extension to cysignals might be useful for allowing it to work with an alternative location of the jmp_buf--for example as an alternative to sig_on().

comment:265 Changed 13 months ago by embray

Also I haven't pushed updates to my branch yet, but I already have a macro that sets sigsetjmp(STATE(ReadErrorJump)) before calls to any GAP functions. This is called after sig_on() so we get the best of both worlds. But I also think it would be possible, and handy, to have a way to merge the two into a single sig_on_altbuf call or something.

comment:266 Changed 13 months ago by git

  • Commit changed from f3a909cdec30f70891b515fed05c53072fdfec09 to 478d554716f58275be53342819f3c8d927092fcb

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

478d554some error-handling 'improvement'

comment:267 in reply to: ↑ 264 ; follow-up: Changed 13 months ago by jdemeyer

Replying to embray:

What I meant here was that in addition to setting the stack bottom, libgap_enter might also be needed to work as something like sig_on(), but that uses an alternative location for the jmp_buf (specifically GAP's internal STATE(ReadErrorJump) as opposed to cysignals' cysigs.env). To that end I think an extension to cysignals might be useful for allowing it to work with an alternative location of the jmp_buf--for example as an alternative to sig_on().

One immediate problem: would that be a jmp_buf or a sigjmp_buf? What cysignals uses depends on the platform: on Linux and OS X, it's sigjmp_buf. On other platforms, it's jmp_buf.

Does GAP really have to perform a longjmp()? Couldn't we propose to change GAP to use a callback function instead? That would be much more user friendly and it's also how other libraries implement error handling.

comment:268 follow-up: Changed 13 months ago by jdemeyer

The current libGAP-4.8 uses a callback mechanism for error handling (see libgap_set_error_handler). IMHO, porting that to GAP-4.10 would be a much better solution than messing with setjmp()/longjmp() calls.

comment:269 Changed 13 months ago by git

  • Commit changed from 478d554716f58275be53342819f3c8d927092fcb to 025740df47e6a3dd00929c0903e441d8584a1476

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

025740da little bit of gap_includes cleanup

comment:270 in reply to: ↑ 267 ; follow-ups: Changed 13 months ago by embray

Replying to jdemeyer:

Replying to embray:

What I meant here was that in addition to setting the stack bottom, libgap_enter might also be needed to work as something like sig_on(), but that uses an alternative location for the jmp_buf (specifically GAP's internal STATE(ReadErrorJump) as opposed to cysignals' cysigs.env). To that end I think an extension to cysignals might be useful for allowing it to work with an alternative location of the jmp_buf--for example as an alternative to sig_on().

One immediate problem: would that be a jmp_buf or a sigjmp_buf? What cysignals uses depends on the platform: on Linux and OS X, it's sigjmp_buf. On other platforms, it's jmp_buf.

GAP also has macros for these that depend on platform availability (e.g. syJmp_buf, which is a sigjmp_buf where available). As long as these macros are used it should be safe for a GAP and a cysignals that were configured and compiled for the same platform.

Does GAP really have to perform a longjmp()? Couldn't we propose to change GAP to use a callback function instead? That would be much more user friendly and it's also how other libraries implement error handling.

I don't think it really does. I wrote something a little like you're suggesting here. Internally it uses these longjmps to effectively raise/catch exceptions, and that's a design choice that it's not my place to question. But I think for integration as a library it would be nicer and simpler for everyone if it never long-jumped out of the GAP library at all, and instead just set some "error occurred" indicator with a way to retrieve the most recent error message (a la PyErr_Occurred + PyErr_Fetch).

comment:271 Changed 13 months ago by embray

Also, part of the problem is that as of 4.10.0, the official libGAP API does not offer as much functionality as needed, so we have to call some internal functions directly, and this means we have to provide some sensible return point for when errors occur.

comment:272 in reply to: ↑ 270 ; follow-up: Changed 13 months ago by jdemeyer

Replying to embray:

Internally it uses these longjmps to effectively raise/catch exceptions, and that's a design choice that it's not my place to question.

If if turns out that this makes error handling difficult when using GAP as a library, then it is our place to question that design choice. I never said that we should drop the ReadJmpError thing, we could add the callback as additional error handling mechanism. That's what PARI does for example: it allows a longjmp()-based error handler which is mostly for internal use and it has an error callback function which is used by our Python bindings to PARI.

comment:273 follow-up: Changed 13 months ago by gh-ChrisJefferson

I can't imagine what you would suggest we use instead of longjmp? If a GAP function raises an error, we need to return from multiple functions, back to some well-defined calling point (in practice, back to the user prompt). In a C system, you can really only do that with longjmp.

Of course, we can stop those internal longjmps from escaping a well-defined libgap API, by adding a longjmp in each ABI function that could cause an Error.

comment:274 in reply to: ↑ 273 Changed 13 months ago by jdemeyer

Replying to gh-ChrisJefferson:

I can't imagine what you would suggest we use instead of longjmp?

Various libraries (in particular, PARI, NTL, GLPK and Volker Braun's libGAP) call a callback function to handle errors. Of course, that callback function might also call some variant of longjmp(). In Sage, we use sig_on() to define a single setjmp() jump point that the various callback functions jump to.

A callback function also has the advantage that it can take arguments: you could pass it some info about the error and it may store that or take different actions depending on the error.

comment:275 follow-up: Changed 13 months ago by gh-ChrisJefferson

I'm leaving this thread. It isn't being productive, and asking that GAP stops using longjmp shows a fundamental misunderstanding of.. well C. Or rewriting the entire of GAP basically from scratch.

comment:276 Changed 13 months ago by git

  • Commit changed from 025740df47e6a3dd00929c0903e441d8584a1476 to a407140ae046980369a23934b5e13e1892a19f18

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

a407140more gap_includes cleanup: just use IS_LIST and IS_REC

comment:277 in reply to: ↑ 275 Changed 13 months ago by jdemeyer

Replying to gh-ChrisJefferson:

asking that GAP stops using longjmp

Nobody ever asked that. See 272

comment:278 in reply to: ↑ 270 Changed 13 months ago by jdemeyer

Replying to embray:

instead just set some "error occurred" indicator with a way to retrieve the most recent error message (a la PyErr_Occurred + PyErr_Fetch).

But that would require adding error propagation support to all (at least, a lot) GAP functions, no?

In Python for example, most C API functions return a PyObject* and a return value NULL means that an error occurred. This is a good convention, but it does require support for error propagation in virtually all Python/C API functions.

comment:279 in reply to: ↑ 268 ; follow-up: Changed 13 months ago by embray

Replying to jdemeyer:

The current libGAP-4.8 uses a callback mechanism for error handling (see libgap_set_error_handler). IMHO, porting that to GAP-4.10 would be a much better solution than messing with setjmp()/longjmp() calls.

I should add, this has already been done. It is the errorCallback function that is passed to GAP_Initialize. This serves the same purpose and sets a callback in the same place in the code. It doesn't strictly help the problem because this callback is in a location right before GAP longjmps to its CATCH_ERROR handler (if any). This is fine, but the problem is that sometimes this does not jump to a sensible place, but rather just to wherever the last TRY_IF_NO_ERROR macro was, which is not always in a sensible place (yet) for calls to the public API.

comment:280 in reply to: ↑ 272 Changed 13 months ago by embray

Replying to jdemeyer:

Replying to embray:

Internally it uses these longjmps to effectively raise/catch exceptions, and that's a design choice that it's not my place to question.

If if turns out that this makes error handling difficult when using GAP as a library, then it is our place to question that design choice. I never said that we should drop the ReadJmpError thing, we could add the callback as additional error handling mechanism. That's what PARI does for example: it allows a longjmp()-based error handler which is mostly for internal use and it has an error callback function which is used by our Python bindings to PARI.

The use of that mechanism does not, on its own, cause any problems and is perfectly reasonable to use for error handling within the GAP kernel. The only problem is that it's applied somewhat inconsistently at the moment, from the standpoint of third-party API consumers (who, ideally, should not be exposed to this implementation detail at all). Many of the problems I'm encountering related to this are because we're still currently using internal implementation details like CALL_<N>ARGS without setting up the appropriate error handling that the GAP kernel itself otherwise might.

Fortunately in GAP's master branch there's now a GAP_CallFuncArray, for example, that gives an "official" implementation allowing us to avoid (eventually) using these internal details. The API still doesn't get some of the error-handling details quite right (it does not fix the problem I describe above) but it does provide the appropriate area for making these improvements.

comment:281 in reply to: ↑ 279 Changed 13 months ago by jdemeyer

Replying to embray:

It doesn't strictly help the problem because this callback is in a location right before GAP longjmps to its CATCH_ERROR handler (if any). This is fine, but the problem is that sometimes this does not jump to a sensible place, but rather just to wherever the last TRY_IF_NO_ERROR macro was, which is not always in a sensible place (yet) for calls to the public API.

I think I understand what you are saying: there is no way to detect whether the TRY_IF_NO_ERROR block has ended. I.e. you would want something like

TRY_IF_NO_ERROR
{
  enable_error_recovery = 1;
  ....
  enable_error_recovery = 0;
}

and then check enable_error_recovery in the error handler to know whether one should longjmp() to the location set by TRY_IF_NO_ERROR?

comment:282 follow-up: Changed 13 months ago by embray

Yes, that's definitely part of it. After a TRY_IF_NO_ERROR there's no resetting of the mechanism to disable future jumps to that point.

I had some notes about this further up this already confusing ticket, but for example I had some cases where it was possible, if some error occurred very early on in Sage's libgap API, it could result in a longjmp back to InitializeGap (which just happened to be where the last actual TRY_IF_NO_ERROR was). What the GAP API needs is some reasonable terminal location to jump to (preferably within the GAP kernel code itself, or possibly the public API code in libgap-api.c) that can set an error indicator and return.

Incidentally, almost all (and there are some exception) of these jumps originate from calls to the function JUMP_TO_CATCH (this is where the error callback is called as well), and within GAP almost all calls to JUMP_TO_CATCH originate from the default error handler ErrorInner, which is implemented in GAP code in error.g. Chris mentioned to me that the GAP test suite runner actually messes with this in some way (I haven't looked yet), and that the GAP Juypter kernel also replaces ErrorInner. We might consider doing the same, which might simplify some issues. That's still not ideal, and should not be necessary--some internal cleanup is still needed. But replacing ErrorInner might be acceptable as a temporary workaround.

comment:283 Changed 13 months ago by embray

I also can't speak to how JUMP_TO_CATCH may be used, if at all, by third-party libraries. Really it shouldn't be used at all, but it might be somewhere. It's worth doing a search on...

comment:284 Changed 13 months ago by embray

It seems that, at least of the "officially supported" packages, the Browse and SCSCP packages mess around with ErrorInner as well, albeit in ways that I think at least aren't likely to come up when using GAP as a library (e.g. in the SCSCP package this only happens in running its included SCSCP server, which one generally would not do from library code...)

comment:285 in reply to: ↑ 282 Changed 13 months ago by jdemeyer

Replying to embray:

Yes, that's definitely part of it. After a TRY_IF_NO_ERROR there's no resetting of the mechanism to disable future jumps to that point.

And it doesn't look easy to add such a mechanism now.

What the GAP API needs is some reasonable terminal location to jump to (preferably within the GAP kernel code itself, or possibly the public API code in libgap-api.c) that can set an error indicator and return.

I would prefer not a "terminal location to jump to" but a callback function to be called.

Concretely, I would suggest the following:

  • add a macro to reset TRY_IF_NO_ERROR. This could be implemented as memset(STATE(ReadJmpError), 0, sizeof(syJmp_buf));
  • use this macro in GAP in the relevant places, in particular in InitializeGap.
  • have a way to check whether we are inside a TRY_IF_NO_ERROR block or not. Implemented by checking whether syJmp_buf consists of all-zero bytes.
  • in the GAP error callback in Sage, do the following:
    • if we are inside TRY_IF_NO_ERROR: return from the error callback
    • otherwise: call sig_error

comment:286 follow-up: Changed 13 months ago by embray

I don't see why you would need/want sig_error at all. I'm trying to think about ways this could be usefully implemented for all users of GAP as a library. My thinking is that the TRY_IF_NO_ERROR stuff should never leak out of the GAP kernel code at all, which is what I meant by a "terminal location to jump to"--that is--within GAP's own code. At this point it would check whether an error occurred, and should have a way of setting some indication of that fact that can be checked easily from third-party code, along with retrieving the error message. Being able to get a stack trace might be nice too but less important. I don't think a "callback function" is necessary at all, but could also be used I suppose. Though it would be more useful if said callback function were passed the actual error message, rather than having to wade through ErrorInner as we currently do, and read the error message out of a stream object that we're then also responsible for resetting :(

comment:287 in reply to: ↑ 286 Changed 13 months ago by jdemeyer

Replying to embray:

Though it would be more useful if said callback function were passed the actual error message

Of course. That's one of the advantages that a callback would give.

A callback function can run arbitrary code. So whatever specific mechanism that you want to use, you could always implement that using a callback. A callback can call longjmp() for example. Or a callback could set some global variables that you would want to check later.

It's also pretty standard to have an error callback function: PARI, NTL, GLPK have it.

comment:288 Changed 13 months ago by embray

Most sage.libs.gap doctests working, modulo some minor issues. But still getting segfaults during fresh docbuild :(

Suspect garbage collection problems regarding stack pointer issue, but that isn't confirmed.

comment:289 Changed 13 months ago by embray

Also have some doubts regarding interrupt handing being screwed up by InitializeGap.

comment:290 Changed 13 months ago by dimpase

failures during fresh docbuild might indicate multithreading problems. Does “make -j1 ...” make a difference?

comment:291 Changed 13 months ago by git

  • Commit changed from a407140ae046980369a23934b5e13e1892a19f18 to ec4c3a03e18d5789338b40e1e4fa22b56f8b0020

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

ec4c3a0fixes soem goofy bugs with the spkg-install for gap

comment:292 Changed 13 months ago by git

  • Commit changed from ec4c3a03e18d5789338b40e1e4fa22b56f8b0020 to 7ccc120e3a606e044bdd9ef6835930075fa70a31

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

7ccc120fixes some goofy bugs with the spkg-install for gap

comment:293 Changed 13 months ago by embray

I found a neat thing while digging around in the GAP code. It seems there is already basically a way to "throw" an "exception" with the JUMP_TO_CATCH calls.

JUMP_TO_CATCH already accepts a payload argument which can be any GAP object. It stows this away in a thread-local state variable called ThrownObject.

At the moment there is no particular convention for what should go there, and it is not used at all, for anything. But it certainly could be.

Perhaps it was once used for something else. Most existing JUMP_TO_CATCH pass some integer between 0 and 3,but the meaning of this is long lost and the GAP git repository doesn't go back far enough to find out what it might have once meant. I'd be curious to dig around on that a bit more.

But I could implement and document a convention for how this is intended to be used, for example, to make the last error message or something like that available. This would be useful as part of my larger vision for cleaning up GAP's error handling.

comment:294 Changed 13 months ago by embray

  • Description modified (diff)

Added some more current status updates.

comment:295 Changed 12 months ago by embray

I think at least some of the problems I'm having now are related to the issue that the old writeandcheck patch was intended to fix. I removed that patch just to see if it was still needed and it seems very much like it is. That issue is also closely related to the issue I opened here: https://github.com/gap-system/gap/issues/3028

I'm going to try re-applying the patch to see if it fixes any of the known problems I'm having, and then will work with the GAP team to find a fix that's acceptable to them. It's definitely a bug in GAP regardless of interaction with Sage.

comment:296 Changed 12 months ago by dimpase

so that's something renamed to echoandcheck(), right?

comment:297 Changed 12 months ago by embray

There's now echoandcheck and SyWriteandcheck where are nearly exact duplicates of each other except for a minor difference. The latter seems to be more directly affected but echoandcheck might be too.

I cobbled together an updated version of Jeroen's original patch for this, though I'm not exactly sure if that version is gonna fly, since I think there is some deeper cleanup of error handling needed.

The updated patch seems to have helped with some issues, but not others that I was hoping it might fix... :/

comment:298 Changed 12 months ago by embray

Definitely having some problems with gap + fork() and/or multiprocessing in general. Not exactly sure what the deal is yet.

comment:299 Changed 12 months ago by dimpase

Well, are you on Windows? This might be an extra weight to carry around...

comment:300 Changed 12 months ago by embray

Nope.

Interesting; I see now that there is some evil interaction between libgap and libecl (either the actual library or something to do with Sage's wrapper around it). Will investigate more next week.

comment:301 Changed 12 months ago by embray

Ah, if libgap happens to be initialized before libecl this is what happens:

/home/embray/src/sagemath/sage/local/lib/python2.7/site-packages/sage/interfaces/maxima_lib.py in <module>()
     91 from sage.symbolic.ring import SR
     92
---> 93 from sage.libs.ecl import EclObject, ecl_eval
     94
     95 from .maxima_abstract import (MaximaAbstract, MaximaAbstractFunction,

/home/embray/src/sagemath/sage/local/lib/python2.7/site-packages/sage/libs/ecl.pyx in init sage.libs.ecl (build/cythonized/sage/libs/ecl.c:13344)()
   1342     return ecl_wrap(o)
   1343
-> 1344 init_ecl()

/home/embray/src/sagemath/sage/local/lib/python2.7/site-packages/sage/libs/ecl.pyx in sage.libs.ecl.init_ecl (build/cythonized/sage/libs/ecl.c:5603)()
    271     cdef sigaction_t sig_test
    272     sigaction(SIGCHLD, NULL, &sig_test)
--> 273     assert sage_action[SIGCHLD].sa_handler == NULL  # Sage does not set SIGCHLD handler
    274     assert sig_test.sa_handler == NULL              # And ECL bootup did not set one

However, under some circumstances (I reproduced this under pdb) the AssertionError is being caught and ignored, so libecl initialization is incomplete and broken. Future ecl calls result in a segfault. Neat!

comment:302 Changed 12 months ago by embray

I didn't notice this before since it's set up by the InitKernel function in one of the GAP kernel modules (src/iostream.c), but GAP also installs its own special SIGCHLD handler, which it does even if it hasn't started any child processes.

We still want GAP to be able to clean up whatever I/O-related state it cleans up in this SIGCHLD handler. This need was anticipated somewhat in a CheckChildStatusChanged function that is not used by GAP itself, but made available for wrapping by applications that set their own SIGCHLD handler. This may need to be made available by the libgap API. I still need to think more about how best to handle this in Sage though. It would also be nice if Cysignals had a helper, in the form of a context manager perhaps, to save restore all registered signal handlers.

In any case, for now, just disabling GAP's SIGCHLD handler fixed most of the more severe problems I've been having.

comment:303 Changed 12 months ago by embray

Still have a number of doctests failing, but a lot more of them seem to have to do with changes to GAP itself, rather than with the interface to it. I'm not completely positive about that though. I need to go through the failing tests and classify the types of errors. Still a few segfaults--I suspect mostly to do with the problems with garbage collection of stack local variables (see https://trac.sagemath.org/ticket/22626#comment:252)

comment:304 Changed 12 months ago by dimpase

OK, this all sounds very promising!

Sorry for actually removing all these libgap_enter/exit pairs, it seems we need to put them back in now...

comment:305 Changed 12 months ago by embray

  • Description modified (diff)

I tried to describe the issue with premature garbage collection of stack local variables here: https://github.com/gap-system/gap/issues/3089 I'm still not 100% sure that this is the cause for some of the problems I'm seeing and need to try to perform a more careful analysis of a real-world case of this. But I'm not sure what else would be causing some of the problems I'm seeing with objects being randomly replaced with other objects.

comment:306 Changed 12 months ago by embray

Alright, I came up with some GAP_Enter and GAP_Leave macros--partially inspired by cysignals and partially by Volker's libgap--that handle both setting the stack bottom, and setting a sigsetjmp buffer for catching unhandled ReadJmpError errors from GAP.

I'm still running a ptestlong on Sage, and while there are still some failures these seem to have more to do with intentional changes to GAP, and for the most part are not severe errors (I'm not seeing any segfaults or other such oddities yet: I think we have the stack handling to thank for that).

There are also still some problems with broken handling of syntax errors. I think that will be fixed, at least partially, by https://github.com/gap-system/gap/pull/3043

comment:307 Changed 12 months ago by dimpase

I'd be happy fixing maths-related things in doctests, but I need your branch...

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

I need to clean some things up; I'll work on that today. It also needs some custom patches to GAP, unfortunately... Fortunately they are patches that should be acceptable, in some form or other, upstream as well.

Last night I ran a make ptestlong against my current branches and am down to the following failures:

sage -t --long src/sage/groups/matrix_gps/finitely_generated.py  # Bad exit: 1
sage -t --long src/sage/combinat/tiling.py  # 1 doctest failed
sage -t --long src/sage/graphs/strongly_regular_db.pyx  # 1 doctest failed
sage -t --long src/sage/coding/linear_code.py  # 4 doctests failed
sage -t --long src/sage/groups/perm_gps/permgroup.py  # 3 doctests failed
sage -t --long src/sage/homology/examples.py  # 1 doctest failed
sage -t --long src/sage/homology/simplicial_set.py  # 1 doctest failed
sage -t --long src/doc/en/thematic_tutorials/group_theory.rst  # 3 doctests failed
sage -t --long src/sage/combinat/symmetric_group_algebra.py  # 1 doctest failed
sage -t --long src/sage/knots/link.py  # 1 doctest failed
sage -t --long src/sage/coding/codecan/autgroup_can_label.pyx  # 1 doctest failed
sage -t --long src/sage/groups/braid.py  # 1 doctest failed
sage -t --long src/sage/tests/books/judson-abstract-algebra/sylow-sage.py  # 1 doctest failed
sage -t --long src/sage/groups/finitely_presented.py  # 5 doctests failed
sage -t --long src/sage/groups/finitely_presented_named.py  # 3 doctests failed
sage -t --long src/sage/categories/modules_with_basis.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/root_system.py  # 2 doctests failed
sage -t --long src/sage/combinat/root_system/hecke_algebra_representation.py  # 1 doctest failed
sage -t --long src/doc/en/thematic_tutorials/lie/weyl_groups.rst  # 1 doctest failed
sage -t --long src/sage/categories/simplicial_sets.py  # 1 doctest failed
sage -t --long src/sage/groups/matrix_gps/coxeter_group.py  # 1 doctest failed
sage -t --long src/sage/libs/gap/util.pyx  # 7 doctests failed
sage -t --long src/sage/combinat/matrices/hadamard_matrix.py  # 1 doctest failed
sage -t --long src/doc/en/constructions/groups.rst  # 4 doctests failed
sage -t --long src/doc/ja/tutorial/tour_groups.rst  # 1 doctest failed
sage -t --long src/sage/groups/matrix_gps/linear.py  # 1 doctest failed
sage -t --long src/sage/groups/libgap_morphism.py  # 1 doctest failed
sage -t --long src/sage/groups/matrix_gps/group_element.pyx  # 1 doctest failed
sage -t --long src/sage/groups/abelian_gps/abelian_group_gap.py  # 1 doctest failed
sage -t --long src/sage/libs/gap/operations.py  # 1 doctest failed
sage -t --long src/sage/tests/gap_packages.py  # 4 doctests failed
sage -t --long src/sage/misc/randstate.pyx  # 8 doctests failed

Most of these failures fall into a few different categories:

  1. Bugs caused by broken syntax error handling, which still needs some fixing.
  2. Minor bugs remaining in our GAP interface; in particular places where errors (besides syntax errors) seem to not be handled properly. I think there are only a couple of these.
  3. Missing functionality that was removed from the branch but needs to be restored (particularly a couple test functions in sage.libs.gap.util).
  4. Tests that depend on some PRNG state that seem to be getting messed up by GAP somehow--I still need to investigate this.
  5. Tests that fail due to warnings from GAP that weren't present before.
  6. Tests that fail due to actually different mathematical results from GAP, which may still be valid, just different.
Last edited 12 months ago by embray (previous) (diff)

comment:309 Changed 12 months ago by embray

  • Description modified (diff)

Added a PR for my prototype GAP_Enter/GAP_Leave macros. Still working on updating my branch here to something usable; I'll have to finish that tomorrow though.

comment:310 Changed 12 months ago by git

  • Commit changed from 7ccc120e3a606e044bdd9ef6835930075fa70a31 to 18478b9e261e7f5a1c83de6c08916fdea9c9851a

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

14be074this appears to be an expected difference in the results GAP gives for NormalSubgroups(SymmetricGroup(4))
a2394d8should include sysinfo.gap; fix small bug in chmod call
fc959ecminor cleanup in sage.interfaces.gap
6433be8ensure readline is disabled
8a0696esome fixes to doc parsing
8228bacensure that the xgap package is not loaded
aa33f74whitespace; trivial
18478b9significant cleanup of gap_includes.pxd

comment:311 follow-up: Changed 12 months ago by jdemeyer

So what's the conclusion on the error/interrupt handling? That discussion seems to have ended abruptly.

comment:312 Changed 12 months ago by git

  • Commit changed from 18478b9e261e7f5a1c83de6c08916fdea9c9851a to b7f6221a8590b0cc821e5dd8bce7abd0bea854ad

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

98baa3csignificant cleanup of gap_includes.pxd
7239725this file is no longer needed since I'm patching GAP with this functionality instead
1dc74b2speed up this code up a little bit
47968d3use ViewObj instead of ViewString
9231e56use more GAP_Enter() and GAP_Leave() where it seems appropriate
11f52b7fix some tests whose output changed since
f176451fix needed due to changes in record keys
e7dc33dminor test fixes and cleanup
17371cbuse changesignal to restore SIGCHLD and SIGINT handling
b7f6221remove this debug function for now

comment:313 Changed 12 months ago by git

  • Commit changed from b7f6221a8590b0cc821e5dd8bce7abd0bea854ad to 09d229f1db78ff96d6b46df9ffad22d03155c2c8

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

20c6279significant cleanup of gap_includes.pxd
8b53d4fthis file is no longer needed since I'm patching GAP with this functionality instead
94f09a6speed up this code up a little bit
f571399use ViewObj instead of ViewString
b106ff9use more GAP_Enter() and GAP_Leave() where it seems appropriate
acc75fefix some tests whose output changed since
e588e7afix needed due to changes in record keys
e11e438minor test fixes and cleanup
b686acfuse changesignal to restore SIGCHLD and SIGINT handling
09d229fremove this debug function for now

comment:314 Changed 12 months ago by embray

Finished committing all my changes so far, and rebased on 8.5.beta6, so this should be buildable and usable. I included the absolutely necessary patches to GAP that are needed so far so that it will work; I will hope to get these, or some version of them, in GAP 4.10.1. In case anyone wants to look at the remaining test failures.

comment:315 in reply to: ↑ 311 Changed 12 months ago by embray

Replying to jdemeyer:

So what's the conclusion on the error/interrupt handling? That discussion seems to have ended abruptly.

There are multiple issues pertaining to error handling and interrupts, so I'm not sure at this point exactly to which you refer. But to give a brief summary of some of the issues that have been addressed:

  1. Right now I'm using changesignal from cysignals to unset (and restore originals, if any) the SIGINT and SIGCHLD handlers installed by GAP during its initialization. It would be nice to have another way to do this but the current method works well-enough. I would also like to patch GAP so that this isn't needed in the first place.
    • Related: Since GAP is now not using its own SIGINT handler we still wrap all calls to the GAP API with sig_on()/sig_off() as this will handle async interrupts, as well as any other unhandled exceptions from GAP. This is not completely ideal because it means it may be possible to interrupt GAP in critical sections possibly without giving GAP the opportunity to perform its normal interrupt handling. But AFAICT maybe this isn't such a problem--this was true of the old libgap as well so nothing has really changed here.
  1. The GAP_Enter() and GAP_Error_Setjmp() macro ensures that if a "read error" occurs in GAP--any error that results in a longjmp--that is not already explicitly handled within GAP (e.g. with a TRY_IF_NO_ERROR macro), there is a sane place to jump to back in our code. A Python exception is also set which can be checked by Cython code. So in other words as long as any GAP code is surrounded by something like:
    try:
        GAP_Error_Setjmp()  # Or GAP_Enter()
        <libgap calls go here>
    except RuntimeError:
        ...
    

The resulting RuntimeError will be handled. As mentioned in the first point, I still also recommend wrapping all this in sig_on()/sig_off() too, as with most any C library code.

This also requires a patch to GAP that I hope to get some form of in 4.10.1. I was also able to get this working (the error handling) without a patch, but the need for managing the "stack bottom" is great enough that we need that patch at the very least, and it might as well have the error handling combined into it.

GAP still could use significant refactoring to its error handling and I have some vague plans for that. Though the week before last I spent time investigating how much work that might take, and it's not insignificant, so I didn't want to go down a rabbit hole when there were practical solutions for dealing with what we have now.

comment:316 in reply to: ↑ 308 Changed 12 months ago by embray

Replying to embray:

  1. Tests that depend on some PRNG state that seem to be getting messed up by GAP somehow--I still need to investigate this.

For the tests that depend on pseudo-random results it seems GAP is actually not affecting results except for those that come from GAP itself. It is no surprise that a new version of GAP might mean slightly different results from its own PRNGs and that's probably something we have no control over. I just need to confirm that the different results are only due to changes internal to GAP (and are not affecting other PRNGs in Sage), and then update those tests.

comment:317 Changed 12 months ago by dimpase

I'll look at the tests today/tomorrow

comment:318 follow-up: Changed 12 months ago by jdemeyer

Thanks for the update. I'm a bit bothered by the fact that we now need two setjmp() calls for GAP: one for cysignals and one for GAP itself. Of course, we should be happy that it works, it just doesn't feel right.

comment:319 Changed 12 months ago by dimpase

sagelib now does not build for me, the stuff with GAP's T_INT, T_BOOL etc has not made it into the patches it seems. I probably can edit headers in local/include/gap/ to work around this.

...
[sagelib-8.5.beta6] [  3/373] gcc -fno-strict-aliasing -g -O2 -g -O0 -Wall -Wno-unused -fPIC -I./sage/groups/perm_gps/partn_ref2 -I./sage/cpython -I/mnt/opt/Sage/sage-dev/local/include -I/mnt/opt/Sage/sage-dev/local/include/python2.7 -I/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/numpy/core/include -I/mnt/opt/Sage/sage-dev/src -I/mnt/opt/Sage/sage-dev/src/sage/ext -Ibuild/cythonized -I/mnt/opt/Sage/sage-dev/local/include/python2.7 -c build/cythonized/sage/graphs/spanning_tree.c -o build/temp.linux-x86_64-2.7-pydebug/build/cythonized/sage/graphs/spanning_tree.o -fno-strict-aliasing -DCYTHON_CLINE_IN_TRACEBACK=1 -std=c99
[sagelib-8.5.beta6] [  4/373] gcc -fno-strict-aliasing -g -O2 -g -O0 -Wall -Wno-unused -fPIC -I./sage/cpython -I./sage/libs/ntl -I/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/cysignals -I/mnt/opt/Sage/sage-dev/local/include -I/mnt/opt/Sage/sage-dev/local/include/python2.7 -I/mnt/opt/Sage/sage-dev/local/lib/python2.7/site-packages/numpy/core/include -I/mnt/opt/Sage/sage-dev/src -I/mnt/opt/Sage/sage-dev/src/sage/ext -Ibuild/cythonized -I/mnt/opt/Sage/sage-dev/local/include/python2.7 -c build/cythonized/sage/graphs/connectivity.c -o build/temp.linux-x86_64-2.7-pydebug/build/cythonized/sage/graphs/connectivity.o -fno-strict-aliasing -DCYTHON_CLINE_IN_TRACEBACK=1 -std=c99
[sagelib-8.5.beta6] In file included from /mnt/opt/Sage/sage-dev/local/include/gap/system.h:27,
[sagelib-8.5.beta6]                  from build/cythonized/sage/graphs/spanning_tree.c:660:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/gap/config.h:159: warning: "HAVE_STAT" redefined
[sagelib-8.5.beta6]  #define HAVE_STAT 1
[sagelib-8.5.beta6]  
[sagelib-8.5.beta6] In file included from /mnt/opt/Sage/sage-dev/local/include/python2.7/Python.h:61,
[sagelib-8.5.beta6]                  from build/cythonized/sage/graphs/spanning_tree.c:48:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/python2.7/pyport.h:374: note: this is the location of the previous definition
[sagelib-8.5.beta6]  #define HAVE_STAT
[sagelib-8.5.beta6]  
[sagelib-8.5.beta6] In file included from build/cythonized/sage/graphs/spanning_tree.c:13041:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/python2.7/structmember.h:48:1: error: redeclaration of enumerator ‘T_INT’
[sagelib-8.5.beta6]  T_INT   =       1,
[sagelib-8.5.beta6]  ^~~~~
[sagelib-8.5.beta6] In file included from /mnt/opt/Sage/sage-dev/local/include/gap/ariths.h:17,
[sagelib-8.5.beta6]                  from build/cythonized/sage/graphs/spanning_tree.c:661:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/gap/objects.h:145:13: note: previous definition of ‘T_INT’ was here
[sagelib-8.5.beta6]              T_INT,      // immediate
[sagelib-8.5.beta6]              ^~~~~
[sagelib-8.5.beta6] In file included from build/cythonized/sage/graphs/spanning_tree.c:13041:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/python2.7/structmember.h:52:1: error: redeclaration of enumerator ‘T_STRING’
[sagelib-8.5.beta6]  T_STRING=       5,
[sagelib-8.5.beta6]  ^~~~~~~~
[sagelib-8.5.beta6] In file included from /mnt/opt/Sage/sage-dev/local/include/gap/ariths.h:17,
[sagelib-8.5.beta6]                  from build/cythonized/sage/graphs/spanning_tree.c:661:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/gap/objects.h:211:28: note: previous definition of ‘T_STRING’ was here
[sagelib-8.5.beta6]              NEXT_ENUM_EVEN(T_STRING),
[sagelib-8.5.beta6]                             ^~~~~~~~
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/gap/objects.h:84:5: note: in definition of macro ‘NEXT_ENUM_EVEN’
[sagelib-8.5.beta6]      id = _##id##_pre + (_##id##_pre & 1)
[sagelib-8.5.beta6]      ^~
[sagelib-8.5.beta6] In file included from build/cythonized/sage/graphs/spanning_tree.c:13041:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/python2.7/structmember.h:55:1: error: redeclaration of enumerator ‘T_CHAR’
[sagelib-8.5.beta6]  T_CHAR  =       7,       /* 1-character string */
[sagelib-8.5.beta6]  ^~~~~~
[sagelib-8.5.beta6] In file included from /mnt/opt/Sage/sage-dev/local/include/gap/ariths.h:17,
[sagelib-8.5.beta6]                  from build/cythonized/sage/graphs/spanning_tree.c:661:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/gap/objects.h:161:9: note: previous definition of ‘T_CHAR’ was here
[sagelib-8.5.beta6]          T_CHAR,
[sagelib-8.5.beta6]          ^~~~~~
[sagelib-8.5.beta6] In file included from build/cythonized/sage/graphs/spanning_tree.c:13041:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/python2.7/structmember.h:67:1: error: redeclaration of enumerator ‘T_BOOL’
[sagelib-8.5.beta6]  T_BOOL   =      14,
[sagelib-8.5.beta6]  ^~~~~~
[sagelib-8.5.beta6] In file included from /mnt/opt/Sage/sage-dev/local/include/gap/ariths.h:17,
[sagelib-8.5.beta6]                  from build/cythonized/sage/graphs/spanning_tree.c:661:
[sagelib-8.5.beta6] /mnt/opt/Sage/sage-dev/local/include/gap/objects.h:160:9: note: previous definition of ‘T_BOOL’ was here
[sagelib-8.5.beta6]          T_BOOL,
[sagelib-8.5.beta6]          ^~~~~~
...

comment:320 Changed 12 months ago by jdemeyer

I thought that this shouldn't happen. I'll look into it.

comment:321 Changed 12 months ago by jdemeyer

Dima, you seem to have a different version of Python. My structmember.h looks different from yours. Any idea what is going on?

comment:322 Changed 12 months ago by dimpase

oops, sorry, my fault. On that tree I played around with headers to understand the problem better, and left in a header where I replaced #defines with enums...

Now I reverted this, and it's back to normal, building, sorry for noise.

comment:323 Changed 12 months ago by embray

Another source of test failures I've found--possibly different from the PRNG differences--is that iteration order over a number of objects seems to be different; particularly iterating over some matrix groups. I'm not sure why, or if it's expected. But it seems mostly arbitrary--you get the same group elements just in a different order. For example, in the case of:

sage: SGA = SymmetricGroupAlgebra(QQ, WeylGroup(["A",3], prefix='s'))
sage: SGA.an_element()

I now get

s1*s2*s3 + 3*s3*s2 + 2*s3 + 1

where previously it returned

2*s1*s2*s3*s2*s1 + 3*s1*s2*s3*s1 + s1*s2*s3 + 1

This is of course meant to be somewhat arbitrary, though it should be deterministic (and it is, the result just changed due to some change in GAP when iterating over the underlying matrix group).

Last edited 12 months ago by embray (previous) (diff)

comment:324 in reply to: ↑ 318 Changed 12 months ago by embray

Replying to jdemeyer:

Thanks for the update. I'm a bit bothered by the fact that we now need two setjmp() calls for GAP: one for cysignals and one for GAP itself. Of course, we should be happy that it works, it just doesn't feel right.

I don't like it either. I think we could combine them into one, and I have some ideas for how to make that work via an enhancement to cysignals. In the meantime it does work though.

Changed 12 months ago by dimpase

patches for src/sage/groups/

comment:325 follow-up: Changed 12 months ago by dimpase

attached fixes all the doctests in src/sage/groups/ (please apply them!) except one, which is trickier, and stems from broken support for sending to GAP matrices over inexact fields:

sage: m=matrix(CC, [[1,2],[3,4]])
sage: libgap(m)
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
RuntimeError: Error, Variable: 'Complex' must have a value
Exception RuntimeError: "Error, Variable: 'Complex' must have a value" in 'sage.libs.gap.util.error_handler' ignored
Syntax error: ; expected in stream:1
Complex Field with 53 bits of precision;
 ^^^^^^^^^^^^^^^^^^^^
Error, Variable: 'bits' must have a value
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
...
Error, no 1st choice method found for `CloseStream' on 1 arguments
gap: panic, could not open *errout*!

Edit: it is just the broken error handling: the current Sage behaviour for this is

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: base ring is not supported by GAP
Last edited 12 months ago by dimpase (previous) (diff)

comment:326 Changed 12 months ago by dimpase

  • Description modified (diff)

I've opened #26856 to deal with update of gap_packages and database_gap.

comment:327 Changed 12 months ago by dimpase

more doctest changes are on u/dimpase/GQ, actually, just this commit (which includes the diffs in the attachment above too) https://git.sagemath.org/sage.git/commit/?h=1ed2d00b452155e803c79040460edcd55c120320

I think we are in a pretty good shape here - once the error handling in GAP is sorted out, what would remain are few things in libs/gap/util, which are strictly speaking not necessary for (lib)gap functionality.

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

comment:328 follow-up: Changed 12 months ago by git

  • Commit changed from 09d229f1db78ff96d6b46df9ffad22d03155c2c8 to f48ca24b7df9c6e28fa2c1672fadff09e1c19b1e

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

f48ca24fix some tests that depend on randomized results from GAP

comment:329 Changed 12 months ago by git

  • Commit changed from f48ca24b7df9c6e28fa2c1672fadff09e1c19b1e to 90838a1109d818c6dc5f4bcc36a4e5530cc90e53

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

90838a1GAP_ROOT_PATHS is obsolete and should be spelled GAPInfo.RootPaths

comment:330 in reply to: ↑ 328 Changed 12 months ago by dimpase

Replying to git:

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

f48ca24fix some tests that depend on randomized results from GAP

Erik, these are a part of the commit I've invited you to apply to your branch...

comment:331 Changed 12 months ago by git

  • Commit changed from 90838a1109d818c6dc5f4bcc36a4e5530cc90e53 to 39e42a4259c0d208d1623fc5bc3f949453d805cb

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

f1d8cf7improve handling of error results from GAP_EvalString, in particular syntax errors
39e42a4Improved error handling, especially in the case where multiple errors are

comment:332 in reply to: ↑ 325 ; follow-up: Changed 12 months ago by embray

Replying to dimpase:

attached fixes all the doctests in src/sage/groups/ (please apply them!) except one, which is trickier, and stems from broken support for sending to GAP matrices over inexact fields:

Thanks, I fixed that error just now, and am applying your patches now. One thing I noticed is that you are just adding some of these new warning messages from GAP to the expected output, like

#I  MakeReadWriteGlobal: CosetTableDefaultMaxLimit already read-write

Perhaps we should instead fix whatever is causing those messages, or make them silent somehow: It's confusing noise in the context of code that uses GAP somewhere deep down but not in any way that's obvious to users.

comment:333 in reply to: ↑ 332 ; follow-up: Changed 12 months ago by dimpase

Replying to embray:

Replying to dimpase:

attached fixes all the doctests in src/sage/groups/ (please apply them!) except one, which is trickier, and stems from broken support for sending to GAP matrices over inexact fields:

Thanks, I fixed that error just now, and am applying your patches now.

I hope you just cherry-pick the commit I posted, rather than using much less complete attachment to this ticket.

One thing I noticed is that you are just adding some of these new warning messages from GAP to the expected output, like

#I  MakeReadWriteGlobal: CosetTableDefaultMaxLimit already read-write

Perhaps we should instead fix whatever is causing those messages, or make them silent somehow: It's confusing noise in the context of code that uses GAP somewhere deep down but not in any way that's obvious to users.

these messages are default in new GAP. I prefer to stay close to what GAP does, unless really necessary. I agree that these messages are ugly, but we'd work with upstream on this.

comment:334 in reply to: ↑ 333 Changed 12 months ago by embray

Replying to dimpase:

Replying to embray:

Replying to dimpase:

attached fixes all the doctests in src/sage/groups/ (please apply them!) except one, which is trickier, and stems from broken support for sending to GAP matrices over inexact fields:

Thanks, I fixed that error just now, and am applying your patches now.

I hope you just cherry-pick the commit I posted, rather than using much less complete attachment to this ticket.

I did.

One thing I noticed is that you are just adding some of these new warning messages from GAP to the expected output, like

#I  MakeReadWriteGlobal: CosetTableDefaultMaxLimit already read-write

Perhaps we should instead fix whatever is causing those messages, or make them silent somehow: It's confusing noise in the context of code that uses GAP somewhere deep down but not in any way that's obvious to users.

these messages are default in new GAP. I prefer to stay close to what GAP does, unless really necessary. I agree that these messages are ugly, but we'd work with upstream on this.

GAP shouldn't be printing anything we didn't explicitly ask it to print when used as a library (unless, maybe, the user is directly calling libgap.eval(...)). So if nothing else there should be an option to disable these messages either before a libgap.eval(...) or as an optional argument to it. I don't know exactly where they come from though.

comment:335 Changed 12 months ago by embray

Here are some of the relevant docs on GAP Info messages: https://www.gap-system.org/Manuals/doc/ref/chap7.html#X864E4B6886E2697D

I think it would be useful for the python module to register its own custom info handler to convert these to Python warnings that can then be filtered with the standard Python warnings facilities.

comment:336 follow-up: Changed 12 months ago by dimpase

the messages about MakeReadWriteGlobal appear to indicate mini-bugs in GAP, I think. I'll dig this up and open a GAP issue about it.

comment:337 Changed 12 months ago by embray

Thanks. I agree that, in general, we do want to see these messages, as they could indicate real issues. But if nothing else it should be possible to squelch them in those cases where the issues are known, or have otherwise been addressed.

comment:338 in reply to: ↑ 336 ; follow-up: Changed 12 months ago by dimpase

Replying to dimpase:

the messages about MakeReadWriteGlobal appear to indicate mini-bugs in GAP, I think. I'll dig this up and open a GAP issue about it.

in fact, it's entirely Sage's issue: we have 2 calls like:

with libgap.global_context('CosetTableDefaultMaxLimit', limit):

in src/sage/groups/finitely_presented.py which trigger these, as they call GAP's MakeReadWriteGlobal on already read/write MakeReadWriteGlobal.

So this is something for us to fix.

comment:339 Changed 12 months ago by dimpase

However,

#I  Forcing finiteness test

messages seem to be really triggered by GAP alone.

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

There is (at least) one test that's proving vexing. In src/doc/en/constructions/groups.rst:

sage: print(gap.eval("G := SymmetricGroup( 4 )"))
Sym( [ 1 .. 4 ] )
sage: print(gap.eval("normal := NormalSubgroups( G );"))
[ Sym( [ 1 .. 4 ] ), Alt( [ 1 .. 4 ] ), Group([ (1,4)(2,3), (1,2)(3,4) ]),
  Group(()) ]

When I run those commands directly in GAP I seem to always get the above result. However, sometimes this test (just in Sage...?) returns:

[ Sym( [ 1 .. 4 ] ), Alt( [ 1 .. 4 ] ), Group([ (1,4)(2,3), (1,3)(2,4) ]),
  Group(()) ]

(note the only difference is the second element of the third subgroup). An equally valid result, but I don't know why this is't entirely deterministic or where the randomness is coming from.

comment:341 in reply to: ↑ 340 Changed 12 months ago by dimpase

Replying to embray:

There is (at least) one test that's proving vexing. In src/doc/en/constructions/groups.rst:

sage: print(gap.eval("G := SymmetricGroup( 4 )"))
Sym( [ 1 .. 4 ] )
sage: print(gap.eval("normal := NormalSubgroups( G );"))
[ Sym( [ 1 .. 4 ] ), Alt( [ 1 .. 4 ] ), Group([ (1,4)(2,3), (1,2)(3,4) ]),
  Group(()) ]

When I run those commands directly in GAP I seem to always get the above result. However, sometimes this test (just in Sage...?) returns:

[ Sym( [ 1 .. 4 ] ), Alt( [ 1 .. 4 ] ), Group([ (1,4)(2,3), (1,3)(2,4) ]),
  Group(()) ]

(note the only difference is the second element of the third subgroup). An equally valid result, but I don't know why this is't entirely deterministic or where the randomness is coming from.

GAP is certainly using (pseudo)randomization in many places, sometimes by default. I would not be worried here, and change the test to compute orders (24,12,4,1) of these subgroups instead.

comment:342 Changed 12 months ago by embray

Right, it's using some random processes in searching for subgroups or something, and indeed the orders of the groups are consistent either way. But I am still concerned here, because between test runs pseudo-random results should still be consistent, unless there is some RNG in GAP that we are now not seeding properly.

comment:343 follow-ups: Changed 12 months ago by embray

Aha, to my surprise randstate.set_seed_gap does not actually do anything for the libgap interface, just for the pexpect GAP interface. So that will indeed be a problem in general. I wonder how this was handled before...?

This also leads me to wonder if we're in a good place where we can start to deprecate/remove the pexpect interface entirely. I'm not sure what advantages it might still have, if any...

Last edited 12 months ago by embray (previous) (diff)

comment:344 in reply to: ↑ 343 ; follow-up: Changed 12 months ago by dimpase

Replying to embray:

Aha, to my surprise randstate.set_seed_gap does not actually do anything for the libgap interface, just for the pexpect GAP interface. So that will indeed be a problem in general. I wonder how this was handled before...?

I guess it was just broken, and tests didn't pick it up for some reason. (seems easy to fix though).

This also leads me to wonder if we're in a good place where we can start to deprecate/remove the pexpect interface entirely. I'm not sure what advantages it might still have, if any...

IIRC there are few places in Sage that still use gap in place of libgap, but these should be easy. I think after this ticket and associated GAP packages are done, it's high time to pull GAP pexpect interface.

comment:345 in reply to: ↑ 344 Changed 12 months ago by embray

Replying to dimpase:

Replying to embray:

Aha, to my surprise randstate.set_seed_gap does not actually do anything for the libgap interface, just for the pexpect GAP interface. So that will indeed be a problem in general. I wonder how this was handled before...?

I guess it was just broken, and tests didn't pick it up for some reason. (seems easy to fix though).

I guess it must have been broken: I can't see any way that the libgap interface was otherwise being re-seeded. I am working on a fix.

This also leads me to wonder if we're in a good place where we can start to deprecate/remove the pexpect interface entirely. I'm not sure what advantages it might still have, if any...

IIRC there are few places in Sage that still use gap in place of libgap, but these should be easy. I think after this ticket and associated GAP packages are done, it's high time to pull GAP pexpect interface.

Yes, I hope so. This should be done first though.

comment:346 in reply to: ↑ 343 Changed 12 months ago by embray

Replying to embray:

Aha, to my surprise randstate.set_seed_gap does not actually do anything for the libgap interface, just for the pexpect GAP interface. So that will indeed be a problem in general. I wonder how this was handled before...?

For some reason I led myself astray by thinking the test that was randomly failing had anything to do with libgap, but it was actually using the pexpect interface in the first place, and it had nothing to do with libgap.

I realized that many of the doctests manually call something like current_randstate().set_seed_gap() before calls that use GAP that might involve some random process. I think that's rather unfortunate, especially for documentation (it raises a distracting question "why do I have to do this first?") so I'm also adding a patch that does this automatically when the GAP interface is used in doctests.

Other than this random seed issue, we're very close to having 100% of the standard doctests passing.

Last edited 12 months ago by embray (previous) (diff)

comment:347 in reply to: ↑ 338 Changed 12 months ago by dimpase

Replying to dimpase:

in fact, it's entirely Sage's issue: we have 2 calls like:

with libgap.global_context('CosetTableDefaultMaxLimit', limit):

in src/sage/groups/finitely_presented.py which trigger these, as they call GAP's MakeReadWriteGlobal on already read/write MakeReadWriteGlobal.

So this is something for us to fix.

I've fixed this, it was easy. I've pushed commit 5d3fce on u/dimpase/GQ for you to pick. Basically, the nontrivial part was

  • src/sage/libs/gap/libgap.pyx

    a b class Gap(Parent): 
    485485            ...
    486486            ValueError: libGAP: Error, VAL_GVAR: No value bound to FooBar
    487487        """
     488        is_readonlyglobal = self.function_factory('IsReadOnlyGlobal')
    488489        make_readwrite = self.function_factory('MakeReadWriteGlobal')
    489490        unbind_global = self.function_factory('UnbindGlobal')
    490         make_readwrite(variable)
     491        if is_readonlyglobal(variable):
     492            make_readwrite(variable)
    491493        unbind_global(variable)
    492494
    493495    def get_global(self, variable):

comment:348 Changed 12 months ago by git

  • Commit changed from 39e42a4259c0d208d1623fc5bc3f949453d805cb to 5a2949d204bf6d7bf90e6254371b668f4522f019

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

36b305ffix doctests changed due to GAP formatting etc changes
52c002dfix a few more tests that changed due to minor output differences in GAP
34ed6e1remove some obsolete debug functions
5a2949dfixed noisy "#I MakeReadWriteGlobal" things

comment:349 Changed 12 months ago by embray

Excellent, thank you

comment:350 Changed 12 months ago by git

  • Commit changed from 5a2949d204bf6d7bf90e6254371b668f4522f019 to 0717586c0b2ea22c2e6a012ba23b3d9fd6f753cd

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

0717586Deprecate Gap.mem and remove its functionality

comment:351 Changed 12 months ago by embray

I removed some of the old, obsolete debugging code that wasn't used anywhere. I also deprecated the Gap.mem() method since there's no way to implement this without digging into GASMAN internals, and even then it's very low-level data not likely of interest to any users--I can't find any reference to it being used anywhere online. To make up for it I improved the (poorly named) Gap.show() a little bit. Now, rather than printing, it returns a dict, and contains a little more information.

comment:352 Changed 12 months ago by git

  • Commit changed from 0717586c0b2ea22c2e6a012ba23b3d9fd6f753cd to 15b7c7855d9097143e2b582b4831d2fb50294f53

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

15b7c78Allow Gap.__getattr__ to return GAP objects other than just functions

comment:353 Changed 12 months ago by embray

  • Dependencies set to #26874

I think it would be good to resolve #26874 for this ticket, since there are new ways, it seems, that results from GAP can be slightly random. I think my solution in #26874 is fine, but any other solution that works is fine too.

comment:354 Changed 12 months ago by embray

  • Description modified (diff)

Got around to opening a PR for the writeandcheck issue: https://github.com/gap-system/gap/pull/3102

As I wrote there, I'm not entirely satisfied with the solution, but it's "good enough for now" for our purposes.

comment:355 Changed 12 months ago by embray

One of the most severe remaining issues is the problem with all_documented_functions. Specifically this one snippet of code, which returns the list of documented global functions (here in plain GAP code):

documented_funcs := Filtered(NamesGVars(), IsDocumentedWord);

I profiled this by setting ProfileGlobalFunctions(true); and then DisplayProfile(); afterwards. On GAP 4.8.6 in Sage's devel branch I get:

  count  self/ms  chld/ms  stor/kb  chld/kb  package  function
  43509       23        0     2514        0  GAP      LowercaseString
  59996       29        3     1874        0  GAPDoc   IntListUnicodeString
  14941       37        0     2228        0  GAPDoc   StripEscapeSequences
  21356       85        0    12680        0  GAP      Union
 194454      106        1    10578        0  GAP      Concatenation
  10678       32       87     8718    13516  GAP      SIMPLE_STRING
  89499      103       25     9089     2514  GAP      HELP_BOOK_INFO
 113152      118      119     8575     7391  GAP      List
132426*    33312       89  9047115        0  GAP      MATCH_BEGIN
  43509    28045    33593    29768  9070369  GAP      HELP_GET_MATCHES
  10679      152    61879    11525  9133134  GAP      Filtered
              18              1865                    OTHER
           62060           9146534                    TOTAL

whereas in the GAP on this branch I get:

  count  self/ms  chld/ms  stor/kb  chld/kb  package  function
  63144       79        3        0        0  GAPDoc   IntListUnicodeString
  13737      178        0     1158        0  GAPDoc   StripEscapeSequences
 132388      257        7     1551        0  GAP      Minimum
  10876      315      458     8470     5143  GAP      SIMPLE_STRING
 129752     1044        0    21506        0  GAP      Cartesian
  21752     1741        1    34699        0  GAP      Union
1096610     2733        0    35812        0  GAP      LowercaseString
6238192    15599       21   111496        0  GAP      Concatenation
3048782    13451     2776     1104    35812  GAP      HELP_BOOK_INFO
2349706     9325    14996    48333    43036  GAP      List
664468*  1493442        0  100189*        0  GAP      MATCH_BEGIN_COUNT
1096610  1090807  1530461  1097484  102079*  GAP      HELP_GET_MATCHES
  10877     3653  2628892    21606  113805*  GAP      Filtered
         2632624           114021*                    TOTAL

which is a several order of magnitude regression. It should be noted that Size(NamesGVars()) is about the same in both cases; ~10000.

comment:356 follow-up: Changed 12 months ago by dimpase

I have a problem building this branch, with Sage 8.5.rc0, on Debian: (even tried make distclean first, to no avail). Any idea what's wrong?:

[sagelib-8.5.rc0] [  1/304] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -fPIC -I./sage/cpython -Isage/cpython -I/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/cysignals -I/home/dimpase/Sage/sagetrac-mirror/local/include -I/home/dimpase/Sage/sagetrac-mirror/local/include/python2.7 -I/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/numpy/core/include -I/home/dimpase/Sage/sagetrac-mirror/src -I/home/dimpase/Sage/sagetrac-mirror/src/sage/ext -Ibuild/cythonized -I/home/dimpase/Sage/sagetrac-mirror/local/include/python2.7 -c build/cythonized/sage/libs/gap/element.c -o build/temp.linux-x86_64-2.7/build/cythonized/sage/libs/gap/element.o -fno-strict-aliasing -DCYTHON_CLINE_IN_TRACEBACK=1 -std=c99
[sagelib-8.5.rc0] In file included from /home/dimpase/Sage/sagetrac-mirror/local/include/gap/system.h:27:0,
[sagelib-8.5.rc0]                  from build/cythonized/sage/libs/gap/element.c:660:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/config.h:159:0: warning: "HAVE_STAT" redefined
[sagelib-8.5.rc0]  #define HAVE_STAT 1
[sagelib-8.5.rc0]  
[sagelib-8.5.rc0] In file included from /home/dimpase/Sage/sagetrac-mirror/local/include/python2.7/Python.h:61:0,
[sagelib-8.5.rc0]                  from build/cythonized/sage/libs/gap/element.c:53:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/python2.7/pyport.h:374:0: note: this is the location of the previous definition
[sagelib-8.5.rc0]  #define HAVE_STAT
[sagelib-8.5.rc0]  
[sagelib-8.5.rc0] In file included from /home/dimpase/Sage/sagetrac-mirror/local/include/gap/system.h:32:0,
[sagelib-8.5.rc0]                  from build/cythonized/sage/libs/gap/element.c:660:
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_crepr’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:37: warning: implicit declaration of function ‘STATE’ [-Wimplicit-function-declaration]
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                      ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4137:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 100, __pyx_L4_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4137:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 100, __pyx_L4_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: note: each undeclared identifier is reported only once for each function it appears in
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4137:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 100, __pyx_L4_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_make_gap_record’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4443:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 142, __pyx_L6_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_make_gap_integer’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4700:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 172, __pyx_L4_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_make_gap_string’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4831:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 198, __pyx_L4_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_make_any_gap_element’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:4991:17: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]      __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 249, __pyx_L4_error)
[sagelib-8.5.rc0]                  ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__compare_equal’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:8510:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 821, __pyx_L7_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__compare_less’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:8810:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 845, __pyx_L7_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__add_’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:9107:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 875, __pyx_L6_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__sub_’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:9480:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 907, __pyx_L6_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__mul_’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:9867:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 939, __pyx_L6_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__div_’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:10253:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 976, __pyx_L6_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_f_4sage_4libs_3gap_7element_10GapElement__mod_’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:10625:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_5 = GAP_Enter(); if (unlikely(__pyx_t_5 == ((int)0))) __PYX_ERR(0, 1005, __pyx_L6_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_pf_4sage_4libs_3gap_7element_10GapElement_40__pow__’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:11048:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_6 = GAP_Enter(); if (unlikely(__pyx_t_6 == ((int)0))) __PYX_ERR(0, 1044, __pyx_L7_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_pf_4sage_4libs_3gap_7element_19GapElement_Function_2__call__’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:17678:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_9 = GAP_Enter(); if (unlikely(__pyx_t_9 == ((int)0))) __PYX_ERR(0, 2310, __pyx_L9_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c: In function ‘__pyx_pf_4sage_4libs_3gap_7element_17GapElement_Record_6__getitem__’:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:43: error: ‘ReadJmpError’ undeclared (first use in this function)
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                                            ^
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:83:28: note: in expansion of macro ‘sySetjmp’
[sagelib-8.5.rc0]          _GAP_Error_Postjmp(sySetjmp(STATE(ReadJmpError))))
[sagelib-8.5.rc0]                             ^~~~~~~~
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/libgap-api.h:86:21: note: in expansion of macro ‘GAP_Error_Setjmp’
[sagelib-8.5.rc0]  #define GAP_Enter() GAP_Error_Setjmp(); GAP_EnterStack()
[sagelib-8.5.rc0]                      ^~~~~~~~~~~~~~~~
[sagelib-8.5.rc0] build/cythonized/sage/libs/gap/element.c:21586:21: note: in expansion of macro ‘GAP_Enter’
[sagelib-8.5.rc0]          __pyx_t_1 = GAP_Enter(); if (unlikely(__pyx_t_1 == ((int)0))) __PYX_ERR(0, 2994, __pyx_L6_error)
[sagelib-8.5.rc0]                      ^~~~~~~~~
[sagelib-8.5.rc0] [  2/304] gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -fPIC -I./sage/cpython -I/home/dimpase/Sage/sagetrac-mirror/local/include -I/home/dimpase/Sage/sagetrac-mirror/local/include/python2.7 -I/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/numpy/core/include -I/home/dimpase/Sage/sagetrac-mirror/src -I/home/dimpase/Sage/sagetrac-mirror/src/sage/ext -Ibuild/cythonized -I/home/dimpase/Sage/sagetrac-mirror/local/include/python2.7 -c build/cythonized/sage/libs/gap/libgap.c -o build/temp.linux-x86_64-2.7/build/cythonized/sage/libs/gap/libgap.o -fno-strict-aliasing -DCYTHON_CLINE_IN_TRACEBACK=1 -std=c99
[sagelib-8.5.rc0] error: command 'gcc' failed with exit status 1
[sagelib-8.5.rc0] In file included from /home/dimpase/Sage/sagetrac-mirror/local/include/gap/system.h:27:0,
[sagelib-8.5.rc0]                  from build/cythonized/sage/libs/gap/libgap.c:653:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/gap/config.h:159:0: warning: "HAVE_STAT" redefined
[sagelib-8.5.rc0]  #define HAVE_STAT 1
[sagelib-8.5.rc0]  
[sagelib-8.5.rc0] In file included from /home/dimpase/Sage/sagetrac-mirror/local/include/python2.7/Python.h:61:0,
[sagelib-8.5.rc0]                  from build/cythonized/sage/libs/gap/libgap.c:46:
[sagelib-8.5.rc0] /home/dimpase/Sage/sagetrac-mirror/local/include/python2.7/pyport.h:374:0: note: this is the location of the previous definition
[sagelib-8.5.rc0]  #define HAVE_STAT
[sagelib-8.5.rc0]  
[sagelib-8.5.rc0] Makefile:33: recipe for target 'sage' failed
[sagelib-8.5.rc0] make[4]: *** [sage] Error 1
[sagelib-8.5.rc0] make[4]: Leaving directory '/home/dimpase/Sage/sagetrac-mirror/src'
[sagelib-8.5.rc0] 
[sagelib-8.5.rc0] real	0m5.070s
[sagelib-8.5.rc0] user	0m3.966s
[sagelib-8.5.rc0] sys	0m1.078s
Makefile:1945: recipe for target 'sagelib' failed
make[3]: *** [sagelib] Error 2

comment:357 Changed 12 months ago by embray

I see the problem: The regression isn't in IsDocumentedWord, but rather the number of docs being searched. Right now I'm installing all the officially supported GAP packages, which obviously we should not do for base gap SPGK in Sage. This needs to be fixed.

However, this code should also work on other distributions where any arbitrary number of GAP packages might be installed. Therefore, it would be good to figure out a way to prevent this code from searching docs for packages, and just limit this to global functions built into GAP by default.

comment:358 in reply to: ↑ 356 Changed 12 months ago by embray

Replying to dimpase:

I have a problem building this branch, with Sage 8.5.rc0, on Debian: (even tried make distclean first, to no avail). Any idea what's wrong?:

Yikes, sorry about that. I fixed that locally but I didn't push the fix upstream yet. It's just a missing include in my patch to libgap-api.h.

comment:359 Changed 12 months ago by git

  • Commit changed from 15b7c7855d9097143e2b582b4831d2fb50294f53 to 16be3fc65e8fbac3efae68eebc98abe1aa503790

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

6d04a8afix a typo in the new version of Gap.show
16be3fcupdated GAP_Enter patch that does not require gapstate.h in libgap-api.h

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

Should work now. Make sure after ./sage -f gap that you also at the very least make sure that any modules in Sage that depend on libgap are rebuilt. I ensure that this happens by running touch src/sage/libs/gap/gap_includes.pxd followed by ./sage -b.

Last edited 12 months ago by embray (previous) (diff)

comment:361 Changed 12 months ago by dimpase

I get a segfault in libs/gap/util.so during docbuilding, and also errors like

sage -t src/sage/libs/gap/test.py
**********************************************************************
File "src/sage/libs/gap/test.py", line 18, in sage.libs.gap.test.test_write_to_file
Failed example:
    test_write_to_file()
Exception raised:
    Traceback (most recent call last):
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.libs.gap.test.test_write_to_file[1]>", line 1, in <module>
        test_write_to_file()
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/libs/gap/test.py", line 22, in test_write_to_file
        libgap.PrintTo(fname, message)
      File "sage/misc/lazy_import.pyx", line 322, in sage.misc.lazy_import.LazyImport.__getattr__ (build/cythonized/sage/misc/lazy_import.c:3536)
        return getattr(self.get_object(), attr)
      File "sage/libs/gap/libgap.pyx", line 663, in sage.libs.gap.libgap.Gap.__getattr__ (build/cythonized/sage/libs/gap/libgap.c:6079)
        from sage.libs.gap.gap_globals import common_gap_globals
    ImportError: No module named gap_globals

comment:362 Changed 12 months ago by embray

I am also getting a segfault during doc build. I don't know what that's about--it seems to come straight from GAP_Initialize. Obviously I'll look into it...

The other problem is because I forgot to add a new file to a commit.

comment:363 Changed 12 months ago by git

  • Commit changed from 16be3fc65e8fbac3efae68eebc98abe1aa503790 to 0cf204795df0a2406078d6ac47a5cc8c7173277d

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

2820d4dDeprecate Gap.mem and remove its functionality
60d7eb6Allow Gap.__getattr__ to return GAP objects other than just functions
0cf2047updated GAP_Enter patch that does not require gapstate.h in libgap-api.h

comment:364 Changed 12 months ago by embray

Did a forced push with some updated commits that fixed trivial mistakes (like the missing file). Going to look into the segfault during docbuild now. That's pretty disconcerting.

comment:365 Changed 12 months ago by dimpase

multithreading seems to be involved. Something with thread-local things, maybe...

...
[dochtml] /home/dimpase/Sage/sagetrac-mirror/local/lib/libgap.so.0(GAP_Initialize+0x1a)[0x7bdfa5a06fba]
[dochtml] /home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/libs/gap/util.so(+0xf72a)[0x7bdfa54dd72a]
...
[dochtml] /home/dimpase/Sage/sagetrac-mirror/local/lib/libpython2.7.so.1.0(+0x14e772)[0x7be030f22772]
[dochtml] /lib/x86_64-linux-gnu/libpthread.so.0(+0x7494)[0x7be030bbe494]
[dochtml] /lib/x86_64-linux-gnu/libc.so.6(clone+0x3f)[0x7be0301f5acf]
[dochtml] ------------------------------------------------------------------------
[dochtml] Attaching gdb to process id 14080.
...

comment:366 Changed 12 months ago by embray

Could be, but I don't suspect so at the moment, as I'm able to reproduce this without starting any threads. But I'm still not sure exactly what the problem is, as this is proving to be a bit of a heisenbug.

comment:367 in reply to: ↑ 360 Changed 12 months ago by jdemeyer

Replying to embray:

I ensure that this happens by running touch src/sage/libs/gap/gap_includes.pxd

Isn't Cython picking up that dependency automatically? It should...

comment:368 Changed 12 months ago by git

  • Commit changed from 0cf204795df0a2406078d6ac47a5cc8c7173277d to 132a7c303a8639c51eb92061ac7cb39106f5d316

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

eefc0fcfix warning from cython
9292d47add sig_on/off() around GAP_Initialize, just in case unhandled segfaults or the like occur
132a7c3copy the current environment before passing it to GAP

comment:369 Changed 12 months ago by embray

This should fix it. What a sneaky problem though. I think I will complain about this upstream...

A better solution, I think, would be to allow passing NULL as the environment argument to GAP_Initialize, in which case GAP can just use the correct environ.

comment:370 follow-up: Changed 12 months ago by dimpase

Yep, this seems to work, great! But why? Why does one have to pass a copy?

comment:371 Changed 12 months ago by arojas

I tested this branch on Arch with system-wide GAP and it works almost flawlessly, great job. I only get one test failure, which doesn't look too serious:

File "/usr/lib/python2.7/site-packages/sage/libs/gap/operations.py", line 96, in sage.libs.gap.operations.OperationInspector.operations
Failed example:
    Unknown in x.operations()
Expected:
    True
Got:
    False

One further question: since packages would be installed to GAP_DIR/pkg and some of them are compiled binaries, wouldn't it make more sense to put GAP_DIR under SAGE_LOCAL/lib instead of SAGE_LOCAL/share?

comment:372 Changed 12 months ago by embray

Yeah, I know about that failure--I haven't looked into it yet but like you said it doesn't look serious. Thank you for testing otherwise.

I'm a bit torn about the best place to put the GAP stuff--as you said, under <prefix>/share might not be the best after all. Hypothetically there could be multiple gap-dirs split up in a way that makes sense, but that's also more complicated. I wonder how Debian installs GAP...

comment:373 Changed 12 months ago by git

  • Commit changed from 132a7c303a8639c51eb92061ac7cb39106f5d316 to 38e687b90bf9a411ac524273c3713f5db199824f

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

38e687binstall only those packages that are required by GAP to run

comment:374 Changed 12 months ago by embray

Yeah, it seems Debian uses multiple GAP_ROOT_DIRs. Most of the GAP stdlib and other GAP packages go into /usr/share/gap, but there's also a /usr/lib/x86_64-linux-gnu/gap/ where it puts binary modules like for the IO package.

I think for now I don't want to worry about it, since the core gap SPKG does not install any binary packages. But it's still worth considering, perhaps as part of #26856.


New commits:

38e687binstall only those packages that are required by GAP to run
Last edited 12 months ago by embray (previous) (diff)

comment:375 in reply to: ↑ 370 Changed 12 months ago by embray

Replying to dimpase:

Yep, this seems to work, great! But why? Why does one have to pass a copy?

I think it's kind of a bug in GAP--something a bit ill-considered and complicated.

GAP's main() function is in the somewhat archaic (in that it is not standard, though many unices support it) three argument form:

int main ( int argc, char * argv[], char * environ[] )

This is used to pass the program's environment variables to the program. But POSIX-conforming systems don't need this because they have the char** environ global variable. GAP then passes the environment passed via main(...) to GAP_Initialize which stores a copy of it off to GAP's own global variable static char **sysenviron (this all happens in src/gap.c).

It then accesses the environment through the sysenviron variable. Actually the only place this is specifically used is FuncKERNEL_INFO in the same file which is called during startup (from lib/system.g) to set up some basic system info, including GAP-level copy of the environment.

This is problematic because functions that modify the environment such as putenv() or setenv() may require the array of environment variable pointers to be resized (obviously, but I double-checked and while this behavior is implementation-defined it's straightforward: in libc it just effectively does environ = realloc(new_size)). So the value of the global environ variable can be changed by the program if you're not careful. If this happens between the start of the process, and when GAP loops over its sysenviron variable you're in trouble, because that's now pointing to some old location of the original environ which now points to arbitrary/uninitialized data. To avoid this the program should always use environ directly, rather than saving off some possibly outdated pointer.

It so happened, in this case, that it almost worked--the old array that sysenviron was pointing to still contained the old environment variables, but its last (NULL) entry was already allocated to some other use and would contain some arbitrary value (in my testing it was typically either 0x300 or 0x310--who knows) hence the segfault.

I believe this is not a problem for GAP normally, as the pointer that's passed to main() is already to a copy and not to the actual environ array. But in Sage we were passing the global environ to GAP and that's where the problems arise, so we have to make a copy :|

This was a heisenbug, because I had an example case that reliably reproduced the problem when run by itself, but that worked when run under gdb. The reason for this was tricky: There's actually only one place during GAP's initialization that modifies the environment (and even then it only happens if HAVE_LIBREADLINE is defined): In the rl_initialize() call that initializes readline. It sets the COLUMNS and LINES variables if they are not already defined.

The reason the bug didn't occur under gdb, then, is because gdb itself uses readline, so when running the program under gdb those variables are already set, and the environment does not get modified, hence no crash when FuncKERNEL_INFO tries to read it :(

For all that reason, I think GAP_Initialize should accept NULL for its environ argument, and if so use the libc environ global, and it should do this by default on those systems that support it (most) and not save a (possibly outdated) copy of it.

Last edited 12 months ago by embray (previous) (diff)

comment:376 follow-up: Changed 12 months ago by dimpase

Does

sage: DihedralGroup(8).cardinality()

work on your branch? It hangs on mine at #26856 after I removed build/pkgs/gap/patches/nodefaultpackages.patch as well

comment:377 Changed 12 months ago by embray

Works fine for me.

comment:378 in reply to: ↑ 376 ; follow-up: Changed 12 months ago by dimpase

Replying to dimpase:

Does

sage: DihedralGroup(8).cardinality()

work on your branch? It hangs on mine at #26856 after I removed build/pkgs/gap/patches/nodefaultpackages.patch as well

putting this patch back makes me able to compute DihedralGroup(8).cardinality(). Hmm, this patch prevents autoloading packages. The hang is due to some sort of locking issue in pexpect. I don't like this, especially as it stalls #26856...

comment:379 Changed 12 months ago by dimpase

  • Work issues changed from fix libgap workspace loading, etc, and much more work... to ironing out the last kinks

comment:380 follow-up: Changed 12 months ago by arojas

Can the message "The gap-4.5.5.spkg (or later) seems to be not installed!" be dropped? It is not clear to me what the purpose of this message is: it is not declared an error, so sage will simply print the message and continue. If GAP_ROOT_DIR doesn't exist but GAP_ROOT is correctly declared in gap.sh, then everything will work fine but Sage will keep printing this message. IMO this should be replaced by a proper error if gap_root() doesn't return a valid path.

comment:381 Changed 12 months ago by git

  • Commit changed from 38e687b90bf9a411ac524273c3713f5db199824f to 6a5d34a92ddb510cfb5944f8dd05c0947eb9d23c

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

8b0d0adImplement GAPElement_List.__bool__ to work like a Python list
6a5d34afix the test for available operations on an object

comment:382 in reply to: ↑ 380 Changed 12 months ago by embray

Replying to arojas:

Can the message "The gap-4.5.5.spkg (or later) seems to be not installed!" be dropped? It is not clear to me what the purpose of this message is: it is not declared an error, so sage will simply print the message and continue. If GAP_ROOT_DIR doesn't exist but GAP_ROOT is correctly declared in gap.sh, then everything will work fine but Sage will keep printing this message. IMO this should be replaced by a proper error if gap_root() doesn't return a valid path.

I hadn't noticed that before. Indeed, it is just code rot and should be removed.

comment:383 Changed 12 months ago by git

  • Commit changed from 6a5d34a92ddb510cfb5944f8dd05c0947eb9d23c to 72df280f83fbcbc85ab6df809b2c57fc2d652068

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

72df280clean up gap_root() a bit more; remove obsolete warning message

comment:384 Changed 12 months ago by dimpase

What is the current state of GAP kernel packages such as io? Do they work? (we can postpone them to a later ticket though)

comment:385 in reply to: ↑ 378 ; follow-up: Changed 12 months ago by embray

Replying to dimpase:

Replying to dimpase:

Does

sage: DihedralGroup(8).cardinality()

work on your branch? It hangs on mine at #26856 after I removed build/pkgs/gap/patches/nodefaultpackages.patch as well

putting this patch back makes me able to compute DihedralGroup(8).cardinality(). Hmm, this patch prevents autoloading packages. The hang is due to some sort of locking issue in pexpect. I don't like this, especially as it stalls #26856...

FWIW the current version of my branch does still have this patch so I don't know what you mean by "putting this patch back". It's unfortunate that that code path uses the pexpect interface at all. We still intend to remove that. I still can't reproduce the problem though. Does it only happen if you have additional packages installed? It might be just that the method lookup is too slow.

I am going to continue looking into kernel packages next; I believe the only issue with that is minor but if not then I will postpone it.

comment:386 in reply to: ↑ 385 Changed 12 months ago by dimpase

Replying to embray:

Replying to dimpase:

Replying to dimpase:

Does

sage: DihedralGroup(8).cardinality()

work on your branch? It hangs on mine at #26856 after I removed build/pkgs/gap/patches/nodefaultpackages.patch as well

putting this patch back makes me able to compute DihedralGroup(8).cardinality(). Hmm, this patch prevents autoloading packages. The hang is due to some sort of locking issue in pexpect. I don't like this, especially as it stalls #26856...

FWIW the current version of my branch does still have this patch so I don't know what you mean by "putting this patch back".

I mean I, locally, removed the patch, and it broke Sage, by getting hanging DihedralGroup(8).cardinality().

It's unfortunate that that code path uses the pexpect interface at all.

well, yes, but getting rid of GAP's pexpect is quite a big task, it certainly cannot be done before this ticket is merged.

We still intend to remove that. I still can't reproduce the problem though. Does it only happen if you have additional packages installed? It might be just that the method lookup is too slow.

Most additional packages are installed just by untarring the canonical GAP 4.10 tarball. (Some of them also need some compiling done, but these are in minority).

I don't think it is related, it might be that pexpect GAP needs different options for startup, so that package autoloading is not conflicting with pexpect. This would be an easy fix, otherwise I suppose we will need to keep the patch in question.

I am going to continue looking into kernel packages next; I believe the only issue with that is minor but if not then I will postpone it.

comment:387 Changed 12 months ago by dimpase

OK, with 38e687b90bf (I tested on an older branch) in I can remove build/pkgs/gap/patches/nodefaultpackages.patch and have DihedralGroup(8).cardinality() running fine.

Of course, now I need to adjust the GAP packages installed, to have at least everything mentioned in build/pkgs/gap/patches/nodefaultpackages.patch, namely

 default:= [ "autpgrp", "alnuth", "crisp", "ctbllib", "factint", "fga", 
             "irredsol", "laguna", "polenta", "polycyclic", "resclasses",              
             "sophus", "tomlib" ]

and try again.

comment:388 follow-up: Changed 12 months ago by dimpase

OK, that DihedralGroup(8).cardinality() has been a red herring, it seems, sorry; probably a stale GAP workspace, or something like this. At the very least, I can add more GAP packages, and still everything works. On #26856 I added b63db23, removing nodefaultpackages.patch, and adding the packages needed to make GAP happy after this.

Let me run tests and see if everything is still OK.

comment:389 Changed 12 months ago by dimpase

few doctests need adjustments, as these GAP packages provide more methods, resulting in different output orders for some things, but otherwise all is fine. I've posted one more commit to #26856 to also install package altasrep, a missing (new) dependence to tomlib. More to come.

comment:390 Changed 12 months ago by dimpase

I've tested the current branch (with the one of #26856, at commit 1b71099 to be precise) on MacOSX 10.13, everything works, with OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES. Without the latter, few segfaults, but this is to be expected.

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

comment:391 in reply to: ↑ 388 Changed 12 months ago by embray

Replying to dimpase:

OK, that DihedralGroup(8).cardinality() has been a red herring, it seems, sorry; probably a stale GAP workspace, or something like this. At the very least, I can add more GAP packages, and still everything works. On #26856 I added b63db23, removing nodefaultpackages.patch, and adding the packages needed to make GAP happy after this.

The xgap package is known to case the pexpect interface to hang. This was discussed somewhere around comment:225. A few of the default packages (at least sonata and guava) will load the xgap package if the -p flag is set. I fixed this (see comment:230) by modifying sage.g to prevent loading of the xgap package, but if you have an old workspace from before this patch it will still be loaded.

This won't affect most users of Sage since most users will not have existing workspaces with these packages loaded (if they did, they would be getting hit with this same bug).

comment:392 Changed 12 months ago by dimpase

Anyhow, should we try to wrap up what we have?

I can use the old way to build/install GAP packages from gap_packages spkg that need it, and deal with "right way" to do this, as well as with more packages, and GAP kernel packages, on a follow-up ticket.

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

Hmm, when I remove the nodefaultpackages.patch, and if I don't have the additional packages installed, then when GAP starts up I get annoying info messages like:

    #I  autpgrp package is not available. Check that the name is correct
    #I  and it is present in one of the GAP root directories (see '??RootPaths')

either we install these packages by default (not ideal), I can find some way to override the default packages when loading GAP with sage.g (probably best, if it can be done), or we figure out how best to just silence those warnings.

comment:394 Changed 12 months ago by embray

Ah, apparently there's already an -A option for exactly this. I think, at least for calling GAP from Sage, we should do this.

Somewhat unfortunate in terms of principle of least astonishment ("some thing that was available in my GAP is not automatically available when I use GAP from Sage"). But in fairness this was already the case for most users of Sage. Perhaps at a minimum it should be documented in the docs for the GAP interfaces that GAP in Sage is started with a minimal set of packages autoloaded, and packages that one needs should be explicitly loaded.

comment:395 in reply to: ↑ 393 Changed 12 months ago by dimpase

Replying to embray:

Hmm, when I remove the nodefaultpackages.patch, and if I don't have the additional packages installed, then when GAP starts up I get annoying info messages like:

    #I  autpgrp package is not available. Check that the name is correct
    #I  and it is present in one of the GAP root directories (see '??RootPaths')

either we install these packages by default (not ideal),

Why? This is what I have done on #26856, having the same default packages autoloaded as GAP does. I think this is the best, and you seem to have agreed...

I can find some way to override the default packages when loading GAP with sage.g (probably best, if it can be done), or we figure out how best to just silence those warnings.

Why should we diverge from the canonical GAP here? Default autoloaded packages load more efficient routines for a number of tasks, too.

comment:396 Changed 12 months ago by dimpase

Not loading the default packages also lead to different formats of quite a few output results, or worse. I think we have had enough disagreement with GAP people on this, and I really do not want to keep annoying them.

comment:397 Changed 12 months ago by git

  • Commit changed from 72df280f83fbcbc85ab6df809b2c57fc2d652068 to 5e61d7b6a0da3aa53d8176fa1fb9353cc559b098

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

5e61d7bpass -A option to GAP to disable autoloading of default packages for both GAP interfaces

comment:398 Changed 12 months ago by gh-timokau

This discussion may be relevant: https://github.com/NixOS/nixpkgs/pull/38754

comment:399 Changed 12 months ago by embray

Considering that there's an option specifically to do this I have no qualms with doing so, at least for the default usage in Sage where not all users are going to care about what is being done with GAP anyways.

If there are specific routines used by Sage that are improved by specific pages I think we should explicitly load those packages anyways.

Perhaps, if requested, there could also be an option in Sage to change this off or on. Right now I'm trying to keep things as simple as possible. This doesn't change any test results because we were previously patching this out anyways. If there are some doctests in Sage whose results are impacted by what GAP packages are loaded, those specific tests should be rewritten to account for that, since otherwise they're sensitive to non-local environmental effects which is not desirable.


New commits:

5e61d7bpass -A option to GAP to disable autoloading of default packages for both GAP interfaces

comment:400 Changed 12 months ago by embray

In other words, I'd be fine with revisiting this later, but for the purposes of wrapping up this ticket I think that's the path of least resistance (and explicitly supported, which makes it that much more attractive an option).

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

OTOH, now when I run ./sage -gap I also get those annoying warnings unless I explicitly pass -A, so this is not good. Let me do a bit of analysis into what the impact is of adding those default packages for startup time and disk space... We would also want them, then, to be included in the core GAP spkg.

comment:402 follow-ups: Changed 12 months ago by embray

BTW building, installing, and loading the IO package works fine now, so I guess I fixed that somewhere along the line.

comment:403 Changed 12 months ago by embray

Ugh. Just that minimal set of packages adds 374MB. tomlib alone is 229MB. Not really sure what to do about that. In this day and age it's not a huge imposition but it does certainly increase the download size. Let me see how well it compresses.

Last edited 12 months ago by embray (previous) (diff)

comment:404 in reply to: ↑ 401 Changed 12 months ago by dimpase

Replying to embray:

OTOH, now when I run ./sage -gap I also get those annoying warnings unless I explicitly pass -A, so this is not good. Let me do a bit of analysis into what the impact is of adding those default packages for startup time and disk space... We would also want them, then, to be included in the core GAP spkg.

I have already done this on #26856 - including fixing the needed doctests. The disk space is small in this case, as all them are just GAP code. Apart from tomlib.

Tomlib was (as of #26856) part of database_gap.

GAP people really consider these packages core GAP, and keep them packages for historical/authoriship etc reasons.

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

comment:405 follow-up: Changed 12 months ago by dimpase

Once we distribute the original GAP tarball, I would not worry about 250Mb more or less. It is almost 2019, terabytes of disk space are dirt-cheap.

comment:406 Changed 12 months ago by gh-timokau

But terabytes of SSDs aren't. Also the download takes time.

comment:407 in reply to: ↑ 405 ; follow-up: Changed 12 months ago by embray

Replying to dimpase:

Once we distribute the original GAP tarball, I would not worry about 250Mb more or less. It is almost 2019, terabytes of disk space are dirt-cheap.

That's not relevant here except for people building Sage from source, for whom I don't care how much disk space we use within reason. I'm just concerned about download size for binary packages, and a third of a gigabyte is not trivial in that case.

comment:408 in reply to: ↑ 407 Changed 12 months ago by dimpase

Replying to embray:

Replying to dimpase:

Once we distribute the original GAP tarball, I would not worry about 250Mb more or less. It is almost 2019, terabytes of disk space are dirt-cheap.

That's not relevant here except for people building Sage from source, for whom I don't care how much disk space we use within reason. I'm just concerned about download size for binary packages, and a third of a gigabyte is not trivial in that case.

but that data are textfiles, after compression they will be much smaller.

comment:409 Changed 12 months ago by embray

Okay, first of all, to be precise, the default packages in question are:

autpgrp-1.10
alnuth-3.1.0
crisp-1.4.4
ctbllib
FactInt-1.6.2
fga
irredsol-1.4
laguna-3.9.0
polenta-1.3.8
polycyclic-2.14
resclasses-4.7.1
sophus-1.24
tomlib-1.2.7

With bz2 compression this squashes down to 66MB which, while not ideal, is somehow psychologically less troubling than 375MB, perhaps just being ~an order of magnitude smaller.

With LZMA compression (at the default compression level 6) it gets down to 58MB--this is what I use for the Windows installer. This will probably make installation take that much longer (decompressing the files from the Windows installer takes quite a long time). But I'm less concerned about that for now.

Last edited 12 months ago by embray (previous) (diff)

comment:410 follow-up: Changed 12 months ago by dimpase

Regarding the GAP startup time, this would hardly change, as loading a GAP workspace is blazing fast (I wish Python had this option...)

comment:411 in reply to: ↑ 402 Changed 12 months ago by dimpase

Replying to embray:

BTW building, installing, and loading the IO package works fine now, so I guess I fixed that somewhere along the line.

You mean, doing it in-place, like GAP does?

comment:412 in reply to: ↑ 410 ; follow-up: Changed 12 months ago by jdemeyer

Replying to dimpase:

I wish Python had this option...

The option of having an unreliable startup system which would crash whenever Python is upgraded or different packages are installed? :-)

comment:413 in reply to: ↑ 412 ; follow-up: Changed 12 months ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

I wish Python had this option...

The option of having an unreliable startup system which would crash whenever Python is upgraded or different packages are installed? :-)

Well, how often an application programmer updates Python? And surely, a package installation can trigger a workspace rebuild, just as what code you wrote to handle GAP workspaces does...

comment:414 follow-up: Changed 12 months ago by gh-timokau

Is it technically difficult to just make those gap packages an optional sage spkg?

comment:415 in reply to: ↑ 414 ; follow-up: Changed 12 months ago by dimpase

Replying to gh-timokau:

Is it technically difficult to just make those gap packages an optional sage spkg?

Not really, but we've decided to get rid of these optional things, I think. cf. sage-devel thread.

It's an extra complication resulting in chasing inconsistencies in GAP resulting from loading a different set of packages, textually different test results (and it annoys upstream).

comment:416 in reply to: ↑ 413 Changed 12 months ago by embray

Replying to dimpase:

Replying to jdemeyer:

Replying to dimpase:

I wish Python had this option...

The option of having an unreliable startup system which would crash whenever Python is upgraded or different packages are installed? :-)

Well, how often an application programmer updates Python? And surely, a package installation can trigger a workspace rebuild, just as what code you wrote to handle GAP workspaces does...

+1 for a general Python install it would be annoying as the default behavior, but lots of Python apps are using a virtualenv or some other alternate PYTHONPATH with a fixed, unchanging set of packages, and for this case it would be nice to have some kind of "frozen" workspace that can load quicker. One thing Python does have that GAP doesn't is pre-compiled bytecode modules, which does make a big difference, however. But it would be nice if one could somehow mark module-level code as "pure", such that it could be stored as a pickle or something without having to re-execute it. But that's getting off-topic...

comment:417 in reply to: ↑ 402 Changed 12 months ago by embray

Replying to embray:

BTW building, installing, and loading the IO package works fine now, so I guess I fixed that somewhere along the line.

Nevermind, this is still broken. I had it working, apparently, but possibly only after some manual tinkering. I need to figure out what I did previously to fix this. Maybe I didn't even fix it at all: LoadPackage("IO") works from GAP, but not from libgap.eval(...) in Sage :(

comment:418 Changed 12 months ago by git

  • Commit changed from 5e61d7b6a0da3aa53d8176fa1fb9353cc559b098 to 4d56e389afc507efc047ebb9a6308b0372f06fe0

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

8404d4dinstead of disabling autoloading of default packages, just install all default packages as a standard part of the GAP SPKG
4d56e38more fixes to the GAP SPKG installation

comment:419 Changed 12 months ago by embray

  • Description modified (diff)

I understand the issue with compiled packages now, and it is still broken, as nthiery reported originally at https://github.com/markuspf/gap/issues/1 (why on Markus's fork I'm not sure).

This is really more a problem with the build system for the individual compiled packages though, and I'm not sure it's something really within the scope of this ticket (unless there are some generic build package tools in GAP itself that we need to fix, which might be possible; I haven't checked). The shared libraries for compiled packages really need to be linked against libgap so that the loader can find GAP symbols from libgap.

I'm still slightly surprised by this and would appreciate a detailed explanation if someone can give one. I'd have assumed that if libgap.so had already been loaded, that the loader would still be able to resolve symbols from it when dlopen()-ing the GAP package. But it seems that in this case a DT_NEEDED entry is still needed for some subtle reason that I'm not quite getting.


New commits:

8404d4dinstead of disabling autoloading of default packages, just install all default packages as a standard part of the GAP SPKG
4d56e38more fixes to the GAP SPKG installation

comment:420 follow-up: Changed 12 months ago by dimpase

Do I understand that having a GAP kernel module linked against libgap.so means that it would only work with libgap, but not with gap binary?

If so, this work should really wait for another ticket.

comment:421 Changed 12 months ago by embray

From the dlopen docs

External references in the library are resolved using the libraries in that library's dependency list and any other libraries previously opened with the RTLD_GLOBAL flag. If the executable was linked with the flag "-rdynamic" (or, synonymously, "--export-dynamic"), then the global symbols in the executable will also be used to resolve references in a dynamically loaded library.

Okay, so that means this depends on what binding mode libgap itself is opened with when it gets loaded (which likely only happens when one of the Python modules that depends on libgap is loaded). I'm not sure exactly what that would be, but it appears sys.setdlopenflags might be useful for this purpose.

comment:422 Changed 12 months ago by git

  • Commit changed from 4d56e389afc507efc047ebb9a6308b0372f06fe0 to da909ef2a07cf361223dcbf552865201fff22167

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

18f2f43instead of disabling autoloading of default packages, just install all default packages as a standard part of the GAP SPKG
da909efmore fixes to the GAP SPKG installation

comment:423 follow-up: Changed 12 months ago by dimpase

one also needs to install pkg/atlasrep, a dependency of pkg/tomlib

(that's what I did, but you prefer to roll your own... https://git.sagemath.org/sage.git/commit/?id=d305273fcdd2c247a0c50b8432e2939bc9c6a0f6)

comment:424 in reply to: ↑ 420 ; follow-ups: Changed 12 months ago by embray

  • Status changed from needs_work to needs_review

Replying to dimpase:

Do I understand that having a GAP kernel module linked against libgap.so means that it would only work with libgap, but not with gap binary?

If so, this work should really wait for another ticket.

I'm inclined to agree it might be best to hold off on this. However, this little hack fixed the issue for me:

  • src/sage/libs/gap/util.pyx

    diff --git a/src/sage/libs/gap/util.pyx b/src/sage/libs/gap/util.pyx
    index 5e2805d..ed33453 100644
    a b import os 
    1818import signal
    1919import warnings
    2020
     21from posix.dlfcn cimport dlopen, dlclose, RTLD_NOW, RTLD_GLOBAL
    2122from libc.string cimport strcpy, strlen
    2223
    2324from cpython.exc cimport PyErr_SetObject, PyErr_Occurred, PyErr_Fetch
    cdef initialize(): 
    259260    global _gap_is_initialized, environ
    260261    if _gap_is_initialized: return
    261262
     263    # Hack to ensure that all symbols provided by libgap are loaded into the
     264    # global symbol table
     265    # Note: we could use RTLD_NOLOAD and avoid the subsequent dlclose() but
     266    # this isn't portable
     267    cdef void* handle
     268    handle = dlopen("libgap.so", RTLD_NOW | RTLD_GLOBAL)
     269    if handle == NULL:
     270        raise RuntimeError(
     271                "Could not dlopen() libgap.so even though it should already "
     272                "be loaded!")
     273    dlclose(handle)
     274
    262275    # Define argv and environ variables, which we will pass in to
    263276    # initialize GAP. Note that we must pass define the memory pool
    264277    # size!

at least on Linux. In theory it should work on Cygwin and macOS as well but I think that should be tested.

Otherwise, pending any other issues anyone wants to raise, I'm calling this "done".

comment:425 in reply to: ↑ 423 Changed 12 months ago by embray

Replying to dimpase:

one also needs to install pkg/atlasrep, a dependency of pkg/tomlib

I see...

What's strange is that if one of the default packages doesn't exist GAP makes a big warning about it, but if it simply is "unavailable" due to some of its dependencies being missing it just quietly does not load that package.

comment:426 Changed 12 months ago by git

  • Commit changed from da909ef2a07cf361223dcbf552865201fff22167 to d1c98f33d429a110e38b9576676ae3f3ea9faaec

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

d1c98f3also include the atlasrep package, which is a dependency of tomlib

comment:427 in reply to: ↑ 424 Changed 12 months ago by dimpase

Replying to embray:

Replying to dimpase:

Do I understand that having a GAP kernel module linked against libgap.so means that it would only work with libgap, but not with gap binary?

If so, this work should really wait for another ticket.

I'm inclined to agree it might be best to hold off on this. However, this little hack fixed the issue for me:

Otherwise, pending any other issues anyone wants to raise, I'm calling this "done".

I already asked about means to install the GAP packages that need compilation. Will we install and keep them in-place, at least for now, is this what you do with GAP's io?

comment:428 in reply to: ↑ 415 Changed 12 months ago by gh-timokau

Replying to dimpase:

Replying to gh-timokau:

Is it technically difficult to just make those gap packages an optional sage spkg?

Not really, but we've decided to get rid of these optional things, I think. cf. sage-devel thread.

It's an extra complication resulting in chasing inconsistencies in GAP resulting from loading a different set of packages, textually different test results (and it annoys upstream).

Ah makes sense. In general I'd prefer the sage core to be as lean as possible with everything that isn't essential being optional. But then again I also don't want to maintain the split packages myself, so I guess I don't feel strongly about it :)

comment:429 follow-up: Changed 12 months ago by dimpase

  • Reviewers set to Erik Bray, Dima Pasechnik, Jeroen Demeyer
  • Status changed from needs_review to positive_review
  • Work issues ironing out the last kinks deleted

Good to go, I think. I also think that Erik truly deserves an award for getting this very tricky ticket done!

comment:430 in reply to: ↑ 429 ; follow-up: Changed 12 months ago by gh-timokau

Replying to dimpase:

Erik truly deserves an award for getting this very tricky ticket done!

Agreed. Thank you for working on this Erik!

(Now if the ECL ticket finally gets resolved I think its possible to run sage with all system dependencies :))

comment:431 in reply to: ↑ 430 Changed 12 months ago by embray

Replying to gh-timokau:

Replying to dimpase:

Erik truly deserves an award for getting this very tricky ticket done!

Agreed. Thank you for working on this Erik!

(Now if the ECL ticket finally gets resolved I think its possible to run sage with all system dependencies :))

Which ECL ticket? Can you CC me on that? I've spent some time in the past working on the ECL integration with Sage. I didn't know there were still problems with using the system ECL.

comment:432 Changed 12 months ago by dimpase

  • Status changed from positive_review to needs_work

a small problem with permissions, only seen on OSX:

[gap-4.10.0] Copying package files from temporary location /Users/dima/sagetrac-mirror/local/var/tmp/sage/build/gap-4.10.0/inst to /Users/dima/sagetrac-mirror/local
[gap-4.10.0] cp: /Users/dima/sagetrac-mirror/local/./share/gap/pkg/ctbllib/tst/docxpl.tst: Permission denied

for some reason, the permissions of this file are rrr (probably in the tarball)

comment:433 Changed 12 months ago by dimpase

trying again, and getting

[gap-4.10.0] Copying package files from temporary location /Users/dima/sagetrac-mirror/local/var/tmp/sage/build/gap-4.10.0/inst to /Users/dima/sagetrac-mirror/local
[gap-4.10.0] cp: cannot overwrite directory /Users/dima/sagetrac-mirror/local/./share/gap/bin/x86_64-apple-darwin17.7.0-default64/src with non-directory /Users/dima/sagetrac-mirror/local/var/tmp/sage/build/gap-4.10.0/inst/Users/dima/sagetrac-mirror/local/./share/gap/bin/x86_64-apple-darwin17.7.0-default64/src
[gap-4.10.0] cp: /Users/dima/sagetrac-mirror/local/./share/gap/pkg/ctbllib/tst/docxpl.tst: Permission denied
[gap-4.10.0] ************************************************************************
[gap-4.10.0] Error copying files for gap-4.10.0.

looks like local/share/gap does not get wiped up completely, due to this read-only file. Indeed:

$ ls -l local/share/gap/pkg/ctbllib/tst/
total 2272
-rw-r--r--  1 dima  staff   57320 20 Dec 09:32 ambigfus.tst
-rw-r--r--  1 dima  staff   18162 20 Dec 09:32 ctblcons.g
-rw-r--r--  1 dima  staff  133327 20 Dec 09:32 ctblcons.tst
-rw-r--r--  1 dima  staff   10762 20 Dec 09:32 ctbldeco.tst
-rw-r--r--  1 dima  staff    7254 20 Dec 09:32 ctblj4.tst
-rw-r--r--  1 dima  staff    3230 20 Dec 09:32 ctbllib.tst
-rw-r--r--  1 dima  staff   58554 20 Dec 09:32 ctblpope.tst
-rw-r--r--  1 dima  staff    8265 20 Dec 09:32 ctocenex.tst
-rw-r--r--  1 dima  staff   17708 20 Dec 09:32 dntgap.tst
-r--r--r--  1 dima  staff   35292 18 Dec 21:28 docxpl.tst
-rw-r--r--  1 dima  staff   14865 20 Dec 09:32 hamilcyc.g
-rw-r--r--  1 dima  staff   40835 20 Dec 09:32 hamilcyc.tst
-rw-r--r--  1 dima  staff    4208 20 Dec 09:32 maintain.tst
-rw-r--r--  1 dima  staff  328346 20 Dec 09:32 mferctbl.gap
-rw-r--r--  1 dima  staff   64934 20 Dec 09:32 multfre2.tst
-rw-r--r--  1 dima  staff   19893 20 Dec 09:32 multfree.dat
-rw-r--r--  1 dima  staff   12505 20 Dec 09:32 multfree.g
-rw-r--r--  1 dima  staff   25647 20 Dec 09:32 multfree.tst
-rw-r--r--  1 dima  staff    8555 20 Dec 09:32 o8p2s3_o8p5s3.g
-rw-r--r--  1 dima  staff    9870 20 Dec 09:32 o8p2s3_o8p5s3.tst
-rw-r--r--  1 dima  staff   42530 20 Dec 09:32 probgen.g
-rw-r--r--  1 dima  staff  163817 20 Dec 09:32 probgen.tst
-rw-r--r--  1 dima  staff   19540 20 Dec 09:32 sporsolv.tst
-rw-r--r--  1 dima  staff    1218 20 Dec 09:32 testall.g
-rw-r--r--  1 dima  staff     639 20 Dec 09:32 testauto.g
-rw-r--r--  1 dima  staff    2237 20 Dec 09:32 testinst.g

notice the date of this file, it's 18th Dec, not 20th Dec!

comment:434 Changed 12 months ago by dimpase

Installation works if I just do rm -rf local/share/gap/ before starting it. So it looks like the cleaning of old files is not done with sufficient force, i.e. the clever uninstallation code fails to clean in such a case on OSX.

Can we add that rm -rf local/share/gap/ at the start?

comment:435 Changed 12 months ago by embray

This looks more like a problem to be addressed in the build system. I'll look into it. If we do anything here at all it would be fixing permissions on those files. I don't see why they would need to be like that.

comment:436 follow-up: Changed 12 months ago by dimpase

There of course no reason for GAP to distribute some read-only files. It'll chase it up with them.

comment:437 Changed 12 months ago by git

  • Commit changed from d1c98f33d429a110e38b9576676ae3f3ea9faaec to 53f07cc92ee670a7aa091eee1c056c488167ab13

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

53f07ccdisable parallel make for gap

comment:438 Changed 12 months ago by embray

I see, just that one file has goofed permissions in the tarball. I wonder why:

-r--r--r-- gap-jenkins/gap-jenkins   35292 2018-11-01 22:56 gap-4.10.0/pkg/ctbllib/tst/docxpl.tst

It's the only one like that.

comment:439 Changed 12 months ago by embray

  • Status changed from needs_work to positive_review

I don't think there's anything to do here for now. The uninstaller removes that file fine so long as it was part of the installation in the first place. As you and I have been monkeying around with the package, it's possible you manually put that file there, rather than it being placed there by the installer, in which case it would have been untracked, so the uninstaller would not have removed it. The installer then failed to overwrite the existing file. This wouldn't happen under normal usage.

I consider it a feature that the installer will fail if it tries to overwrite an existing file and can't, as that's an indication of an issue we'd want to know about (which in this case it was!) but not one we really need to worry about in this case.

comment:440 Changed 12 months ago by dimpase

OK, fine with me then. I have not realised how advanced the (un)installation system is.

By the way, why "disable parallel make for gap"? I never saw any problems with this.

comment:441 in reply to: ↑ 436 Changed 12 months ago by dimpase

Replying to dimpase:

There of course no reason for GAP to distribute some read-only files. It'll chase it up with them.

see https://github.com/gap-system/gap/issues/3126

comment:442 Changed 12 months ago by embray

On cygwin, and only on cygwin, I've started consistently getting the following failure:

sage -t --long src/sage/graphs/generators/families.py
**********************************************************************
File "src/sage/graphs/generators/families.py", line 3179, in sage.graphs.generators.families.MathonPseudocyclicStronglyRegularGraph
Failed example:
    L = sum(i*(r[a]-r[b]) for i,(a,b) in zip(range(1,len(ff)+1), ff)); L
Expected:
    [ 0 -1  1 -2 -3 -4  2  4  3]
    [ 1  0 -1 -4 -2 -3  3  2  4]
    [-1  1  0 -3 -4 -2  4  3  2]
    [ 2  4  3  0 -1  1 -2 -3 -4]
    [ 3  2  4  1  0 -1 -4 -2 -3]
    [ 4  3  2 -1  1  0 -3 -4 -2]
    [-2 -3 -4  2  4  3  0 -1  1]
    [-4 -2 -3  3  2  4  1  0 -1]
    [-3 -4 -2  4  3  2 -1  1  0]
Got:
    [ 0  1 -1 -3 -2 -4  3  4  2]
    [-1  0  1 -4 -3 -2  2  3  4]
    [ 1 -1  0 -2 -4 -3  4  2  3]
    [ 3  4  2  0  1 -1 -3 -2 -4]
    [ 2  3  4 -1  0  1 -4 -3 -2]
    [ 4  2  3  1 -1  0 -2 -4 -3]
    [-3 -2 -4  3  4  2  0  1 -1]
    [-4 -3 -2  2  3  4 -1  0  1]
    [-2 -4 -3  4  2  3  1 -1  0]
**********************************************************************
1 item had failures:
   1 of  17 in sage.graphs.generators.families.MathonPseudocyclicStronglyRegularGraph
    [363 tests, 1 failure, 61.19 s]

This didn't happen until recently. It might be a change introduced by one of the packages we're installing that weren't installed before, but it's still unclear why the failure would only occur on cygwin. It might be the sort of thing where the code is not strictly deterministic, but happens to work consistently between runs on the same platform. I'll double-check.

comment:443 Changed 12 months ago by dimpase

Oh, are we on the right ticket? Isn't this this change on #26856?

--- a/src/sage/graphs/generators/families.py
+++ b/src/sage/graphs/generators/families.py
@@ -3177,15 +3177,16 @@ def MathonPseudocyclicStronglyRegularGraph(t, G=None, L=None):
         sage: ff = list(map(lambda y: (y[0]-1,y[1]-1),
         ....:          Permutation(map(lambda x: 1+r.index(x^-1), r)).cycle_tuples()[1:]))
         sage: L = sum(i*(r[a]-r[b]) for i,(a,b) in zip(range(1,len(ff)+1), ff)); L
-        [ 0 -1  1 -2 -3 -4  2  4  3]
-        [ 1  0 -1 -4 -2 -3  3  2  4]
-        [-1  1  0 -3 -4 -2  4  3  2]
-        [ 2  4  3  0 -1  1 -2 -3 -4]
-        [ 3  2  4  1  0 -1 -4 -2 -3]
-        [ 4  3  2 -1  1  0 -3 -4 -2]
-        [-2 -3 -4  2  4  3  0 -1  1]
-        [-4 -2 -3  3  2  4  1  0 -1]
-        [-3 -4 -2  4  3  2 -1  1  0]
+        [ 0  1 -1 -3 -2 -4  3  4  2]
+        [-1  0  1 -4 -3 -2  2  3  4]
+        [ 1 -1  0 -2 -4 -3  4  2  3]
+        [ 3  4  2  0  1 -1 -3 -2 -4]
+        [ 2  3  4 -1  0  1 -4 -3 -2]
+        [ 4  2  3  1 -1  0 -2 -4 -3]
+        [-3 -2 -4  3  4  2  0  1 -1]
+        [-4 -3 -2  2  3  4 -1  0  1]
+        [-2 -4 -3  4  2  3  1 -1  0]
+
         sage: G.relabel()
         sage: G3x3=graphs.MathonPseudocyclicStronglyRegularGraph(2,G=G,L=L)
         sage: G3x3.is_strongly_regular(parameters=True

Yes, indeed, this is needed as more default packages are now loaded. Probably it makes sense to make #26856 dependence of this one.

Else we should get the needed part of the branch of #26856 into here.

I admit it's a workflow screwup on my side, primarily, as #26856 does get rid of database_gap, everything related to it must be here now, by right...

comment:444 Changed 12 months ago by dimpase

Specifically, looking at the history of #26856, one would have to put here everything, except for changes in in build/pkgs/gap_packages

comment:445 Changed 12 months ago by embray

Oh, I think I see the problem then: On Linux I happened to be on a branch on to which I added your work from #26856.

If I switch back to the branch for this ticket, that test fails on Linux too. Therefore I think that test fix should be moved to this ticket.

comment:446 Changed 12 months ago by dimpase

  • Status changed from positive_review to needs_work

comment:447 Changed 12 months ago by embray

In #26856 you had already added the additional default packages. I have now done that here, but didn't making the relevant adjustments for minor doctest failures. I'll just cherry-pick the relevant commits from your branch over to here.

comment:448 Changed 12 months ago by dimpase

I think you can just merge everything from #26856 and then rewrite everything in build/pkgs/gap_packages back to where is it was.

Might be faster.

comment:449 Changed 12 months ago by git

  • Commit changed from 53f07cc92ee670a7aa091eee1c056c488167ab13 to b446ebb496d45c4408aa949f98f855f962d9388a

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

b446ebbadjust doctests to accommodate more GAP methods

comment:450 Changed 12 months ago by embray

  • Status changed from needs_work to needs_review

This was the only commit from #26856 that I believe is truly necessary here. With this, I confirmed that all the tests that were failing for me now pass on linux and cygwin. Though I'm doing one last ptestlong just to double-check.

Dima, were there any other specific changes from the other ticket that you think would be appropriate to move here? Otherwise you can set back to positive review, or double-check if you want.

comment:451 Changed 12 months ago by dimpase

  • Status changed from needs_review to positive_review

Looks good to me. As on this branch database_gap spkg and tags are still present, the latter make sure that lots of otherwise broken tests are skipped.

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

comment:452 in reply to: ↑ 424 Changed 12 months ago by dimpase

Replying to embray:

Replying to dimpase:

Do I understand that having a GAP kernel module linked against libgap.so means that it would only work with libgap, but not with gap binary?

If so, this work should really wait for another ticket.

at least on Linux. In theory it should work on Cygwin and macOS as well but I think that should be tested.

this is continued on #26930 (OSX needs a bit more work)

comment:453 Changed 12 months ago by vbraun

  • Branch changed from u/embray/spkgs/gap-410 to b446ebb496d45c4408aa949f98f855f962d9388a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:454 Changed 12 months ago by embray

  • Commit b446ebb496d45c4408aa949f98f855f962d9388a deleted
  • Milestone changed from sage-8.5 to sage-8.6

Sage-8.6 I guess.

comment:455 Changed 12 months ago by gh-timokau

I'm trying to package this and in the process updating gap to 4.10. I really wish gap had a workable make install.

gap builds and tests fine. It works interactively. However when trying to use it through sage's pexepct interface, it hangs. Any idea what may be going wrong here?

sage: a = Gap(logfile = 'gap.log')
sage: a.__dict__
{'_Expect__command': 'gap -r -L /home/timo/.sage/gap/gap-workspace-0x25243fbcd256d4e0 -b -p -T -E -o 622m -s 622m -m 64m  /nix/store/gh48l47qc1irys2xpbwf56g2lwa96pcs-sage-src-8.6.beta0/src/ext/gap/sage.g',
 '_Expect__do_cleaner': True,
 '_Expect__init_code': [],
 '_Expect__initialized': False,
 '_Expect__is_remote': False,
 '_Expect__logfile': None,
 '_Expect__logfilename': 'gap.log',
 '_Expect__path': '/home/timo/repos/nixpkgs/sage-8.6',
 '_Expect__remote_cleaner': False,
 '_Expect__seq': -1,
 '_Expect__verbose_start': False,
 '_Gap__make_workspace': False,
 '_Gap__seq': 0,
 '_Gap__use_workspace_cache': True,
 '_Interface__coerce_name': '_gap_',
 '_Interface__name': 'gap',
 '_Interface__seq': -1,
 '_available_vars': [],
 '_env': {},
 '_eval_using_file_cutoff': 100,
 '_expect': None,
 '_prompt': 'gap> ',
 '_restart_on_ctrlc': True,
 '_seed': None,
 '_server': None,
 '_session_number': 0,
 '_terminal_echo': True}
sage: a('42;') # hangs and doesn't produce a gap.log file

Meanwhile executing the _Expect_command manually (through sage -gap, without -p and without -L) works as expected.

comment:456 Changed 12 months ago by dimpase

this works for me. Check out you don’t have stale GAP workspaces lying around in ~/.sage/gap/

comment:457 Changed 12 months ago by gh-timokau

Yes its almost certainly not an error with this ticket but some packaging error or some assumption that isn't valid for NixOS. I'm just asking here because I thought you or Erik may have thoughts on possible causes.

Removing all files in ~/.sage/gap (or just disabling workspaces) doesn't help unfortunately.

comment:458 Changed 11 months ago by embray

The main reason I know the pexpect interface can hang is if the xgap package is loaded. Search the discussion above for "xgap". This is supposed to be fixed by 8228bace to ensure that the xgap package is not autoloaded. You'll want to check whether that fix is working as intended though.

It might also help to patch the pexpect interface to not hang in this case, but that's more complicated because it has to "talk" to the GAP interpreter as though it were correctly emulating a graphical interface. Better to just make sure xgap isn't loaded.

comment:459 Changed 11 months ago by gh-timokau

That does seem to be the problem, thanks! If I add rm -rf pkg/xgap* to the gap build recipe, the pexpect interface works. So the fix in ​8228bace apparently does work as intended. Any idea why?

comment:460 Changed 11 months ago by embray

I assume you mean does not work as intended. As for why/why not you'll have to debug that yourself I suppose. It works for me. It could be that you have some other auto-loaded package that also loads the xgap package unconditionally. Most packages that use the xgap package at least check to ensure that the -p flag was passed to GAP before attempting to load it. It's possible something more forceful is needed to prevent ever loading the xgap package at all (perhaps, if there's some way to disable it completely).

comment:461 Changed 11 months ago by gh-timokau

It looke like PackagesToIgnore, which just pretends the package doesn't exist, is more robust than ExcludeFromAutoload.

I am able to workaround the issue by creating a ~/.gap/gap.ini file (which for some reason is not actually an ini file) with the contents

SetUserPreference( "PackagesToIgnore", [ "xgap" ] );

And monkey-patching away the -r flag passed to sage:

rm -rf ~/.sage/gap; result/bin/sage -c 'import sage.interfaces.gap as g; g.gap_cmd = "gap"; Gap()(42)'

That terminates. I wasn't able to do the same with sage.g however -- maybe it is just loaded too late? I tried using SetUserPreference as well as GAPInfo.PackagesToIgnore.

Maybe we could make use of the --roots argument to provide a gap.ini file?

comment:462 Changed 11 months ago by embray

The problem with SetUserPreference is that it's applied too early. That's why I didn't use it in sage.g. Instead you can do like I did and assign directly to the value that is set via the SetUserPreference call. In this case I'm not sure exactly which that is; I'd have to check. Please open a new ticket for this and CC me on it.

comment:463 Changed 11 months ago by gh-timokau

#26983

I naively tried GAPInfo.PackagesToIgnore, but that didn't work. A quick grep through the gap source only shows it being accessed through UserPreference("PackagesToIgnore"). I don't know enough about gap to do anything useful with that.

comment:464 follow-up: Changed 11 months ago by chapoton

It seems that the new libgap cause some doctests failure with some patchbots. Somehow the gap doc is not displayed using unicode characters..

See for example:

https://patchbot.sagemath.org/log/26917/LinuxMint/18.2/x86_64/4.4.0-138-generic/rk02-math/2018-12-30%2007:36:36

comment:465 in reply to: ↑ 464 Changed 11 months ago by dimpase

Replying to chapoton:

It seems that the new libgap cause some doctests failure with some patchbots. Somehow the gap doc is not displayed using unicode characters..

See for example:

https://patchbot.sagemath.org/log/26917/LinuxMint/18.2/x86_64/4.4.0-138-generic/rk02-math/2018-12-30%2007:36:36

all these should be fixed by rc1 (out today).

comment:466 Changed 11 months ago by chapoton

This is not fixed in 8.6.beta1. See #26987

comment:467 follow-up: Changed 11 months ago by gh-timokau

I need to revert https://git.sagemath.org/sage.git/patch/?id=b446ebb496d45c4408aa949f98f855f962d9388a to pass the doctests, even though I'm using gap 4.10. Any idea why that output should have changed? Should we maybe just sort it?

comment:468 in reply to: ↑ 467 Changed 11 months ago by thansen

Replying to gh-timokau:

I need to revert https://git.sagemath.org/sage.git/patch/?id=b446ebb496d45c4408aa949f98f855f962d9388a to pass the doctests, even though I'm using gap 4.10. Any idea why that output should have changed? Should we maybe just sort it?

I noticed this too (have to revert the commit) in Debian.

comment:469 Changed 11 months ago by gh-timokau

Hm I still had a patch lying around that applied -A to gap. Without that and with the exact same package set the spkg uses I do get the behaviour introduced in the patch. So it is changed by one of the autoloaded packages. Still probably best to just sort the output.

comment:470 Changed 11 months ago by gh-timokau

Another weird "issue" (with sage-on-nix with the package set from the spkg): I get one more blankline at the start of docs than sage expects.

Failed example:
    print(gap.help('SymmetricGroup', pager=False))
Expected:
    <BLANKLINE>
      50.1-... SymmetricGroup
    <BLANKLINE>
      ‣ SymmetricGroup( [filt, ]deg ) ─────────────────────────────────── function
    ...
    <BLANKLINE>
Got:
    <BLANKLINE>
    <BLANKLINE>
      50.1-10 SymmetricGroup
    <BLANKLINE>
      > SymmetricGroup( [filt, ]deg ) ----------------------------------- function
      > SymmetricGroup( [filt, ]dom ) ----------------------------------- function
    <BLANKLINE>
      constructs  the  symmetric  group of degree deg in the category given by the
      filter  filt.  If filt is not given it defaults to IsPermGroup (43.1-1). For
      more  information  on  possible  values  of  filt see section (50.1). In the
      second  version,  the  function constructs the symmetric group on the points
      given in the set dom which must be a set of positive integers.
    <BLANKLINE>
Last edited 11 months ago by gh-timokau (previous) (diff)

comment:471 Changed 11 months ago by embray

This ticket is closed so please make new tickets for follow-up issues.

Note: See TracTickets for help on using tickets.