Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to GAP 4.10 #22626

Closed
nthiery opened this issue Mar 16, 2017 · 541 comments
Closed

Upgrade to GAP 4.10 #22626

nthiery opened this issue Mar 16, 2017 · 541 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Mar 16, 2017

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 Unprefixed libgap #19915.

  • Removes configure.patch: it was patching configure for better GMP
    detection under Cygwin (New Gap spkg (>=4.5) does not build with shared only GMP/MPIR #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 GAP: use newer version of config.guess #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: markuspf/gap#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

Depends on #26874

CC: @alex-konovalov @dimpase @embray @kiwifb @antonio-rojas @sebasguts @jpflori @sagetrac-markuspf @nthiery @slel @vbraun @williamstein @timokau

Component: interfaces

Keywords: days85, libgap

Stopgaps: error handling in libgap, documentation display

Author: Nicolas M. Thiéry, Dima Pasechnik, Erik Bray, Jeroen Demeyer

Branch: b446ebb

Reviewer: Erik Bray, Dima Pasechnik, Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/22626

@nthiery nthiery added this to the sage-8.0 milestone Mar 16, 2017
@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented Mar 16, 2017

Branch: u/nthiery/upgrade_to_gap_4_9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 16, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 16, 2017

Commit: 3011ac0

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented Mar 17, 2017

Work Issues: Wait for gap 4.9 release

@nthiery
Copy link
Contributor Author

nthiery commented Mar 17, 2017

Changed keywords from none to days85, libgap

@nthiery

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2017

Changed commit from 3011ac0 to 234c54b

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor Author

nthiery commented Mar 17, 2017

Attachment: ptestlong.log

@nthiery

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2017

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, ...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2017

Changed commit from 234c54b to 431845f

@nthiery
Copy link
Contributor Author

nthiery commented Mar 17, 2017

New commits:

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

@nthiery

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2017

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, ...

@embray
Copy link
Contributor

embray commented Dec 28, 2018

Changed commit from b446ebb to none

@embray
Copy link
Contributor

embray commented Dec 28, 2018

comment:454

Sage-8.6 I guess.

@embray embray modified the milestones: sage-8.5, sage-8.6 Dec 28, 2018
@timokau
Copy link
Contributor

timokau commented Dec 29, 2018

comment:455

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.

@dimpase
Copy link
Member

dimpase commented Dec 30, 2018

comment:456

this works for me. Check out you don’t have stale GAP workspaces lying around in ~/.sage/gap/

@timokau
Copy link
Contributor

timokau commented Dec 30, 2018

comment:457

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.

@embray
Copy link
Contributor

embray commented Dec 31, 2018

comment:458

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.

@timokau
Copy link
Contributor

timokau commented Dec 31, 2018

comment:459

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?

@embray
Copy link
Contributor

embray commented Dec 31, 2018

comment:460

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).

@timokau
Copy link
Contributor

timokau commented Dec 31, 2018

comment:461

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?

@embray
Copy link
Contributor

embray commented Dec 31, 2018

comment:462

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.

@timokau
Copy link
Contributor

timokau commented Dec 31, 2018

comment:463

#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.

@fchapoton
Copy link
Contributor

comment:464

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

@dimpase
Copy link
Member

dimpase commented Jan 1, 2019

comment:465

Replying to @fchapoton:

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).

@fchapoton
Copy link
Contributor

comment:466

This is not fixed in 8.6.beta1. See #26987

@timokau
Copy link
Contributor

timokau commented Jan 2, 2019

comment:467

I need to revert https://github.com/sagemath/sagetrac-mirror/commit/b446ebb496d45c4408aa949f98f855f962d9388a.patch 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?

@tobihan
Copy link

tobihan commented Jan 2, 2019

comment:468

Replying to @timokau:

I need to revert https://github.com/sagemath/sagetrac-mirror/commit/b446ebb496d45c4408aa949f98f855f962d9388a.patch 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.

@timokau
Copy link
Contributor

timokau commented Jan 2, 2019

comment:469

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.

@timokau
Copy link
Contributor

timokau commented Jan 3, 2019

comment:470

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>

@embray
Copy link
Contributor

embray commented Jan 3, 2019

comment:471

This ticket is closed so please make new tickets for follow-up issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests